fix: emit "type: 'object'," for schema definitions by noels · Pull Request #3828 · loopbackio/loopback-next

noels · 2019-09-27T13:45:32Z

JSON Schema makes provision for structuring a complex schema into re-usable elements.
Loopback makes use of reference schema re-use through the '$ref' keyword.
in the JSON Schema documentation for complex schemas https://json-schema.org/understanding-json-schema/structuring.html the complex schema definitions are declared as type: 'object'.
Loopback generated JSON Schema omits this declaration. build-schema.ts:353.
Omitting this declaration causes ajv validation to pass when a required element (which is a reference to a different object) is set to null.
Adding the declaration: "type: 'object'" to generated schemas enables ajv validation to correctly fail when a required referenced property is present but the value set to null.

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

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 · 2019-09-27T18:44:35Z

  if (meta.description) {
    result.description = meta.description;
  }
+  result.type = 'object';


Isn't the type default to object if properties is present?

I am not sure, but the test fails when the element is not present so the validation does not work as expected.

raymondfeng · 2019-09-27T18:45:24Z

@noels Please also fix https://travis-ci.com/strongloop/loopback-next/jobs/239863212

bajtos

Thank you for the pull request. Please add a new test to verify the scenario you have described in PR description:

Loopback generated JSON Schema omits this declaration. build-schema.ts:353.
Omitting this declaration causes ajv validation to pass when a required element (which is a reference to a different object) is set to null.
Adding the declaration: "type: 'object'" to generated schemas enables ajv validation to correctly fail when a required referenced property is present but the value set to null.

This test should fail with a descriptive error message on the current master and pass with your changes in place.

noels · 2019-10-01T13:01:58Z

Thank you for the pull request. Please add a new test to verify the scenario you have described in PR description:

Loopback generated JSON Schema omits this declaration. build-schema.ts:353.

Omitting this declaration causes ajv validation to pass when a required element (which is a reference to a different object) is set to null.

Adding the declaration: "type: 'object'" to generated schemas enables ajv validation to correctly fail when a required referenced property is present but the value set to null.

This test should fail with a descriptive error message on the current master and pass with your changes in place.

Please can you give me some guidance as to where the test would be required. The framework tests the validation of the body in the Rest package tests (which I have now updated to work with my changes), but the schema is supplied to the validation requests as a test fixture. If I added a test there I think I would just be testing something that should already be covered in the ajv test suite. I have added tests that test for the presence of the 'type: "object" in the generated schema to the repository-json-schema package. If you can point me to where there are currently tests that verify that the generated JSONSchema works correctly with ajv validation I can add a new test there to ensure the behaviour is correct.

bajtos · 2019-10-01T14:24:17Z

@noels thanks for asking. I was thinking about adding a new test to packages/rest/src/__tests__/acceptance/validation/validation.acceptance.ts. Apparently, we already have a test to verify that request bodies containing only null are rejected:

https://github.com/strongloop/loopback-next/blob/512d37d4f8165ab02c522f071eb1862bf57125b8/packages/rest/src/__tests__/acceptance/validation/validation.acceptance.ts#L95-L101

If I understand your scenario correctly, then you want to reject requests when a required property is set to null. I think the simplest option is to define such property in the existing Product model and then add a new test to verify what happens when it's set to null. Just make sure to make the new property optional so that existing tests pass.

Alternatively, you can create a new context() block where you define a model & a controller that are specific to your scenario only. This is better in the long run, because we don't want the existing Product controller to grow super large to cover all different property types & definitions.

- JSON Schema makes provision for structuring a complex schema into re-usable elements. - Loopback makes use of reference schema re-use through the '$ref' keyword. - in the JSON Schema documentation for complex schemas https://json-schema.org/understanding-json-schema/structuring.html the complex schema definitions are declared as type: 'object'. - Loopback generated JSON Schema omits this declaration. build-schema.ts:353. - Omitting this declaration causes ajv validation to pass when a required element (which is a reference to a different object) is set to null. - Adding the declaration: "type: 'object'" to generated schemas enables ajv validation to correctly fail when a required referenced property is present but the value set to null. - fix failing failing tests. - Add a category to to product in rest acceptance tests in order to test complex schema functionaly.

noels · 2019-10-02T14:22:21Z

Thanks for the guidance. I have added an acceptance test to test for an object property being passed a null value. I have now fixed the prettier and commit message issues, and squashed the previous commits into a single commit for clarity.
I have expanded the product controller to have a Category member. I didn't think that the test was sufficiently different to have to duplicate the product and create a separate controller and helper functions. Let me know if you don't think this is okay. It looks like all the test will now pass.

bajtos

Thank you @noels for the update. AFAICT, you have updated the tests to verify the "happy path" when the user provided data for the nested object property. Now you also need to add a test to verify that a request setting such property to null is rejected. See your own description of the problem:

Omitting this declaration causes ajv validation to pass when a required element (which is a reference to a different object) is set to null.

…ypes

noels · 2019-10-04T15:59:07Z

It seems I forgot to include the new test when I copied everything over to a single commit. I've added the test. Please see if that's what you wanted.

bajtos

I am afraid the new test is not good enough, see my comment below.

Let's build a better understanding of the problem and come up with an improved test that better describes and verifies the bug you want to fix.

bajtos · 2019-10-11T12:01:23Z

+        .type('json')
+        .send(DATA)
+        .expect(422);
+    });


I am afraid this test is passing on the current master too. Here is the error reported:

{ statusCode: 422, name: 'UnprocessableEntityError', message: 'The request body is invalid. See error object `details` property for more info.', code: 'VALIDATION_FAILED', details: [ { path: '', code: 'additionalProperties', message: 'should NOT have additional properties', info: [Object] } ] }

@noels PTAL.

We need a test that fails on the current master and thus reproduces the bug you are trying to fix. Without such test, we have very little confidence that your changes are actually achieving the desired goal, plus it's easy to introduce unwanted regressions in the future.

HI @bajtos Thanks for having a look. I reset my local copy to the upstream master, replaced the validation.acceptance.ts file with the one from my branch, then ran mocha in loopback-next/packages/rest
and got:

Error: expected 422 "Unprocessable Entity", got 200 "OK" at Test._assertStatus (/home/noels/Documents/workspace/ho/GEFEAPI/loopback-next/packages/testlab/node_modules/supertest/lib/test.js:268:12) at Test._assertFunction (/home/noels/Documents/workspace/ho/GEFEAPI/loopback-next/packages/testlab/node_modules/supertest/lib/test.js:283:11) at Test.assert (/home/noels/Documents/workspace/ho/GEFEAPI/loopback-next/packages/testlab/node_modules/supertest/lib/test.js:173:18) at localAssert (/home/noels/Documents/workspace/ho/GEFEAPI/loopback-next/packages/testlab/node_modules/supertest/lib/test.js:131:12) at /home/noels/Documents/workspace/ho/GEFEAPI/loopback-next/packages/testlab/node_modules/supertest/lib/test.js:128:5 at Test.Request.callback (/home/noels/Documents/workspace/ho/GEFEAPI/loopback-next/packages/testlab/node_modules/superagent/lib/node/index.js:716:12) at parser (/home/noels/Documents/workspace/ho/GEFEAPI/loopback-next/packages/testlab/node_modules/superagent/lib/node/index.js:916:18) at IncomingMessage.res.on (/home/noels/Documents/workspace/ho/GEFEAPI/loopback-next/packages/testlab/node_modules/superagent/lib/node/parsers/json.js:19:7) at endReadableNT (_stream_readable.js:1059:12) at _combinedTickCallback (internal/process/next_tick.js:138:11) at process._tickCallback (internal/process/next_tick.js:180:9)

Did you npm run build before running the test? Am I missing something?

That's weird, let me try again.

I created a feature branch based on the current master (as of today morning), adding your commit with the test (b44cac5).

You can find the feature branch here:

https://github.com/strongloop/loopback-next/tree/test-pr-3828

When I run npm it (install & test), all tests are green.

I think the change in behavior may be caused by #3895, where we started to add additionalProperties: false to schema emitted for models in strict mode (which is the default).

When I change the definition of Product model in the acceptance test and disable strict mode, then your new test fails as expected. (Unfortunately, there is one more test that fails too.)

diff --git a/packages/rest/src/__tests__/acceptance/validation/validation.acceptance.ts b/packages/rest/src/__tests__/acceptance/validation/validation.acceptance.ts index 19697b83f..9070273fa 100644 --- a/packages/rest/src/__tests__/acceptance/validation/validation.acceptance.ts +++ b/packages/rest/src/__tests__/acceptance/validation/validation.acceptance.ts @@ -28,7 +28,7 @@ describe('Validation at REST level', () => { let app: RestApplication; let client: Client; - @model() + @model({settings: {strict: false}}) class Product { @property() id: number;

Obviously, we don't want to change all existing tests to start testing the scenario with strict: false and no longer test the default setup with strict: true. To support non-strict mode, we need to create a new group of tests in validation.acceptance.ts that will use a new model with strict: false.

Let me ask you a question: how are your models configured? Do you use the default "strict" model behavior, or are you disabling it via model settings? If you are using strict mode, then I think you should be find a this pull request is not needed for your app.

bajtos · 2019-10-11T12:04:20Z

@noels Last but not least, please rebase your patch on top of the latest master branch.

bajtos · 2020-02-17T13:33:21Z

@noels what's the status of this pull request? Are you still keen to continue working on this improvement?

stale · 2020-03-18T13:35:50Z

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.

noels requested review from bajtos and raymondfeng as code owners September 27, 2019 13:45

noels force-pushed the master branch from 4444076 to 9f88ad7 Compare September 27, 2019 15:32

noels changed the title ~~Emit "type: 'object'," for schema definitions.~~ Fix: Emit "type: 'object'," for schema definitions. Sep 27, 2019

raymondfeng reviewed Sep 27, 2019

View reviewed changes

dhmlau added the community-contribution label Sep 27, 2019

bajtos suggested changes Sep 30, 2019

View reviewed changes

noels force-pushed the master branch from 9f88ad7 to 3ccab98 Compare September 30, 2019 13:51

noels changed the title ~~Fix: Emit "type: 'object'," for schema definitions.~~ fix: emit "type: 'object'," for schema definitions Oct 1, 2019

noels force-pushed the master branch from c3eb35c to ad935df Compare October 2, 2019 13:33

noels force-pushed the master branch from ad935df to f328199 Compare October 2, 2019 14:07

noels closed this Oct 2, 2019

noels reopened this Oct 2, 2019

noels requested a review from bajtos October 2, 2019 15:00

raymondfeng approved these changes Oct 4, 2019

View reviewed changes

bajtos suggested changes Oct 4, 2019

View reviewed changes

fix: add test to ensure it rejects null values for required complex t…

b44cac5

…ypes

bajtos suggested changes Oct 11, 2019

View reviewed changes

bajtos requested review from deepakrkris, emonddr, hacksparrow and jannyHou February 17, 2020 13:33

bajtos added feature OpenAPI REST Issues related to @loopback/rest package and REST transport in general Validation stale labels Feb 17, 2020

stale Bot closed this Mar 18, 2020

bajtos mentioned this pull request Jun 16, 2020

fix(repository-json-schema): added type 'object' to model json schema #5749

Merged

7 tasks

Conversation

noels commented Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

raymondfeng Sep 27, 2019

Choose a reason for hiding this comment

Uh oh!

noels Sep 30, 2019

Choose a reason for hiding this comment

Uh oh!

raymondfeng commented Sep 27, 2019

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

noels commented Oct 1, 2019

Uh oh!

bajtos commented Oct 1, 2019

Uh oh!

noels commented Oct 2, 2019

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

noels commented Oct 4, 2019

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

bajtos Oct 11, 2019

Choose a reason for hiding this comment

Uh oh!

noels Oct 11, 2019 • edited by bajtos Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos Oct 14, 2019

Choose a reason for hiding this comment

Uh oh!

bajtos Oct 15, 2019

Choose a reason for hiding this comment

Uh oh!

bajtos commented Oct 11, 2019

Uh oh!

bajtos commented Feb 17, 2020

Uh oh!

stale Bot commented Mar 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

noels commented Sep 27, 2019 •

edited

Loading

noels Oct 11, 2019 •

edited by bajtos

Loading