Skip to content
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

Discriminator validation issue #10655

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

pnoebauer
Copy link

High-Level Description

This pull request illustrates a bug in the LoopBack framework causing discriminators to not work. The issue is due to LoopBack-Rest invoking the openapi-schema-to-json-schema library function incorrectly under loopback-next/packages/rest/src/validation/request-body.validator.ts, which results in the removal of OpenAPI properties such as discriminator.

Changes Made

Bug Illustration:
The bug is located in loopback-next/packages/rest/src/validation/request-body.validator.ts#L87.
Added comments and TODO notes in the pr to highlight the issue.
Demonstrated the bug in the /examples/validation-app by adding a controller, models, and tests.
Code Changes:
In loopback-next/packages/rest/src/validation/request-body.validator.ts:

function convertToJsonSchema(openapiSchema: SchemaObject) {
  /*
    loopback does not pass the options arg to function toJsonSchema(schema, options?),
    which is part of the openapi-schema-to-json-schema library;
    by default toJsonSchema() removes discriminators, which is why they do not work
    in loopback
  */
  // TODO: fix below bug; toJsonSchema requires the options object to enable discriminators
  const jsonSchema = toJsonSchema(openapiSchema);
  // NOTE: replacing above line with below fixes discriminators not working in loopback and all tests pass
  // const jsonSchema = toJsonSchema(openapiSchema, {
  //   keepNotSupported: ['discriminator'],
  // });
}

Example Application:
Created an example application under /examples/validation-app to reproduce the issue.
Added a controller and models to showcase the problem.
Included tests that demonstrate the expected error messages if discriminators worked correctly.

@aaqilniz
Copy link
Contributor

Hi, @pnoebauer. Can you please uncomment the actual change we need to fix the issue?

@pnoebauer
Copy link
Author

Hi, @pnoebauer. Can you please uncomment the actual change we need to fix the issue?

Hi, @pnoebauer. Can you please uncomment the actual change we need to fix the issue?

Hi, sure! I have just pushed another commit with the comments removed and a possible bug fix.

@pnoebauer
Copy link
Author

@aaqilniz @rmg @klassicd @dhmlau could you please review this pr

please advise if any changes are needed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants