Validate request body arguments #1489

jannyHou · 2018-06-29T17:50:54Z

related to #118

Finished items are checked in #118 (comment)

Checklist

Next steps

Open follow-up pull requests for the following items:

Edge cases
- array
- object
More descriptive error object including machine-readable details
Nice to have: unit-tests in openapi-v3 to verify that user-provided definitions are preserved - see the comments below

Out of scope of this initial pull request and DP3

Support for multiple entries in requestBody.content map (e.g. application/json vs. application/xml)
Support for arbitrary $ref references (see [SPIKE] Resolve $ref in api/controller/route specs #435)
Reusing (pre)compiled AJV validators for better performance

bajtos · 2018-07-02T13:37:53Z

Dereference the schema object from openapiSpec.components.schemas

end to end test

Done in 62ea12a. I updated the pull request description with additional tasks and also things out of scope.

Next, I'll review the changes and look for places that need cleanup and/or better test coverage.

bajtos · 2018-07-03T10:43:07Z

packages/openapi-v3/src/controller-spec.ts

@@ -178,16 +178,34 @@ function resolveControllerSpec(constructor: Function): ControllerSpec {
        if (!spec.components.schemas) {
          spec.components.schemas = {};
        }
+        if (p.name in spec.components.schemas) {
+          // Preserve user-provided definitions


We don't have any test for this branch in openapi-v3 testsuite. Should we add one?

bajtos · 2018-07-03T10:43:46Z

packages/openapi-v3/src/controller-spec.ts

        if (openapiSchema.definitions) {
          for (const key in openapiSchema.definitions) {
-            spec.components.schemas[key] = openapiSchema.definitions[key];
+            // Preserve user-provided definitions
+            if (key in outputSchemas) continue;


This is implicitly tested by an acceptance test in @loopback/rest. Should we add a more focused test to openapi-v3 too?

bajtos · 2018-07-03T14:07:26Z

@jannyHou today, I cleaned up the implementation a bit and added few more tests, both at validation and unit level. As far as I am concerned, this pull request is good to be landed. Could you please review the final patch, perhaps get other team members to review it too, address any review feedback and land this pull request?

I updated the pull request description with the next steps to work on after this PR is landed:

Edge cases (array, object)
More descriptive error object including machine-readable details
Nice to have: unit-tests in openapi-v3 to verify that user-provided definitions are preserved

And also updated the list of things that are out of scope of DP3:

Support for multiple entries in requestBody.content map (e.g. application/json vs. application/xml)
Support for arbitrary $ref references (see [SPIKE] Resolve $ref in api/controller/route specs #435)
Reusing (pre)compiled AJV validators for better performance

Could you please create follow-up issues where it makes sense? For example, I am not sure how to approach multiple content-type entries - do we actually support them in other places of the REST layer? Maybe we need an issue to address this aspect of OpenAPI spec holistically, not only for validation.

bajtos · 2018-07-03T14:08:17Z

@jannyHou last but not least - please preserve my authorship when cleaning up the git commit history, see https://help.github.com/articles/creating-a-commit-with-multiple-authors/

jannyHou

@bajtos your changes LGTM 👍 Thank you for refactoring the tests and added all those missing functionalities, a great effort and improvement!
*I could not approve my own PR that's why it's a comment, *

jannyHou · 2018-07-04T20:35:28Z

packages/rest/src/parser.ts

  if (contentType && !/json/.test(contentType)) {
    throw new HttpErrors.UnsupportedMediaType(
      `Content-type ${contentType} is not supported.`,
    );
  }

  return await parseJsonBody(request).catch((err: HttpError) => {
+    debug('Cannot parse request body', err);


nitpick: debug('Cannot parse request body %j', err)

I intentionally didn't use %j because I want the error to be formatted using util.inspect, not JSON.stringify.

I think we should use %s and inspect(err) as we are already doing in other places?

I will change it in #1495

jannyHou · 2018-07-04T20:37:21Z

packages/rest/src/providers/reject.provider.ts

@@ -23,6 +25,16 @@ export class RejectProvider implements Provider<Reject> {
    const err = <HttpError>error;
    const statusCode = err.statusCode || err.status || 500;
    writeErrorToResponse(response, err);
+
+    // Always log the error in debug mode, even when the application
+    // has a custom error logger configured (e.g. in tests)


jannyHou · 2018-07-04T21:16:47Z

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

+  if (!requestBodySpec) return;
+
+  const content = requestBodySpec.content;
+  // FIXME(bajtos) we need to find the entry matching the content-type


👍 Good catch, doesn't look a big code change, opened an issue to track it: #1494
Will try to address it in the next PR(in a reasonable time)

jannyHou · 2018-07-04T21:17:49Z

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

+  // A temporary solution for resolving schema references produced
+  // by @loopback/repository-json-schema. In the future, we should
+  // support arbitrary references anywhere in the OpenAPI spec.
+  // See https://github.com/strongloop/loopback-next/issues/435


jannyHou · 2018-07-04T21:38:23Z

packages/rest/test/helpers.ts

@@ -25,3 +30,16 @@ export function createUnexpectedHttpErrorLogger(
    );
  };
 }
+
+export function aBodySpec(


👍 I love the idea to create a function generating a body spec, and we can extend the function to allow multiple contents in story #1494

jannyHou · 2018-07-04T21:50:14Z

packages/rest/test/unit/request-body.validator.test.ts

+) {
+  // workaround for Function.prototype.bind not preserving argument types
+  function validateRequestBodyWithBoundArgs() {
+    validateRequestBody(body, spec, schemas);


@bajtos Any reason why we don't run the unit tests with a mock request but testing validateRequestBody directly? Is it because the e-2-e test takes the responsibility of testing a real request?

Is it because the e-2-e test takes the responsibility of testing a real request?

Exactly.

Unit tests should be testing a single unit, in this case the function validateRequestBody.

To verify how validateRequestBody is integrated with the rest of REST layer, we can write few integration or acceptance tests. In this case, we have acceptance tests.

See also https://loopback.io/doc/en/lb4/Defining-your-testing-strategy.html

virkt25

LGTM

Co-authored-by: Miroslav Bajtos <[email protected]>

jannyHou · 2018-07-09T13:51:47Z

@bajtos I was trying to address "More descriptive error object including machine-readable details" in the next PR, but didn't find any clue from AJV doc, could you elaborate more about the "machine-readable details"? Thanks!

bajtos · 2018-07-09T15:45:23Z

Here is the end-user experience I am looking for:

Imagine you are building a single-page web application with a registration form where the user enters email, password, name, etc. When they submit the form, the page makes a REST request to LB4 backend. When the request fails because of a validation error, the page needs to tell user what went wrong and how to fix the filled data. To do so, the page needs to know:

Which model properties are not valid, so that corresponding input forms can be highlighted (e.g. email, password).
Validation error to display to the user for each input form. It should be easy to localize these errors, therefore we need an error code (e.g. code: "required" for a missing required field) in addition to English message (e.g. "email is required" - whatever is returned back by AJV).

See how LB3.x/Juggler validation works and the example of a ValidationError shown here: https://loopback.io/doc/en/lb3/Validating-model-data.html#localizing-validation-messages

From what I remember, AJV provides additional structured data about the validation errors, we just need to find out where it is stored (I think it's ajv.errors) and how to present it to REST clients in the HTTP response. See https://github.com/epoberezkin/ajv#validation-errors for more details about the format of the error object returned by AJV, you may need to inspect the actual object at runtime to build a better understanding.

virkt25 · 2018-07-09T16:25:43Z

Which model properties are not valid, so that corresponding input forms can be highlighted (e.g. email, password).

I know this is an example but for a login form it's bad practice to point out the field in which the error / validation failed due to security implications / hints it provides. For a registration form however it's not the case and individual fields should be pointed out. -- Something to keep in mind once we work on auth / User model.

jannyHou · 2018-07-11T18:27:05Z

but for a login form it's bad practice to point out the field in which the error / validation failed due to security implications / hints it provides.

@virkt25 I agree that hiding sensitive info in response is important. While wondering should we do it or the lb app developer do it by filtering the full response data/error. IMO we should do the default error handling of "auth / User model", while for other cases, I prefer to let developers to filter the error. We can make it easy to add an action that applies the customized error filter.

Feel free to move the discussion to #1511, up to you :)

David-Mulder · 2018-10-17T12:15:02Z

@virkt25 Do note: Most sites already expose whether an a username/email is already registered on the registration page, so

bad practice to point out the field in which the error / validation failed due to security implications / hints it provides

only applies to a subset of systems which do not have any public registration page.

jannyHou requested review from bajtos and raymondfeng as code owners June 29, 2018 17:50

jannyHou force-pushed the validate-request-body branch 2 times, most recently from 52fecaa to 68ca845 Compare June 29, 2018 20:30

jannyHou mentioned this pull request Jun 29, 2018

Router: validate input parameters #118

Closed

10 tasks

jannyHou changed the title ~~[DO NOT REVIEW]feat: body validation~~ feat: body validation Jul 1, 2018

jannyHou force-pushed the validate-request-body branch from 68ca845 to f2d1117 Compare July 1, 2018 23:04

bajtos changed the title ~~feat: body validation~~ Validate request body arguments Jul 2, 2018

bajtos reviewed Jul 3, 2018

View reviewed changes

bajtos force-pushed the validate-request-body branch from 37b29df to 267fd47 Compare July 3, 2018 14:01

bajtos approved these changes Jul 3, 2018

View reviewed changes

jannyHou force-pushed the validate-request-body branch from 267fd47 to f1ca281 Compare July 4, 2018 18:57

jannyHou commented Jul 4, 2018

View reviewed changes

virkt25 approved these changes Jul 4, 2018

View reviewed changes

feat: body validation

a66ebb4

Co-authored-by: Miroslav Bajtos <[email protected]>

jannyHou force-pushed the validate-request-body branch from f1ca281 to a66ebb4 Compare July 5, 2018 15:10

jannyHou merged commit d284ad8 into master Jul 5, 2018

jannyHou deleted the validate-request-body branch July 5, 2018 18:02

jannyHou mentioned this pull request Jul 5, 2018

add tests for array and object #1495

Merged

7 tasks

shimks added this to the July Milestone milestone Jul 10, 2018

jannyHou mentioned this pull request Jul 11, 2018

feat: localize error in details #1511

Merged

jannyHou mentioned this pull request Jul 11, 2018

docs: add parsing request #1519

Merged

7 tasks

dhmlau mentioned this pull request Jul 21, 2018

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

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate request body arguments #1489

Validate request body arguments #1489

jannyHou commented Jun 29, 2018 •

edited by bajtos

Loading

bajtos commented Jul 2, 2018

bajtos Jul 3, 2018

bajtos Jul 3, 2018

bajtos commented Jul 3, 2018

bajtos commented Jul 3, 2018

jannyHou left a comment •

edited

Loading

jannyHou Jul 4, 2018

bajtos Jul 9, 2018

jannyHou Jul 9, 2018

jannyHou Jul 4, 2018

jannyHou Jul 4, 2018

jannyHou Jul 4, 2018

jannyHou Jul 4, 2018

jannyHou Jul 4, 2018

bajtos Jul 9, 2018

virkt25 left a comment

jannyHou commented Jul 9, 2018

bajtos commented Jul 9, 2018

virkt25 commented Jul 9, 2018

jannyHou commented Jul 11, 2018 •

edited

Loading

David-Mulder commented Oct 17, 2018

Validate request body arguments #1489

Validate request body arguments #1489

Conversation

jannyHou commented Jun 29, 2018 • edited by bajtos Loading

Checklist

Next steps

Out of scope of this initial pull request and DP3

bajtos commented Jul 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Jul 3, 2018

bajtos commented Jul 3, 2018

jannyHou left a comment • edited Loading

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

Choose a reason for hiding this comment

virkt25 left a comment

Choose a reason for hiding this comment

jannyHou commented Jul 9, 2018

bajtos commented Jul 9, 2018

virkt25 commented Jul 9, 2018

jannyHou commented Jul 11, 2018 • edited Loading

David-Mulder commented Oct 17, 2018

jannyHou commented Jun 29, 2018 •

edited by bajtos

Loading

jannyHou left a comment •

edited

Loading

jannyHou commented Jul 11, 2018 •

edited

Loading