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

Validation passes when an required complex property value is set to an empty string. #3804

Closed
noels opened this issue Sep 25, 2019 · 1 comment · Fixed by #5749
Closed

Validation passes when an required complex property value is set to an empty string. #3804

noels opened this issue Sep 25, 2019 · 1 comment · Fixed by #5749

Comments

@noels
Copy link

noels commented Sep 25, 2019

Steps to reproduce

Create a model which references another model. e.g.

export class Order extends Entity {
  @property({
    type: 'number',
    id: true,
    generated: false,
  })
  id: number;

  @property({
    type: 'string',
  })
  orderNumber: string;

  @property({
    type: Customer,
    required: true,
  })
  customer: Customer;


  constructor(data?: Partial<Order>) {
    super(data);
  }
}

export class Customer extends Entity {
  @property({
    type: 'string',
    required: true,
  })
  name: string;
}

export class Customer extends Entity {
  @property({
    type: 'number',
    id: true,
    required: true,
    generated: false,
  })
  id: number;

  @property({
    type: 'string',
    required: true,
  })
  name: string;


  constructor(data?: Partial<Customer>) {
    super(data);
  }
}

Generate a datasource, repository and controller for Order.

Current Behavior

Posting :

{
  "orderNumber": "123413",
  "customer": {} 
}

to the /orders endpoint correctly validates that customer is required, however posting:

{
  "orderNumber": "123413",
  "customer": ""
}

successfully creates a new order with an empty customer object.

Expected Behavior

I would expect that the second post above would not pass validation and throw a 422 error and not create an order with an invalid customer object.

Link to reproduction sandbox

https://github.com/SilexConsulting/loopback-next/tree/validation-should-fail-empty-required-object
npm test will have a single failing test to demonstrate the issue.

Additional information

I'm sure the issue is not relevant to the platform or node/npm version but it is:
linux x64 11.8.0

loopback version tested is:
├── @loopback/boot@1.5.6
├── @loopback/context@1.23.0
├── @loopback/core@1.10.2
├── @loopback/openapi-v3@1.9.7
├── @loopback/repository@1.14.0
├── @loopback/rest@1.19.0
├── @loopback/rest-explorer@1.3.7
└── @loopback/service-proxy@1.3.6

I have confirmed that the behaviour is the same with the head of master.

I believe that this behaviour is due to the fact that the JSON schema generated by repository-json-schema omits the type: object for LB models. I think from the documentation here: https://json-schema.org/understanding-json-schema/structuring.html that complex shemas should have a type:object property.

I have made a change in my fork of LB here: SilexConsulting@4444076 which produces the expected results and all the modules tests pass.

@noels noels added the bug label Sep 25, 2019
@bajtos
Copy link
Member

bajtos commented Sep 27, 2019

Thank you @noels for the bug report, a detailed analysis and a PoC solution.

I believe that this behaviour is due to the fact that the JSON schema generated by repository-json-schema omits the type: object for LB models. I think from the documentation here: https://json-schema.org/understanding-json-schema/structuring.html that complex shemas should have a type:object property.

I have made a change in my fork of LB here: SilexConsulting/loopback-next@4444076 which produces the expected results and all the modules tests pass.

I reviewed your changes, they make perfect sense to me. Can you open a pull request please to contribute them to the framework?

See our Contributing guide and Submitting a pull request to LoopBack 4 to get started.

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

Successfully merging a pull request may close this issue.

3 participants