feat(rest): switch to body parser and add extensibility for parsers #1936

raymondfeng · 2018-10-29T16:28:45Z

Recreation of #1887

This PR adds the following features:

switches to express body-parser:

body-parser is more actively maintained that body
body-parser supports compression options

Allow body parsers to be plugged in

define an interface for BodyParser
allow body parser extensions to be bound to the context with REQUEST_BODY_PARSER_TAG.

Checklist

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
API Documentation in code was updated
Documentation in /docs/site was updated
Affected artifact templates in packages/cli were updated
Affected example projects in examples/* were updated

Documentation TODO as seen by @bajtos:

How to build extensions providing custom body parsers.
- Show them how to contribute custom body parsers, the guide should mention the proposed helper function for creating bindings (createBodyParserBinding).
- Describe executeBodyParserMiddleware, this function can be very useful to extension authors building custom body parsers, because it effectively allows them to take any Express middleware performing body parsing and wrap it into LB4 RequestParser. It would be great to change it into a well-documented public API.
How to consume request body as a stream
- In the documentation for this new feature, use the scenario where the app/extension developer wants to consume the request body as a stream to drive the narrative.
- I can imagine this new documentation can take a form of a new page in the section "How tos", plus a short mention of the new extension flag in apidocs and/or @requestBody overview.

bajtos · 2018-11-05T10:45:32Z

@raymondfeng what's the status of this pull request, is it ready for review? I see a conflict in packages/rest/package.json reported by GitHub.

raymondfeng · 2018-11-05T16:39:26Z

@bajtos I resolved the conflict and it's ready for review.

raymondfeng · 2018-11-08T15:49:59Z

@bajtos PTAL

bajtos · 2018-11-09T16:18:35Z

Sorry, the GitHub notification got buried in my inbox, I'll review early next week.

bajtos

Besides the comments below, the new extension points (custom body parsers, x-skip-body-parsing) need a documentation.

It would be also great to have a new example app showing a reference implementation of file uploads, ideally with a simple HTML5 front-end that we can open in browser to play with this functionality. I think that's out of scope of this pull request though.

docs/site/Parsing-requests.md

packages/rest/src/body-parser.ts

packages/rest/src/providers/parse-params.provider.ts

bajtos · 2018-11-12T15:29:48Z

packages/rest/test/acceptance/validation/validation.acceptance.ts

+      await client
+        .post('/products')
+        .type('json')
+        .expect(422);


With this change in place, the comment at the top of the test is inconsistent with what is the test asserting. Please take a closer look, I would like us to understand how are empty payloads handled with the new request handler in place.

PTAL ☝️

Still not addressed ☝️

❗️ Is this change in the test hinting at breaking backwards compatibility?

It turns out that express json body parser creates {} for empty body. I added some logic in json parser to treat empty body as undefined.

packages/rest/test/integration/http-handler.integration.ts

raymondfeng · 2018-11-13T05:33:01Z

@bajtos PTAL

bajtos

I love the new file structure, it makes the code so much easier to read and navigate around.

The code in individual built-in parser classes seems to be duplicated a lot, have you considered using a Strategy or Template design pattern to extract a shared base class?

I run out of time while reviewing the changes and did not manage to review the changes in test files yet. Sorry for that.

packages/rest/src/body-parsers/body-parser.json.ts

packages/rest/src/body-parsers/body-parser.text.ts

bajtos · 2018-11-13T15:33:36Z

packages/rest/src/keys.ts

+    'rest.requestBodyParser',
+  );
+
+  export const REQUEST_BODY_PARSER_JSON = BindingKey.create<BodyParser>(


Is it a good idea to turn binding keys of individual parsers into a public API? Personally, I would expect that these keys should be treated as an implementation detail, similarly to how we are not making much guarantees about the binding keys for controllers, repositories and routes.

@raymondfeng you haven't addressed nor responded to this comment. PTAL again.

Let's discuss this point before landing the pull request. If we wanted to remove these public keys later, then we would have to wait until we are ready for a semver-major version.

packages/rest/src/parser.ts

packages/rest/src/rest.component.ts

packages/rest/src/rest.server.ts

bajtos · 2018-11-13T15:45:46Z

packages/rest/test/acceptance/validation/validation.acceptance.ts

+      await client
+        .post('/products')
+        .type('json')
+        .expect(422);


PTAL ☝️

bajtos · 2018-11-13T18:08:37Z

@raymondfeng I would like you to consider the following high-level scenarios, I am not sure if the current design for body-parser registration is flexible enough to support them.

While the canonical content-type for JSON payloads is application/json, the alternative text/json used to be pretty popular. (See https://stackoverflow.com/q/477816/69868 for more alternative types). Should LB4 parse text/json as JSON, not as plain text? If yes, then please add a test for this.

This scenario came to my attention because I incorrectly assumed that text/json would be accepted by both JsonParser and TextParser. It turns out that typeis.is(contentType, 'json') does not recognize text/json, so this is not really a concern right now. But:

It opens a more generic question of the priority of body parsers.

Let's say the user has two parsers, one recognizing */*+json (a generic JSON parser) and the other one recognizing application/vnd.api+json (a custom parser for JSON-API payloads). How can the developer control which parser will take precedence? What if the generic JSON parser is contributed by @loopback/rest and the JSON-API parser is contributed by another component (e.g. @loopback/jsonapi)? What if both parsers are contributed by 3rd party components that are not aware of each other?

Do we need to sort the parsers from most-specific to most-generic?

To support all JSON-based content-types, should we broaden content types accepted by our built-in parser to allow any of ['*/json', 'application/*+json'], for example text/json and application/vnd.api+json?

raymondfeng · 2018-11-13T22:37:48Z

@bajtos We can possibly use the http accept header format to define the q-factor of accepted media types for any given body parser - see https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html. For example, a JSON body parser can declare:

accept: 'application/vnd.x.json; q=0.5, application/json'

Such features can be done outside of this PR.

Please note that http content negotiation utilizes the Accept header for matching response media types. We can borrow the idea.

bajtos · 2018-11-14T08:16:44Z

We can possibly use the http accept header format to define the q-factor of accepted media types for any given body parser.

I am not sure if that will work, to be honest.

Such features can be done outside of this PR.

I am fine to leave it outside of this PR, but I am concerned that before we do at least some minimal implementation, then we will not know whether the proposed approach is actually feasible :(

Can we at least put the built-in parsers to the end of the list, so that user-defined parsers always take precedence? It may be enough to sort the list of injected parsers in reverse, so that the last parser registered will be the first parser to have a chance to process the request.

bajtos · 2018-11-14T08:18:27Z

I was thinking about this problem more and have another idea to consider.

In Express, middleware preprocessing requests (e.g. parsing the body) does not have any route-specific metadata to use as a guide to decide which behavior to use (which parser to execute). In LoopBack 4, routes provide OpenAPI spec which is allows us to provide additional instructions for body parsing (accepted content types + extra metadata for each type, e.g. schema).

So I was thinking about the following solution, which will cover both selection of the right parser and the ability to skip body parsing.

Restrict the built-in body parsers to a single type only (text/plain, application/javascript, etc.).
Define a new OpenAPI extension allowing developers to choose which parser to use. This extension can be called e.g. x-parser and allow string values (parser names) & parser classes (constructor functions). We can define a special parser called e.g. stream that will be a no-op.
When the operation provides x-parser setting, we skip content-type matching algorithm and use the selected parser regardless of whether it is accepting the actual content-type.

Few examples:

// parse text/json as JSON
@requestBody({
  content: {
    'text/json': {
      'x-parser': 'json',
      schema: {/*...*/},
    },
  }
})

// parse custom type as text
@requestBody({
  content: {
    'text/calendar': {
      'x-parser': 'text',
      schema: {/*...*/},
    },
  },
})

// consume unsupported type as a stream (skip body parsing)
@requestBody({
  content: {
    'multipart/form-data': {
      'x-parser': 'stream',
      schema: {(/*...*/},
    },
  },
});

// override handling of a known type - consume the body as a stream
@requestBody({
  content: {
    'application/json': {
      'x-parser': 'stream',
      schema: {(/*...*/},
    },
  },
});

// provide a custom parser class
@requestBody({
  content: {
    'multipart/form-data': {
      'x-parser': FileUploadParser,
      schema: {(/*...*/},
    },
  },
});

class FileUploadParser implements BodyParser {
  // ...
}

I find this approach very elegant, because we can leverage the same mechanism (x-parser extension) for both customizing which parser is used and disabling body parsing entirely. It also avoids automagic algorithm for deciding which parser to use, thus it should make life easier both for our users (it's easier to learn and understand how parser selection works, because it's explicit) and us, the maintainers (less confused users = less time spent on support, no need to build & maintain complex code like handling of weight parameter q).

We can even push this approach one step further and always use "stream" (no-op) body parser for content types that don't have any matching parser. I.e. introduce a new default behavior where bodies with unsupported content types are automatically passed to controller methods/route handlers as streams.

This is based on the assumption that our body parsing step is accepting only the content types described in the OpenAPI spec, and returning "Unsupported Media Type" for requests with other content types.

@raymondfeng @strongloop/loopback-next thoughts?

raymondfeng · 2018-11-14T17:15:45Z

@bajtos I have implemented the x-parser proposal. PTAL.

bajtos

I am glad you liked my x-parser proposal, I think the updated implementation looks much better.

Besides the comments I am leaving below, I am missing tests for the following scenarios/cases:

Acceptance-level tests at RestApplication level showing how to:

Show how to configure the app to parse all incoming text/json requests using JsonBodyParser.
Override a built-in body parser, e.g. configure a custom parser to handle application/json type for all incoming requests.
Disable a built-in body parser, e.g. configure the app to always reject url-encoded requests, even if the spec allows that media type. (Maybe this isn't very useful and there is no need for such test?)

Specifically, these tests must not use x-parser extension.

Further scenarios, I think they are more suited for integration tests of unit tests:

What happens when a custom parser returns undefined.
How is each built-in parser dealing with an empty request body. Does it throw 400, pass undefined or an empty string, something else?
When a custom parser throws an error, it should be normalized consistently with errors reported by built-in parser.
A test showing why we need error normalization for built-in parsers, i.e. setup a scenario where a built-in parser returns a "wrong" error and we need to normalize it.

Documentation wise, I am missing guide for extension developers showing them how to write an extension contributing custom parsers. I am ok to leave this out of this PR as long as you create a follow-up issue.

docs/site/Parsing-requests.md

packages/context/src/binding.ts

packages/rest/src/body-parsers/body-parser.helpers.ts

packages/rest/test/integration/http-handler.integration.ts

packages/rest/test/unit/coercion/utils.ts

packages/rest/test/unit/parser.unit.ts

bajtos · 2018-11-15T15:48:52Z

Cross-posting a part of one of the line comments that I consider as still relevant:

It would be even more cool if we could have two acceptance tests for file upload:

one showing how to handle file uploads by registering a custom request body parser
the other showing how to handle file uploads in individual controller methods by receiving the request body as a stream.

bajtos · 2018-11-15T16:00:01Z

@raymondfeng To make my further reviews easier, please don't amend any existing commits and push new commits only. (Rebasing on top of master is fine.) That way I can review only new increments and don't have to re-read all ~1.6k changed lines again.

Considering the scope of the changes proposed in this pull request, I am not sure if it still makes sense to preserve the current series of smaller commits. If it's easier for you to stop splitting the changes into small commits, with the vision of landing this entire PR in one or two big commits, then feel free to do so. (As long as I have an easy way how to see what has changed since my last review.)

raymondfeng · 2018-11-16T18:35:00Z

It would be even more cool if we could have two acceptance tests for file upload:
one showing how to handle file uploads by registering a custom request body parser
the other showing how to handle file uploads in individual controller methods by receiving the request body as a stream.

See test/acceptance/file-upload.

raymondfeng · 2018-11-16T18:44:13Z

@bajtos PTAL.

packages/rest/src/body-parsers/body-parser.raw.ts

packages/rest/src/body-parsers/body-parser.ts

bajtos · 2018-11-19T14:36:27Z

packages/rest/src/keys.ts

+    'rest.requestBodyParser',
+  );
+
+  export const REQUEST_BODY_PARSER_JSON = BindingKey.create<BodyParser>(


@raymondfeng you haven't addressed nor responded to this comment. PTAL again.

bajtos · 2018-11-19T14:38:40Z

packages/rest/src/rest.application.ts

-    this.restServer.bodyParser(bodyParserClass);
+  bodyParser(
+    bodyParserClass: Constructor<BodyParser>,
+    address?: BindingAddress<BodyParser>,


Can we use key instead of address for consistency with other places please? Most notably Context class uses key in most of places.

I just checked. The Context class BindingAddress for most cases.

If you refer to app.controller(controllerClass, name) or similar methods, I intentionally make it different so that we can use the well-know binding address to override built-in body parsers.

BTW, export const REQUEST_BODY_PARSER_JSON in keys.ts is by design to support the acceptance test of overriding built-in body parsers.

I agree we should use BindingAddress<BodyParser> type, but I am proposing to call the argument key, not address.

I was referring to Context APIs like ctx.bind, ctx.contains, ctx.unbind and so on.

https://github.com/strongloop/loopback-next/blob/61045e7540ff2f8c41db5991e91f8c0ec77555f5/packages/context/src/context.ts#L102
https://github.com/strongloop/loopback-next/blob/61045e7540ff2f8c41db5991e91f8c0ec77555f5/packages/context/src/context.ts#L88
https://github.com/strongloop/loopback-next/blob/61045e7540ff2f8c41db5991e91f8c0ec77555f5/packages/context/src/context.ts#L47-L49

packages/rest/test/acceptance/file-upload/file-upload-with-parser.acceptance.ts

packages/rest/test/acceptance/request-parsing/request-parsing.acceptance.ts

packages/rest/test/integration/http-handler.integration.ts

packages/rest/src/body-parsers/body-parser.text.ts

bajtos

Looks better now.

Please take a look at the older comments, there are many of them that are not resolved yet. I am open to discussion on which of the comments must be made before landing and which should be left out of scope, the ball is on your side.

Also: Documentation wise, I am (still) missing a guide for extension developers showing them how to write an extension contributing custom parsers.

raymondfeng · 2018-11-19T18:02:00Z

I was using text/json as an example use case (test case), where the user wants to register a specialized parser for a media type that's already covered by a built-in parser.
I see that the current implementation sorts the parsers and invokes built-in parsers at the end, so I suppose this is solved (although I didn't check if there is an accompanying test).

Yes. It's fixed.

text/json is now parsed by json paraser. See https://github.com/strongloop/loopback-next/blob/switch-to-body-parser/packages/rest/test/unit/body-parser.unit.ts#L91
See https://github.com/strongloop/loopback-next/blob/switch-to-body-parser/packages/rest/test/acceptance/request-parsing/request-parsing.acceptance.ts

raymondfeng · 2018-11-19T18:03:41Z

Also: Documentation wise, I am (still) missing a guide for extension developers showing them how to write an extension contributing custom parsers.

Do you think we need to refactor this section to a separate guide? https://github.com/strongloop/loopback-next/blob/switch-to-body-parser/docs/site/Parsing-requests.md#adding-parsers

raymondfeng · 2018-11-19T18:17:38Z

@bajtos I believe that I have addressed most of your comments. PTAL. If you think there are more blocking issues, let me know.

raymondfeng · 2018-11-21T16:51:14Z

@bajtos ping.

bajtos · 2018-11-22T11:19:57Z

Also: Documentation wise, I am (still) missing a guide for extension developers showing them how to write an extension contributing custom parsers.

Do you think we need to refactor this section to a separate guide? https://github.com/strongloop/loopback-next/blob/switch-to-body-parser/docs/site/Parsing-requests.md#adding-parsers

Eventually, I think we will need to split "Parsing requests" into multiple pages, because it's growing too long. BUT: As far as this PR is concerned, I am fine with keeping everything in a single page.

When I put myself in the shoes of an extension developer, I see the following shortcomings:

Discoverability. When I go to docs > LoopBack 4 > Extending LoopBack 4, I don't see any mention of how to write an extension contributing custom body parsers.
The content in "Parsing-requests" seems to be geared towards app developers. I does not show how an extension can export body parsers and have them picked up by the target app. Also how to allow apps to set the configuration options for the custom parser.

Here is what I have in mind:

Add a new page, e.g. "Creating custom body parsers", nest it inside "Extending LoopBack 4" section in the sidebar, alongside "Creating Decorators" and other guides for extension developers.
In the new page:
1. Tell people to read "Parsing-requests" to learn how to write a class implementing a body parser. (Keep it simple, no need to repeat what's written elsewhere. The important part is to make it easier for people to discover the relevant content.)
2. Explain how to wire up the extension (a Component class) so that the custom parser is picked up by target applications. Include a code snippet showing how to use bodyParserBindingKey helper to create the binding.
3. Explain how to allow applications to configure the custom body parser (e.g. set the limit, etc.). Include a code snippet with an example implementation. (Do you expect extensions to leverage getParserOptions helper?)

bajtos · 2018-11-22T11:32:12Z

@raymondfeng Here is the list of comments where I feel we did not reach consensus yet and where it would be difficult to change the code/APIs later:

The built-in hard-coded list of parser instances bypassing Dependency Injection. https://github.com/strongloop/loopback-next/pull/1936/files#r233875934
Exporting binding keys of individual parsers. https://github.com/strongloop/loopback-next/pull/1936/files#r233875934
The type used by custom extensions in the body parser options. https://github.com/strongloop/loopback-next/pull/1936/files#r233878521
A possibly breaking change in the way how empty bodies are handled. https://github.com/strongloop/loopback-next/pull/1936/files#r232701765
A test where it's difficult to spot what's being tested. https://github.com/strongloop/loopback-next/pull/1936/files#r233884268

raymondfeng · 2018-11-22T21:15:14Z

@bajtos I have addressed all of your pending comments. PTAL.

bajtos

LGTM 👍

The new doc page "Extending-request-body-parsing" is missing instructions on how a component can contribute a new/custom parser binding.

Explain how to wire up the extension (a Component class) so that the custom parser is picked up by target applications. Include a code snippet showing how to use bodyParserBindingKey helper to create the binding.

- add `bodyParser` sugar method - use `x-parser` to control custom body parsing - add docs for body parser extensions - add raw body parser

raymondfeng · 2018-11-26T15:01:13Z

The new doc page "Extending-request-body-parsing" is missing instructions on how a component can contribute a new/custom parser binding.

Added.

bajtos · 2018-11-26T15:23:30Z

Yay 🕺 🎉

raymondfeng requested a review from bajtos as a code owner October 29, 2018 16:28

raymondfeng changed the title ~~Switch to body parser~~ feat(rest): switch to body parser and add extensibility for parsers Oct 29, 2018

raymondfeng force-pushed the switch-to-body-parser branch 3 times, most recently from 404bdcc to 8625673 Compare November 1, 2018 18:35

raymondfeng force-pushed the switch-to-body-parser branch from 8625673 to 8c147af Compare November 5, 2018 16:02

raymondfeng force-pushed the switch-to-body-parser branch from 5c2051d to 98f6caf Compare November 6, 2018 00:38

bajtos suggested changes Nov 12, 2018

View reviewed changes

raymondfeng force-pushed the switch-to-body-parser branch 4 times, most recently from c38156f to af27150 Compare November 12, 2018 23:02

bajtos suggested changes Nov 13, 2018

View reviewed changes

raymondfeng force-pushed the switch-to-body-parser branch from af27150 to 4bfc7ab Compare November 13, 2018 21:47

raymondfeng force-pushed the switch-to-body-parser branch from 1030d7d to 9533889 Compare November 13, 2018 22:40

raymondfeng force-pushed the switch-to-body-parser branch from 096cad4 to 087e157 Compare November 14, 2018 17:15

raymondfeng force-pushed the switch-to-body-parser branch from 60b7585 to 03b9be4 Compare November 14, 2018 18:52

bajtos reviewed Nov 15, 2018

View reviewed changes

raymondfeng force-pushed the switch-to-body-parser branch 2 times, most recently from 8f444db to 922fc77 Compare November 16, 2018 18:33

bajtos reviewed Nov 19, 2018

View reviewed changes

raymondfeng force-pushed the switch-to-body-parser branch from 922fc77 to 4c2e033 Compare November 19, 2018 17:58

raymondfeng force-pushed the switch-to-body-parser branch from 4c2e033 to e125072 Compare November 19, 2018 18:16

bajtos mentioned this pull request Nov 22, 2018

File upload with multipart/form-data #1873

Closed

4 tasks

raymondfeng force-pushed the switch-to-body-parser branch 2 times, most recently from d8ebc98 to 2eeee63 Compare November 22, 2018 18:57

raymondfeng force-pushed the switch-to-body-parser branch from eaf80f5 to 3078aea Compare November 22, 2018 21:15

bajtos approved these changes Nov 26, 2018

View reviewed changes

raymondfeng added 2 commits November 26, 2018 06:48

feat(rest): switch to express body-parser

454c28e

feat(rest): allow body parsers to be extended

bf12cae

- add `bodyParser` sugar method - use `x-parser` to control custom body parsing - add docs for body parser extensions - add raw body parser

raymondfeng force-pushed the switch-to-body-parser branch from 3078aea to bf12cae Compare November 26, 2018 15:00

raymondfeng merged commit 86bfcbc into master Nov 26, 2018

raymondfeng deleted the switch-to-body-parser branch November 26, 2018 15:19

bajtos mentioned this pull request Nov 27, 2018

Support multipart form data #1989

Closed

4 tasks

dhmlau mentioned this pull request Nov 27, 2018

[Blog] Enabling users to support other content types #2083

Closed

dhmlau mentioned this pull request Dec 18, 2018

Could you make an example-4.0 for Loopback 4? strongloop/loopback-example-storage#25

Closed

feat(rest): switch to body parser and add extensibility for parsers #1936

feat(rest): switch to body parser and add extensibility for parsers #1936

Conversation

raymondfeng commented Oct 29, 2018 • edited Loading

Checklist

bajtos commented Nov 5, 2018

raymondfeng commented Nov 5, 2018

raymondfeng commented Nov 8, 2018

bajtos commented Nov 9, 2018

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng commented Nov 13, 2018

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Nov 13, 2018

raymondfeng commented Nov 13, 2018

bajtos commented Nov 14, 2018

bajtos commented Nov 14, 2018

raymondfeng commented Nov 14, 2018

bajtos left a comment

Choose a reason for hiding this comment

bajtos commented Nov 15, 2018

bajtos commented Nov 15, 2018 • edited Loading

raymondfeng commented Nov 16, 2018

raymondfeng commented Nov 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

raymondfeng commented Nov 19, 2018

raymondfeng commented Nov 19, 2018

raymondfeng commented Nov 19, 2018

raymondfeng commented Nov 21, 2018

bajtos commented Nov 22, 2018

bajtos commented Nov 22, 2018

raymondfeng commented Nov 22, 2018

bajtos left a comment

Choose a reason for hiding this comment

raymondfeng commented Nov 26, 2018

bajtos commented Nov 26, 2018

raymondfeng commented Oct 29, 2018 •

edited

Loading

bajtos commented Nov 15, 2018 •

edited

Loading