feat(rest): add support for form request body #1838

raymondfeng · 2018-10-11T04:56:18Z

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

packages/rest/src/parser.ts

bajtos · 2018-10-11T08:20:21Z

Correctly supporting application/x-www-form-urlencoded is more difficult than it seams, because this encoding does not preserve any type information. We have been bitten by this in strong-remoting, plus changes made in the coercion algorithm later are usually a breaking change requiring a semver-major version.

Let's make sure we have a good understanding of how different values are treated by our body parser and that we are ok with any limitations discovered.

For example, let's consider the following schema - I am typing it from top of my head, I hope I won't make any mistakes.

const schema ={
  type: 'object',
  properties: {
    id: {type: 'number'},
    created: {type: 'string', format: 'date-time'},
    tagIds: {type: 'array', items: {type: 'number'}}
  }
};

const spec = givenOperationWithRequestBody({
  description: 'data',
  content: {
    'application/x-www-form-urlencoded': {schema}
  },
});

Things to consider:

will the value provided by our parser pass AJV validation?
the type of data.id property - a number or a string?
the type of data.created property - a string or a Date instance?
the type of items in data.tagIds - strings or numbers?

Please add more tests to demonstrate how our REST layer is going to treat these scenarios.

bajtos

PTAL at my comment above, plus there is a missing test to add.

bajtos · 2018-10-11T08:26:35Z

packages/rest/src/parser.ts

+  ) {
+    return await parseFormBody(request).catch((err: HttpError) => {
+      debug('Cannot parse request body %j', err);
+      err.statusCode = 400;


According to code coverage report, this branch is not covered. Please add a test to verify how malformed form bodies are handled, we already have a similar test for JSON bodies.

bajtos · 2018-10-11T08:32:03Z

FWIW, I wrote pretty extensive (possibly too extensive) test suite for strong-remotin 3.0, I think many of the test cases are relevant for loopback-next too and we should incorporate them into our test suite too.

JesusTheHun · 2018-10-11T09:26:10Z

If you allow me to submit some input here.
The minimum feature is to handle submitted data as strings. Every web developer knows form data means everything is a string.

Now, what do I expect from a framework like loopback, is to allow me to transform my data into the type I annotated.
The cast should have a default behavior, ( i.e. casting into an integer using parseInt), but also allow me to specify the data transformer I want to use. This allow very interesting handling, such as transforming into a BigNumber object, transforming a "custom" date format into a date object, transforming a string containing an id into a model instance.
One should be able to register data transformer and use them as a definition of the type they expect. If the transformer fails, the entire call fails with a response defined by the transformer.

This kind of discussion may belong somewhere else ; I would be happy to discuss this matter with you.

bajtos · 2018-10-11T11:23:51Z

@JesusTheHun thank you for chiming in, this is exactly the kind of feedback we need from our users 👍

The minimum feature is to handle submitted data as strings. Every web developer knows form data means everything is a string.

Makes sense.

Now, what do I expect from a framework like loopback, is to allow me to transform my data into the type I annotated.

That would be my expectation to!

Few discussion points:

If we implement the minimal part only - converting form data into strings values (no type coercion into integers, etc.), would it be good enough for the first release of this feature?
If we add type coercion as the next step later in the future, would you consider such change as backwards compatible, despite the fact that your controller methods will start receiving different values for the same requests?

The cast should have a default behavior, ( i.e. casting into an integer using parseInt), but also allow me to specify the data transformer I want to use. This allow very interesting handling, such as transforming into a BigNumber object, transforming a "custom" date format into a date object, transforming a string containing an id into a model instance.
One should be able to register data transformer and use them as a definition of the type they expect. If the transformer fails, the entire call fails with a response defined by the transformer.

Agreed, we were thinking about this sort of extensibility ourselves too. In my opinion, custom data transformers should be left out of the initial implementation and explored as the next step after the basic support for form-data requests is done.

JesusTheHun · 2018-10-11T11:51:57Z

Yes, definitly.
It should not be a change, but a new feature.

Feature v1 :

@formData()
myMethod(formData: object)

// or

@formData({
    properties: ['date', 'username']
    // a shorthand for
    properties: [
        {fieldName: 'date', name: 'date'},
        {fieldName: 'username', name: 'username'}
    ]
})
myMethod(date: string, username: string)

Feature v2 :

// The key 'user' is registred in the application, and binded to the data transformer
// that returns an instance of MyUserModel.
@formData({
    properties: [
        'foo', // shorthand still works
        {fieldName: 'date', type: 'date'},
        {fieldName: 'username', name: 'user', type: 'user'}]
})
myMethod(date: Date, user: MyUserModel)

Does that make sense ?

raymondfeng · 2018-10-15T14:29:34Z

@bajtos PTAL

bajtos

The implementation looks reasonable now.

Please add tests to cover the error paths per my comments below.

bajtos · 2018-10-16T12:00:08Z

packages/rest/src/parser.ts

+  ) {
+    const body = await parseFormBody(request).catch((err: HttpError) => {
+      debug('Cannot parse request body %j', err);
+      err.statusCode = 400;


This branch is still not covered by any test, see https://coveralls.io/builds/19498795/source?filename=packages%2Frest%2Fsrc%2Fparser.ts

Please add a new test. AFAICT, body/form returns error only when the request body is larger than the configured limit (1MB by default) or when an unsupported content-encoding is used. I guess our best option is to explicitly set Content-Length header of the outgoing request to a value larger than 1MB.

Relevant source code:

https://github.com/Raynos/body/blob/master/index.js

https://github.com/stream-utils/raw-body/blob/master/index.js

bajtos · 2018-10-16T12:01:27Z

packages/rest/src/parser.ts

+      coercionRequired: true,
+    };
+  }
+
  if (contentType && !/json/.test(contentType)) {
    throw new HttpErrors.UnsupportedMediaType(


This branch is not covered by any test, see https://coveralls.io/builds/19498795/source?filename=packages%2Frest%2Fsrc%2Fparser.ts

Please add a new test, for example sending a text/plain payload (request body).

raymondfeng · 2018-10-16T16:14:37Z

@bajtos More tests are added to cover the error cases.

There are a few things that we need to fix as follow-ups:

Why do we use body instead of body-parser?
We should allow text and stream formats too
We need to make it configurable for limit. At the moment, the body size cannot go beyond 1MB.

bajtos · 2018-10-18T07:49:13Z

@raymondfeng Thank you for addressing my comments.

I really dislike pull requests combining multiple changes that could have been done in standalone pull requests. Such large pull requests are difficult to review and take long to land. It would be much better if this PR stayed focused on supporting application/x-www-form-urlencoded.

Why do we use body instead of body-parser?

body-parser is a middleware that does not provide promise API.

Please note that we are parsing the request body only if the operation is accepting a parameter parsed from the request body, therefore we cannot simply mount body-parser on our internal Express app.

Also at the time we implemented minimal body parsing, we were not using Express yet (IIRC).

I believe that both body-parser and body share a lot of the implementation under the hood, e.g. raw-body, thus I think there are not many (if any) benefits from migrating to body-parser.

We should allow text and stream formats too

+1

That's out of scope of this pull request IMO.

We need to make it configurable for limit. At the moment, the body size cannot go beyond 1MB.

Agreed. Again, this is best left for a follow-up pull request.

bajtos

Besides the comments below, I'd like to see tests verifying what happens when the content is text or html and the method expects an object. This can happen for example when a client sends text or html payload to CRUD api like POST /products (create a new product).

It makes me wonder how OpenAPI spec is dealing with this issue. AFAICT, Request Body Object typically enumerates content types supported by the endpoint.

IMO, LB4 should leverage the information provided in OpenAPI spec and reject requests with content-type not allowed by the spec early on. There is no need to parse the request body (e.g. text or html) when the endpoint is not prepared to accept it.

bajtos · 2018-10-18T07:40:24Z

packages/rest/src/parser.ts

+/**
+ * Express body parser function type
+ */
+type BodyParser = (


The file packages/rest/src/parser.ts is becoming too large to my taste. Since you are already moving code around in this pull request, please move all body-parsing code into a new file, e.g. packages/rest/src/body-parser.ts.

bajtos · 2018-10-18T07:51:26Z

packages/rest/src/parser.ts

+ * @param request Http request
+ */
+function parse(handle: BodyParser, request: Request): Promise<void> {
+  // A hack to fool TypeScript as we don't need `response`


// A hack to fool TypeScript as we don't need response

👎

Let's keep using body module as it does not require us to do such hacks. body also provides Promise API out of the box.

bajtos · 2018-10-18T07:52:53Z

packages/rest/src/parser.ts

+        reject(err);
+        return;
+      }
+      resolve();


In case we decide to keep this code around: let's resolve the promise with request.body so that consumers of parse don't need to understand Express-specific concepts like storing body on the request object.

bajtos · 2018-10-18T07:54:13Z

packages/rest/src/parser.ts

+  body = await parseUrlencodedBody(request, options);
+  if (body) return body;
+  body = await parseTextBody(request, options);
+  if (body) return body;


This looks rather non-optimal to me. Why are we invoking so many body parsers for each request, when the Content-Type header should contain enough information to decide which parser to invoke?

bajtos · 2018-10-18T07:57:53Z

packages/rest/src/parser.ts

+    options,
+  );
+  if (typeis(request, jsonOptions.type)) {
+    await parse(json(jsonOptions), request);


IIUC, this is creating a new json parser function/middleware for each incoming request. That looks like a preliminary performance pessimization to me - not only we are creating extra closures, but we also prevent the parser from applying memoization/caching and other techniques to improve performance. (This is probably not relevant to JSON, but it may be important for other parsers.)

It would be great if we could reuse parser functions (middleware handlers created by body-parser factories) across the requests.

bajtos · 2018-10-18T08:01:23Z

packages/rest/src/types.ts

+/**
+ * Options for request body parsing
+ */
+export type RequestBodyParserOptions = OptionsJson &


Is it a good idea to mix all content-type-specific options at the top level?

What if a user wants to configure different limits for JSON and Text payloads? Is it a valid requirement to support?

bajtos · 2018-10-18T08:03:23Z

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

+        .catch(err => {
+          // The server side can close the socket before the client
+          // side can send out all data
+          if (err && err.code !== 'EPIPE') throw err;


Please extract this code into a shared helper function.

Example usage:

.catch(rethrowUnlessPipeError)

bajtos · 2018-10-18T10:42:12Z

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

+        });
+    });
+
+    it('rejects over-limit request form body', () => {


Uh, I did not anticipate how complex this test will become when I asked to add it. Considering that we didn't have this kind of tests before this pull request (in the current master), I am ok to leave them out of this PR.

In case you prefer to keep these two new tests, then please:

make it more clear that EPIPE is one of the expected outcomes of the test

add a check to verify that the controller method/route handler was not invoked

raymondfeng · 2018-10-18T17:36:51Z

@bajtos I move the switch of body-parser to a separate PR.

raymondfeng · 2018-10-19T16:23:32Z

@bajtos PTAL

bajtos

The code changes looks much better now! I have few minor comments, PTAL.

Besides the code changes, these two new features (form-urlencoded body, customizable parser config) really need documentation. Please add the docs as part of this pull request.

As a developer building LB4 app accepting large data (>1MB), I want to learn how to increase the maximum request body size accepted by the framework.
As a developer building a SPA using HTML forms to submit data to my API server, I want to understand how can I write a controller method accepting data in application/x-www-form-urlencoded format.

bajtos · 2018-10-22T11:58:22Z

packages/rest/src/parser.ts

+        throw err;
+      },
+    );
+    // form parser returns an object with prototype


This comment looks misplaced to me. IIUC, it's referring to Object.assign call below, could you please move it one line down?

return { // form parser returns an object with prototype value: Object.assign({}, body), coercionRequired: true, };

bajtos · 2018-10-22T12:00:39Z

packages/rest/src/parser.ts

+    contentType &&
+    contentType.startsWith('application/x-www-form-urlencoded')
+  ) {
+    const body = await parseFormBody(request, options).catch(


I find it rather weird that you are mixin await and .catch() style. Can you rewrite this using try/catch please?

try { const body = await parseFormBody(request, options) return { /* ... */ }; } catch (err) { // handle the error, use a helper function shared by JSON & form parsers throw normalizeBodyParserError(err); }

bajtos · 2018-10-22T12:01:02Z

packages/rest/src/parser.ts

-    err.statusCode = 400;
-    throw err;
-  });
+  const jsonBody = await parseJsonBody(request, options).catch(


Ditto, please use try/catch instead of .catch().

packages/rest/src/parser.ts

bajtos · 2018-10-22T12:05:36Z

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

+            statusCode: 413,
+          },
+        })
+        .catch(catchEpipeError)


When I read this line, I have no idea what is the intent. What happens when EPIPE error is caught? What happens when a different error was thrown?

Also I believe E in EPIPE is an abbreviation for Error (EPIPE = error pipe), thus EpipeError feels rather redundant to me ("error pipe error").

I am proposing to rename catchEpipeError to ignorePipeError.

bajtos · 2018-10-22T12:17:24Z

Besides the comments posted above (I hope they were persisted by GitHub despite the current outage), I'd like to discuss the following point I raised above:

IMO, LB4 should leverage the information provided in OpenAPI spec and reject requests with content-type not allowed by the spec early on. There is no need to parse the request body (e.g. text or html) when the endpoint is not prepared to accept it.

IIRC, our @requestBody decorator describes the OpenAPI spec operation as accepting application/json only by default, see here:
https://github.com/strongloop/loopback-next/blob/bf329719db08cacc011aaaf88bf29aad6bed391d/packages/openapi-v3/src/decorators/request-body.decorator.ts#L90

If we are accepting application/x-www-form-urlencoded now too, then the decorator should be changed to correctly announce that fact in the OpenAPI spec we are emitting. I think we should also look into ways how to allow app developers to control which content-types they are willing to accept, but that's out of scope of this pull request - let's open a new issue to discuss the options.

In the documentation for the new features, please include a short section describing the problem of coercion from urlencoded format that supports string type only into JavaScript types like number and boolean, explain that we are using AJV coercion under the hood and point readers to the relevant section of AJV docs. ideally point readers to AJV docume

raymondfeng · 2018-10-22T18:02:30Z

@bajtos I believe that I have addressed your comments and added more information to the docs. PTAL.

bajtos

You are making great progress here!

Please add a test to show how request bodies with complex properties are handled when the body uses URL encoding (not JSON). For example, a Building model has an location property that's a GeoPoint with two number properties: lat and lng.

An example payload in JSON:

{
  "name": "IBM HQ",
  "location": {
    "lat": 40.741895,
    "long": -73.989308,
  }
}

How are we going to treat the following url-encoded payload? Will we also coerce lat/lng from strings to numbers?

name=IBM%20HQ&location[lat]=0.741895&location[lng]=-73.989308

To be clear, I am not asking you to implement support for nested objects as part of this pull request, just write a test documenting what happens when the request body contains nested properties.

It would be great to have a test for nested arrays too, e.g. a Product can have a list of tagIds - which of the following alternatives work?

name=Pen&tagsId[0]=10&tagIds[1]=20
name=Pen&tagsId[]=10&tagIds[]=20
name=Pen&tagsId=10&tagIds=20

I ran our Todo example application to verify the OpenAPI spec rendered for LB4 applications and the description of request bodies does not look correct to me.

openapi: 3.0.0
# [skipped info object]
paths:
  /todos:
    post:
      # [skipped tags, responses, x-controller/operation-name]
      requestBody:
        content:
          application/json:
            schema:
              $ref: '#/components/schemas/Todo'

The request body content Notice that requestBody > content describes only application/json type and does not allow urlencoded.

As I mentioned earlier (#1838 (comment)), I think we should emit entries for all supported content types by default. I am open to alternative solutions - let's dicuss!

IIRC, our @requestBody decorator describes the OpenAPI spec operation as accepting application/json only by default, see here:

https://github.com/strongloop/loopback-next/blob/bf329719db08cacc011aaaf88bf29aad6bed391d/packages/openapi-v3/src/decorators/request-body.decorator.ts#L90

If we are accepting application/x-www-form-urlencoded now too, then the decorator should be changed to correctly announce that fact in the OpenAPI spec we are emitting.
I think we should also look into ways how to allow app developers to control which content-types they are willing to accept, but that's out of scope of this pull request - let's open a new issue to discuss the options.

bajtos · 2018-10-25T07:25:00Z

docs/site/Parsing-requests.md

+Please note that `urlencoded` media type does not support data typing. For
+example, `key=3` is parsed as `{key: '3'}`. The raw result is then coerced by
+AJV based on the matching content schema. See the rules at
+https://github.com/epoberezkin/ajv/blob/master/COERCION.md.


Let's change this naked URL into a nice hyperlink please. For example:

The coercion rules are described in [AJV type coercion rules](https://github.com/epoberezkin/ajv/blob/master/COERCION.md)

bajtos · 2018-10-25T07:28:56Z

docs/site/Parsing-requests.md

+```
+
+By default, the `limit` is `1MB`. Any request with a body length exceeding the
+limit will be rejected with http status code 413 (request entity too large).


Where can readers find a list of all available options? Can we describe them here? Each item should describe: option name, value type (string, number, boolean, etc.), default value (if any), description.

At minimum, please add a hyperlink pointing users to an existing documentation living elsewhere (e.g. apidocs for RequestBodyParserOptions or the docs for the body module).

packages/rest/src/parser.ts

packages/rest/src/types.ts

bajtos · 2018-10-25T07:42:34Z

packages/rest/src/validation/request-body.validator.ts

@@ -46,11 +52,11 @@ export function validateRequestBody(
    throw err;
  }

-  const schema = getRequestBodySchema(requestBodySpec);
+  const schema = options.schema || getRequestBodySchema(requestBodySpec);


What is the purpose of keeping the old flavor where the schema is extracted from requestBodySpec without taking into account the content type? Can we remove this flavor it please and always require callers of validateRequestBody to provide the schema object.

Alternatively, and I think may be a better design, modify validateRequestBody to accept full RequestBody interface with all metadata (instead of accepting the body payload only).

export function validateRequestBody( body: RequestBody, spec: RequestBodyObject | undefined, globalSchemas?: SchemasObject, ) { // ... const schema = body.schema; // ... }

bajtos · 2018-10-25T07:50:41Z

packages/rest/src/parser.ts

@@ -129,9 +208,12 @@ function buildOperationArguments(
  }

  debug('Validating request body - value %j', body);
-  validateRequestBody(body, operationSpec.requestBody, globalSchemas);
+  validateRequestBody(body.value, operationSpec.requestBody, globalSchemas, {


As I proposed elsewhere, let's change validateRequestBody to accept the full RequestBody object, it will greatly simplify this line.

validateRequestBody(body, operationSpec.requestBody, globalSchemas);

The function buildOperationArguments is already complex enough and doing a lot, let's try to push additional functionality down to helper functions like validateRequestBody.

bajtos · 2018-10-25T07:52:21Z

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

+        });
+    });
+
+    let bodyParamControllerInvoked = false;


Please reset this value back to false in a beforeEach hook, otherwise the first test touching this variable can trigger failure of other tests sharing it.

bajtos · 2018-10-25T07:53:53Z

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

+      // The server side can close the socket before the client
+      // side can send out all data. For example, `response.end()`
+      // is called before all request data has been processed due
+      // to size limit


Let's add a link to the relevant Node.js issue please.

bajtos · 2018-10-25T07:55:23Z

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

    function givenBodyParamController() {
+      bodyParamControllerInvoked = false;


I find this rather hacky, let's move the reset to a before-each hook please.

Have you considered using Sinon stubs instead? https://sinonjs.org/releases/v7.0.0/stubs/

raymondfeng · 2018-10-25T20:23:08Z

The request body content Notice that requestBody > content describes only application/json type and does not allow urlencoded.

We're going to have long list of media types supported with extensions. Defaulting to only application/json makes sense to me. The requestBody content can always be customized via @requestBody.

raymondfeng · 2018-10-25T20:23:26Z

@bajtos I have addressed your comments. PTAL.

bajtos · 2018-10-26T12:25:06Z

docs/site/Parsing-requests.md

+```
+
+The encoded value
+`'name=IBM%20HQ&location[lat]=0.741895&location[lng]=-73.989308&tags[0]=IT&tags[1]=NY'`


Great example 👍

bajtos · 2018-10-26T12:27:41Z

packages/rest/src/parser.ts

@@ -129,9 +223,11 @@ function buildOperationArguments(
  }

  debug('Validating request body - value %j', body);
-  validateRequestBody(body, operationSpec.requestBody, globalSchemas);
+  validateRequestBody(body, operationSpec.requestBody, globalSchemas, {
+    coerceTypes: body.coercionRequired,


Why are we passing coerceTypes in options, when validateRequestBody can infer the value from body.coercionRequired?

bajtos · 2018-10-26T12:28:23Z

packages/rest/src/types.ts

+export type RequestBodyParserOptions = {
+  /**
+   * The limit of request body size. By default it is 1MB (1024 * 1024). If a
+   * stream contains more then 1MB, it returns an error. This prevents someone


typo: more then. should be more than.

bajtos · 2018-10-26T12:30:21Z

packages/openapi-v3/src/decorators/request-body.decorator.ts

@@ -81,7 +81,8 @@ export function requestBody(requestBodySpec?: Partial<RequestBodyObject>) {
  return function(target: Object, member: string, index: number) {
    debug('@requestBody() on %s.%s', target.constructor.name, member);
    debug('  parameter index: %s', index);
-    debug('  options: %s', inspect(requestBodySpec, {depth: null}));
+    if (debug.enabled)


Can you add a comment please to tell our code coverage tool to ignore the fact that this block is intentionally skipped by tests and thus should be not be counted towards code coverage?

bajtos · 2018-10-26T12:30:26Z

packages/openapi-v3/src/decorators/request-body.decorator.ts

@@ -95,7 +96,8 @@ export function requestBody(requestBodySpec?: Partial<RequestBodyObject>) {

    const paramType = paramTypes[index];
    const schema = resolveSchema(paramType);
-    debug('  inferred schema: %s', inspect(schema, {depth: null}));
+    if (debug.enabled)


Can you add a comment please to tell our code coverage tool to ignore the fact that this block is intentionally skipped by tests and thus should be not be counted towards code coverage?

bajtos · 2018-10-26T12:30:32Z

packages/openapi-v3/src/decorators/request-body.decorator.ts

@@ -109,7 +111,8 @@ export function requestBody(requestBodySpec?: Partial<RequestBodyObject>) {
      requestBodySpec[REQUEST_BODY_INDEX] = index;
    }

-    debug('  final spec: ', inspect(requestBodySpec, {depth: null}));
+    if (debug.enabled)


Can you add a comment please to tell our code coverage tool to ignore the fact that this block is intentionally skipped by tests and thus should be not be counted towards code coverage?

bajtos · 2018-10-26T12:30:41Z

packages/rest/src/validation/request-body.validator.ts

-  const schema = getRequestBodySchema(requestBodySpec);
-  debug('Request body schema: %j', util.inspect(schema, {depth: null}));
+  const schema = body.schema;
+  if (debug.enabled) {


Can you add a comment please to tell our code coverage tool to ignore the fact that this block is intentionally skipped by tests and thus should be not be counted towards code coverage?

bajtos · 2018-10-26T12:30:52Z

packages/rest/src/validation/request-body.validator.ts

-    'Converted OpenAPI schema to JSON schema: %s',
-    util.inspect(jsonSchema, {depth: null}),
-  );
+  if (debug.enabled) {


Can you add a comment please to tell our code coverage tool to ignore the fact that this block is intentionally skipped by tests and thus should be not be counted towards code coverage?

bajtos · 2018-10-26T12:31:12Z

packages/rest/src/validation/request-body.validator.ts

@@ -111,7 +105,9 @@ function validateValueAgainstSchema(

  const validationErrors = validate.errors;

-  debug('Invalid request body: %s', util.inspect(validationErrors));
+  if (debug.enabled) {


ditto, let's disable code coverage for this block please

bajtos · 2018-10-26T12:37:10Z

The request body content Notice that requestBody > content describes only application/json type and does not allow urlencoded.

We're going to have long list of media types supported with extensions. Defaulting to only application/json makes sense to me. The requestBody content can always be customized via @requestBody.

I am not sure I fully understand what you are proposing.

Here is my opinion:

Any endpoint should be accepting only content types described in its OpenAPI spec.
If the spec says we accept JSON only, then urlencoded bodies should be rejected.
I don't have a strong opinion on whether urlencoded should be enabled by default or not.

Does this match your vision too?

If yes, then I would like this pull request to include the following tests (some of them may already exist, IDK):

Verify that when the request uses a content-type not described in request-body spec, then the request is rejected.
Verify what content types are described by @requestBody decorator in the spec it emits.

If no, then let's discuss more.

raymondfeng · 2018-10-26T14:29:58Z

@bajtos We are on the same page.

Verify that when the request uses a content-type not described in request-body spec, then the request is rejected.

We have a test to cover that. See https://github.com/strongloop/loopback-next/blob/acfb1ac77a1a88e4dfcbee6e8e75cf0cb7042768/packages/rest/test/integration/http-handler.integration.ts#L271

Verify what content types are described by @requestBody decorator in the spec it emits.

Added. See https://github.com/strongloop/loopback-next/blob/1d437895341f5c5283cbbdea2671fd3b760f9e4f/packages/rest/test/unit/rest.server/rest.server.open-api-spec.unit.ts#L197

PTAL.

jannyHou

@raymondfeng Mostly LGTM 👍 a few nitpicks and one question regarding the referenced schema, see comment , please note AJV supports resolving a reference.

jannyHou · 2018-10-26T20:40:23Z

packages/rest/test/unit/rest.server/rest.server.open-api-spec.unit.ts

@@ -194,6 +194,45 @@ describe('RestServer.getApiSpec()', () => {
    });
  });

+  it('emits all media types for request body', () => {
+    const opSepc = {


nitpick: we can leverage OperationSpecBuild to build the spec.

jannyHou · 2018-10-26T21:06:55Z

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

@@ -58,6 +59,223 @@ describe('operationArgsParser', () => {
    expect(args).to.eql([{key: 'value'}]);
  });

+  it('parses body parameter for urlencoded', async () => {


Does this PR support a referenced schema?

I think we do.

jannyHou · 2018-10-26T22:12:50Z

packages/rest/src/validation/request-body.validator.ts

@@ -128,6 +128,7 @@ function validateValueAgainstSchema(
 function createValidator(
  schema: SchemaObject,
  globalSchemas?: SchemasObject,
+  options?: AJV.Options,


I think you created a type RequestBodyValidationOptions to be consistent.

jannyHou · 2018-10-26T22:14:00Z

packages/rest/src/validation/request-body.validator.ts

@@ -92,15 +88,16 @@ const compiledSchemaCache = new WeakMap();
 function validateValueAgainstSchema(
  // tslint:disable-next-line:no-any
  body: any,
-  schema: SchemaObject,
+  schema: SchemaObject | ReferenceObject,


Hmm, any reason remove the ReferenceObject here?

It's adding ReferenceObject back.

jannyHou · 2018-10-26T22:28:51Z

packages/rest/src/parser.ts

+    }
+  }
+
+  throw new HttpErrors.UnsupportedMediaType(


There is a file to organize the HttpErrors we defined: https://github.com/strongloop/loopback-next/blob/a478e567daaf732098ca2c1b9994a479146208fe/packages/rest/src/rest-http-error.ts
Defining errors there makes error messages re-usable, e.g. assert the error and message in test.

raymondfeng · 2018-10-26T23:22:07Z

@jannyHou Good catches. I fixed them.

bajtos

Looks mostly good. Please consider addressing the remaining comments above and below too (e.g. disabling code coverage for if (debug.enabled)).

bajtos · 2018-10-29T07:09:12Z

packages/rest/test/unit/rest.server/rest.server.open-api-spec.unit.ts

@@ -194,6 +194,44 @@ describe('RestServer.getApiSpec()', () => {
    });
  });

+  it('emits all media types for request body', () => {
+    const opSepc = anOperationSpec()


Typo: opSepc, should be opSpec. Since this is the spec we are expecting to be created, and not the spec used to define the API, please rename the variable to expectedOpSpec or expectedSpec to make that distinction clear.

See #1797 - allow applications to configure body parser options - match content type to request body spec - use RequestBody for validation - extract unsupported media type error

JesusTheHun · 2018-10-29T17:07:32Z

Congratulations gentlemen ! I'll get my hands on it as soon as the doc is online and give you a feedback

raymondfeng requested a review from bajtos as a code owner October 11, 2018 04:56

marioestradarosa reviewed Oct 11, 2018

View reviewed changes

packages/rest/src/parser.ts Outdated Show resolved Hide resolved

bajtos suggested changes Oct 11, 2018

View reviewed changes

raymondfeng force-pushed the support-form-body branch from 3cb4df3 to b7b9686 Compare October 12, 2018 21:41

dhmlau mentioned this pull request Oct 16, 2018

Missing doc about form-data parameters in controller #1797

Closed

bajtos suggested changes Oct 16, 2018

View reviewed changes

raymondfeng force-pushed the support-form-body branch from b7b9686 to 55cfec0 Compare October 16, 2018 16:12

raymondfeng mentioned this pull request Oct 17, 2018

fix the headersSent check and avoid Error: write EPIPE loopbackio/strong-error-handler#79

Closed

2 tasks

raymondfeng force-pushed the support-form-body branch 3 times, most recently from bb38d66 to 6d95802 Compare October 18, 2018 05:48

bajtos suggested changes Oct 18, 2018

View reviewed changes

bajtos reviewed Oct 18, 2018

View reviewed changes

bajtos mentioned this pull request Oct 18, 2018

File upload with multipart/form-data #1873

Closed

4 tasks

raymondfeng force-pushed the support-form-body branch from 6d95802 to ceff03f Compare October 18, 2018 17:36

raymondfeng force-pushed the support-form-body branch 2 times, most recently from 7f51f2d to ba7ffcc Compare October 18, 2018 17:47

bajtos mentioned this pull request Oct 19, 2018

Support multipart form data #1880

Closed

7 tasks

raymondfeng force-pushed the support-form-body branch from ba7ffcc to 3d9a005 Compare October 19, 2018 22:23

bajtos suggested changes Oct 22, 2018

View reviewed changes

raymondfeng force-pushed the support-form-body branch 2 times, most recently from d44f033 to 0764840 Compare October 22, 2018 17:43

bajtos suggested changes Oct 25, 2018

View reviewed changes

bajtos mentioned this pull request Oct 25, 2018

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

Closed

7 tasks

raymondfeng force-pushed the support-form-body branch 2 times, most recently from 928658b to eb3af8f Compare October 25, 2018 18:50

raymondfeng force-pushed the support-form-body branch from ecb2edb to acfb1ac Compare October 25, 2018 20:40

bajtos reviewed Oct 26, 2018

View reviewed changes

raymondfeng force-pushed the support-form-body branch from acfb1ac to 1d43789 Compare October 26, 2018 14:29

raymondfeng force-pushed the support-form-body branch 2 times, most recently from 0c0d7d3 to a478e56 Compare October 26, 2018 14:44

jannyHou reviewed Oct 26, 2018

View reviewed changes

raymondfeng force-pushed the support-form-body branch from a478e56 to ba40a29 Compare October 26, 2018 23:21

bajtos approved these changes Oct 29, 2018

View reviewed changes

feat(rest): add support for form request body

61834d8

See #1797 - allow applications to configure body parser options - match content type to request body spec - use RequestBody for validation - extract unsupported media type error

raymondfeng force-pushed the support-form-body branch from ba40a29 to 61834d8 Compare October 29, 2018 15:05

raymondfeng merged commit 2d9e0a8 into master Oct 29, 2018

bajtos deleted the support-form-body branch October 30, 2018 07:47

bajtos mentioned this pull request Oct 30, 2018

[Validation] Get the schema of matching content type for request body validation #1494

Closed

4 tasks

dhmlau mentioned this pull request Oct 31, 2018

Monthly Milestone - October 2018 🌦 #1761

Closed

17 tasks

		function givenBodyParamController() {
		bodyParamControllerInvoked = false;

feat(rest): add support for form request body #1838

feat(rest): add support for form request body #1838

Conversation

raymondfeng commented Oct 11, 2018 • edited Loading

Checklist

bajtos commented Oct 11, 2018

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Oct 11, 2018

JesusTheHun commented Oct 11, 2018

bajtos commented Oct 11, 2018

JesusTheHun commented Oct 11, 2018

raymondfeng commented Oct 15, 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

raymondfeng commented Oct 16, 2018

bajtos commented Oct 18, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng commented Oct 18, 2018

raymondfeng commented Oct 19, 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 Oct 22, 2018

raymondfeng commented Oct 22, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng commented Oct 25, 2018

raymondfeng commented Oct 25, 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

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 Oct 26, 2018 • edited Loading

raymondfeng commented Oct 26, 2018 • edited Loading

jannyHou 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng commented Oct 26, 2018

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JesusTheHun commented Oct 29, 2018

raymondfeng commented Oct 11, 2018 •

edited

Loading

bajtos commented Oct 26, 2018 •

edited

Loading

raymondfeng commented Oct 26, 2018 •

edited

Loading