Support ndjson for application/json requests

[pinkasey]

Backstory -

I have a workflows that starts with a webhook.
This webhook is called with a request with Content-Type: application/json, and ndjson body, like this:

{ "id": 1, "message": "hello" }
{ "id": 2, "message": "I'm a JSON inside a NDJSON" }

I am completely unable to make the webhook work, because the parsing of the json is happening in Server.ts, so fiddling with the workflow settings (e.g, setting rawBody to true) - has no effect.
I get an error in the log, and the workflow is not started.

What I propose in this PR -

If a request is made with Content-Type: application/json, and parsing the body as json fails -
try to parse it as ndjson.

• If that works - great! return an array of all the jsons in the body (1 line = 1 item)
• if not - log an error, and pass the original answer.

Note:
In a perfect world, the API I'm consuming would send the request to the webhook with application/x-ndjson.
But I have no control over that - it's a 3rd party. And that would be the experience of most users of n8n.
I also tried to see what would've happened if the Content-Type was right - I got an empty body.

This PR is related to this feature-request from body-parser:
expressjs/body-parser#478
(If that feature-request is handled, then we could use it here instead of this PR. But I don't know if they'd answer)

[CLAassistant]

All committers have signed the CLA.

[whoiskatrin]

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.

[pinkasey]

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.

[whoiskatrin]

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?

[pinkasey]

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

[whoiskatrin]

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.

[pinkasey]

is this ChatGPT?

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

[whoiskatrin]

What do you mean?

[whoiskatrin]

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.

[whoiskatrin]

Lol, but appreciate the doubt :)

[whoiskatrin]

General questions to this approach:

• what if you cant determine the type of the request body based on the Content-Type header (for example, incorrect header? or if the type is generic)

[pinkasey]

General questions to this approach:

• what if you cant determine the type of the request body based on the Content-Type header (for example, incorrect header? or if the type is generic)

If we can't determine the type of the request body based on the Content-Type header, then we continue as usual, no change there.
I believe the default body type is plain text.
If that causes an error - very well. An incorrect Content-Type is a serious error by the client.

[pinkasey]

I've accepted CR comments, and rebased this branch.

I've since stopped using n8n, so I will not make further changes to this PR.
I hope this PR can be of help to others.

[netroy]

Sorry that this never got reviewed or merged. Unfortunately this can't be rebased anymore, since Server.ts has changed quite a lot.

If anyone needs ndjson support on webhooks, you can enable the "Binary Data" option on the webhook, and then parse this data in a "Code" node.

If you think this should actually be a feature in n8n, then please create a Feature request, so that this can first be discussed with the team before anything is implemented 🙏🏽