Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support ndjson for application/json requests #4969

Closed
wants to merge 4 commits into from
+47 −0
Closed

Support ndjson for application/json requests #4969

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f932a10
Support ndjson for application/json requests
pinkasey Dec 19, 2022
c3c3d34
CR comments
pinkasey Mar 16, 2023
270cb85
Merge branch 'master' into master
pinkasey Mar 16, 2023
b5f88aa
minor
pinkasey Mar 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/cli/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@
"swagger-ui-express": "^4.3.0",
"tslib": "1.14.1",
"typeorm": "0.2.45",
"type-is": "^1.6.18",
"uuid": "^8.3.2",
"validator": "13.7.0",
"winston": "^3.3.3",
Expand Down
48 changes: 48 additions & 0 deletions packages/cli/src/Server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,11 +161,56 @@ import { getLicense } from '@/License';
import { corsMiddleware } from './middlewares/cors';

require('body-parser-xml')(bodyParser);
const typeis = require('type-is');

const exec = promisify(callbackExec);

export const externalHooks: IExternalHooksClass = ExternalHooks();

function parseNdjson(str: string): any[] {
const result: any[] = [];
const items = str
.split('\n')
.map((item: string) => item.trim())
.filter((item: string) => item !== '');
for (const item of items) {
try {
result.push(JSON.parse(item));
} catch (e) {
throw Error(
// eslint-disable-next-line prettier/prettier
`error parsing ndjson, while parsing item ${result.length + 1} of ${result.length}: ${e.message}`,
{ cause: e },
);
}
}
return result;
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

function parseNdjson(str: string): any[] {
  const items = str.trim().split('\n');
  return items.map((item, i) => {
    try {
      return JSON.parse(item);
    } catch (e) {
      throw new Error(
        `error parsing ndjson, while parsing item ${i + 1} of ${items.length}: ${e.message}`,
      );
    }
  });
}

version of the code is equivalent to the original code, but it uses the map method to transform the array of strings into an array of parsed JSON objects, rather than using a for loop. This can make the code easier to read and understand. Additionally, the trim method is used to remove any leading or trailing whitespace from the input string, and the filter method is removed since it is not necessary. Finally, the error message is modified to correctly reference the current item being parsed in the map callback.

Copy link
Author

@pinkasey pinkasey Jan 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
The code looks better indeed, more readable and there's less memory allocation - trimming each line allocates a new string.

About removing the filter -
an empty line is acceptable according to the spec:

The parser MAY silently ignore empty lines, e.g. \n\n. This behavior MUST be documented and SHOULD be configurable by the user of the parser.

From my experience, API providers WILL do stupid things like putting empty-lines in the middle of an ndjson, occasionally or all the time (reminder - I opened this PR because of an API provider that did not adhere to standard).
It would be a shame that the n8n user get stuck because of this.

So I think it's better to filter out empty-lines nevertheless.
I wish there was a str.isEmpty() that does not allocates a new string.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree, it would be fab to have a str.isEmpty() method that does not allocate a new string, but for now, I think, using the trim method is a good solution to avoid unnecessary memory allocation. No?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about what i wrote about filtering out empty lines?
Do you agree?

function onJsonErrorTryNdjson() {
return (err: any, req: express.Request, res: express.Response, next: express.NextFunction) => {
if (
typeis(req, ['application/json', 'application/x-ndjson']) &&
err.type === 'entity.parse.failed'
) {
let body = err.body;
if (body.match(/^\s*\{[^\n]*}\s*(\n\s*\{[^\n]*}\s*)+$/)) {
try {
const result = parseNdjson(body);
req.body = result;
req.rawBody = Buffer.from(`[${result.join(',\n')}]`);
res.status(200);
next();
return; //this is the only "happy flow" - in every other case we continue with the original error
} catch (e) {
console.error(e);
}
}
}
next(err);
};
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

function onJsonErrorTryNdjson() {
  return (err: any, req: express.Request, res: express.Response, next: express.NextFunction) => {
    if (
      typeis(req, ['application/json', 'application/x-ndjson']) &&
      err.type === 'entity.parse.failed'
    ) {
      let body = err.body;
      const match = body.match(/^\s*\{[^\n]*}\s*(\n\s*\{[^\n]*}\s*)+$/);
      if (match) {
        try {
          const result = parseNdjson(body);
          req.body = result;
          req.rawBody = Buffer.from(`[${result.join(',\n')}]`);
          res.status(200);
          next();
          return;
        } catch (e) {
          console.error(e);
        }
      }
    }
    next(err);
  };
}

This code is equivalent to the original code, but it uses the match method to check if the body string matches the regular expression, rather than using the match method in a boolean context. This can make the code easier to read and understand. Additionally, the return statement at the end of the if block is moved to the end of the block, to improve readability and to avoid having to use a comment to indicate the end of the "happy flow" code path. Finally, the variable match is introduced to store the result of the match method, so that it can be used in the if statement.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this ChatGPT?

Additionally, the return statement at the end of the if block is moved to the end of the block...

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the changes made to the code were intended to improve its readability and maintainability, as when I looked at it at first, I wasn't sure that's the best option to go about it.

You are basically using the match method in a boolean context, which makes the whole thing not clear (at least to me), and not obvious what is being checked for. I used the match method to check if the body string matches the rregex, and I think this is more explicit and straightforward.

I hope that explains things.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lol, but appreciate the doubt :)

class App {
app: express.Application;

Expand Down Expand Up @@ -666,6 +711,9 @@ class App {
}),
);

// Support ndjson, by converting the ndjson to an array of objects (only if the body is indeed ndjson, with more than one line)
this.app.use(onJsonErrorTryNdjson());

// Support application/xml type post data
this.app.use(
// @ts-ignore
Expand Down
2 changes: 2 additions & 0 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.