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.

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

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

Merged
merged 1 commit into from Oct 29, 2018

Conversation

Projects
None yet
5 participants
@raymondfeng
Member

raymondfeng commented Oct 11, 2018

See #1797

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

@raymondfeng raymondfeng requested a review from bajtos as a code owner Oct 11, 2018

@bajtos

This comment has been minimized.

Member

bajtos commented Oct 11, 2018

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.

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

This comment has been minimized.

@bajtos

bajtos Oct 11, 2018

Member

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

This comment has been minimized.

Member

bajtos commented Oct 11, 2018

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

This comment has been minimized.

JesusTheHun commented Oct 11, 2018

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

This comment has been minimized.

Member

bajtos commented Oct 11, 2018

@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:

  1. 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?
  2. 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

This comment has been minimized.

JesusTheHun commented Oct 11, 2018

  1. Yes, definitly.

  2. 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 raymondfeng force-pushed the support-form-body branch from 3cb4df3 to b7b9686 Oct 12, 2018

@raymondfeng

This comment has been minimized.

Member

raymondfeng commented Oct 15, 2018

@bajtos PTAL

@bajtos

The implementation looks reasonable now.

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

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

This comment has been minimized.

@bajtos

bajtos Oct 16, 2018

Member

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:

coercionRequired: true,
};
}

if (contentType && !/json/.test(contentType)) {
throw new HttpErrors.UnsupportedMediaType(

This comment has been minimized.

@bajtos

bajtos Oct 16, 2018

Member

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 raymondfeng force-pushed the support-form-body branch from b7b9686 to 55cfec0 Oct 16, 2018

@raymondfeng

This comment has been minimized.

Member

raymondfeng commented Oct 16, 2018

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

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

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

@raymondfeng raymondfeng force-pushed the support-form-body branch 3 times, most recently from bb38d66 to 6d95802 Oct 17, 2018

@bajtos

This comment has been minimized.

Member

bajtos commented Oct 18, 2018

@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.

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

This comment has been minimized.

@bajtos

bajtos Oct 18, 2018

Member

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.

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

This comment has been minimized.

@bajtos

bajtos Oct 18, 2018

Member

// 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.

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

This comment has been minimized.

@bajtos

bajtos Oct 18, 2018

Member

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.

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

This comment has been minimized.

@bajtos

bajtos Oct 18, 2018

Member

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?

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

This comment has been minimized.

@bajtos

bajtos Oct 18, 2018

Member

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.

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

This comment has been minimized.

@bajtos

bajtos Oct 18, 2018

Member

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?

.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;

This comment has been minimized.

@bajtos

bajtos Oct 18, 2018

Member

Please extract this code into a shared helper function.

Example usage:

.catch(rethrowUnlessPipeError)
});
});

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

This comment has been minimized.

@bajtos

bajtos Oct 18, 2018

Member

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

@bajtos bajtos referenced this pull request Oct 18, 2018

Open

File upload with multipart/form-data #1873

1 of 4 tasks complete

@raymondfeng raymondfeng force-pushed the support-form-body branch from 6d95802 to ceff03f Oct 18, 2018

@raymondfeng

This comment has been minimized.

Member

raymondfeng commented Oct 18, 2018

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

@raymondfeng raymondfeng force-pushed the support-form-body branch 2 times, most recently from 7f51f2d to ba7ffcc Oct 18, 2018

@bajtos bajtos referenced this pull request Oct 19, 2018

Closed

Support multipart form data #1880

3 of 7 tasks complete
@raymondfeng

This comment has been minimized.

Member

raymondfeng commented Oct 19, 2018

@bajtos PTAL

@raymondfeng raymondfeng force-pushed the support-form-body branch from ba7ffcc to 3d9a005 Oct 19, 2018

@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.
throw err;
},
);
// form parser returns an object with prototype

This comment has been minimized.

@bajtos

bajtos Oct 22, 2018

Member

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,
     };
contentType &&
contentType.startsWith('application/x-www-form-urlencoded')
) {
const body = await parseFormBody(request, options).catch(

This comment has been minimized.

@bajtos

bajtos Oct 22, 2018

Member

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);
}
err.statusCode = 400;
throw err;
});
const jsonBody = await parseJsonBody(request, options).catch(

This comment has been minimized.

@bajtos

bajtos Oct 22, 2018

Member

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

Show resolved Hide resolved packages/rest/src/parser.ts
statusCode: 413,
},
})
.catch(catchEpipeError)

This comment has been minimized.

@bajtos

bajtos Oct 22, 2018

Member

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

This comment has been minimized.

Member

bajtos commented Oct 22, 2018

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:

requestBodySpec.content = {'application/json': {}};

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 raymondfeng force-pushed the support-form-body branch 2 times, most recently from d44f033 to 0764840 Oct 22, 2018

@raymondfeng

This comment has been minimized.

Member

raymondfeng commented Oct 22, 2018

@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:

requestBodySpec.content = {'application/json': {}};

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.

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.

This comment has been minimized.

@bajtos

bajtos Oct 25, 2018

Member

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)
```

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).

This comment has been minimized.

@bajtos

bajtos Oct 25, 2018

Member

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).

Show resolved Hide resolved packages/rest/src/parser.ts
Show resolved Hide resolved packages/rest/src/types.ts
Show resolved Hide resolved packages/rest/src/types.ts
@@ -46,11 +52,11 @@ export function validateRequestBody(
throw err;
}

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

This comment has been minimized.

@bajtos

bajtos Oct 25, 2018

Member

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;
  // ...
}
@@ -129,9 +208,12 @@ function buildOperationArguments(
}

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

This comment has been minimized.

@bajtos

bajtos Oct 25, 2018

Member

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.

});
});

let bodyParamControllerInvoked = false;

This comment has been minimized.

@bajtos

bajtos Oct 25, 2018

Member

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.

// 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

This comment has been minimized.

@bajtos

bajtos Oct 25, 2018

Member

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

function givenBodyParamController() {
bodyParamControllerInvoked = false;

This comment has been minimized.

@bajtos

bajtos Oct 25, 2018

Member

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 raymondfeng force-pushed the support-form-body branch 2 times, most recently from 928658b to eb3af8f Oct 25, 2018

@raymondfeng

This comment has been minimized.

Member

raymondfeng commented Oct 25, 2018

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

This comment has been minimized.

Member

raymondfeng commented Oct 25, 2018

@bajtos I have addressed your comments. PTAL.

@raymondfeng raymondfeng force-pushed the support-form-body branch from ecb2edb to acfb1ac Oct 25, 2018

```

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

This comment has been minimized.

@bajtos

bajtos Oct 26, 2018

Member

Great example 👍

@@ -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,

This comment has been minimized.

@bajtos

bajtos Oct 26, 2018

Member

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

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

This comment has been minimized.

@bajtos

bajtos Oct 26, 2018

Member

typo: more then. should be more than.

@@ -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)

This comment has been minimized.

@bajtos

bajtos Oct 26, 2018

Member

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?

@@ -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)

This comment has been minimized.

@bajtos

bajtos Oct 26, 2018

Member

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?

@@ -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)

This comment has been minimized.

@bajtos

bajtos Oct 26, 2018

Member

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?

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

This comment has been minimized.

@bajtos

bajtos Oct 26, 2018

Member

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?

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

This comment has been minimized.

@bajtos

bajtos Oct 26, 2018

Member

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?

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

const validationErrors = validate.errors;

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

This comment has been minimized.

@bajtos

bajtos Oct 26, 2018

Member

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

@bajtos

This comment has been minimized.

Member

bajtos commented Oct 26, 2018

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 raymondfeng force-pushed the support-form-body branch from acfb1ac to 1d43789 Oct 26, 2018

@raymondfeng

This comment has been minimized.

Member

raymondfeng commented Oct 26, 2018

@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

it('rejects unsupported request body', () => {

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

Added. See

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

PTAL.

@raymondfeng raymondfeng force-pushed the support-form-body branch 2 times, most recently from 0c0d7d3 to a478e56 Oct 26, 2018

@jannyHou

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

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

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

This comment has been minimized.

@jannyHou

jannyHou Oct 26, 2018

Contributor

nitpick: we can leverage OperationSpecBuild to build the spec.

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

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

This comment has been minimized.

@jannyHou

jannyHou Oct 26, 2018

Contributor

Does this PR support a referenced schema?

This comment has been minimized.

@raymondfeng

raymondfeng Oct 26, 2018

Member

I think we do.

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

This comment has been minimized.

@jannyHou

jannyHou Oct 26, 2018

Contributor

I think you created a type RequestBodyValidationOptions to be consistent.

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

This comment has been minimized.

@jannyHou

jannyHou Oct 26, 2018

Contributor

Hmm, any reason remove the ReferenceObject here?

This comment has been minimized.

@raymondfeng

raymondfeng Oct 26, 2018

Member

It's adding ReferenceObject back.

}
}

throw new HttpErrors.UnsupportedMediaType(

This comment has been minimized.

@jannyHou

jannyHou Oct 26, 2018

Contributor

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 raymondfeng force-pushed the support-form-body branch from a478e56 to ba40a29 Oct 26, 2018

@raymondfeng

This comment has been minimized.

Member

raymondfeng commented Oct 26, 2018

@jannyHou Good catches. I fixed them.

@bajtos

bajtos approved these changes Oct 29, 2018

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

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

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

This comment has been minimized.

@bajtos

bajtos Oct 29, 2018

Member

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.

feat(rest): add support for form request body
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 raymondfeng force-pushed the support-form-body branch from ba40a29 to 61834d8 Oct 29, 2018

@raymondfeng raymondfeng merged commit 2d9e0a8 into master Oct 29, 2018

4 of 5 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.07%) to 89.873%
Details
security/snyk - package.json (dhmlau) No manifest changes detected
@JesusTheHun

This comment has been minimized.

JesusTheHun commented Oct 29, 2018

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

@bajtos bajtos deleted the support-form-body branch Oct 30, 2018

@dhmlau dhmlau referenced this pull request Oct 31, 2018

Closed

Monthly Milestone - October 2018 🌦 #1761

15 of 17 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment