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

property.array generates incorrect schema #4645

Open
mastermunj opened this issue Feb 13, 2020 · 20 comments
Open

property.array generates incorrect schema #4645

mastermunj opened this issue Feb 13, 2020 · 20 comments
Assignees
Labels

Comments

@mastermunj
Copy link
Contributor

Here is the gist of code for easy replication.

Steps to reproduce

@property.array(String, {
  jsonSchema: {
    items: {
      "type": "string",
      "format": "email",
      "minLength": 5,
      "maxLength": 50,
      "transform": ["toLowerCase"]
    },
  },
  uniqueItems: true,
})
emails?: string[];

Current Behavior

Notice the nested items objects created.

"emails":{
  "type":"array",
  "items":{
    "type":"string",
    "items":{
      "type":"string",
      "format":"email",
      "minLength":5,
      "maxLength":50,
      "transform":[
        "toLowerCase"
      ]
    }
  }
}

Expected Behavior

"emails":{
  "type":"string",
  "items":{
    "type":"string",
    "format":"email",
    "minLength":5,
    "maxLength":50,
    "transform":[
      "toLowerCase"
    ]
  },
  "uniqueItems":true
}
@raymondfeng
Copy link
Contributor

@mastermunj The jsonSchema of @property.array applies to the item type. See #4646.

@raymondfeng
Copy link
Contributor

If you want to override array type, use @property instead.

@mastermunj
Copy link
Contributor Author

Does property.array ignore attributes other than jsonSchema? Because uniqueItems is missing from the generated schema in the above case.

@mastermunj
Copy link
Contributor Author

Also, when I try to run migration with @property having type: array following error occurs.

Migrating schemas (alter existing schema)
Cannot migrate database schema Error: Invalid type for property mobiles
    at Function.ModelClass.registerProperty (/app/node_modules/loopback-datasource-juggler/lib/model-builder.js:556:13)
    at ModelBuilder.defineClass [as define] (/app/node_modules/loopback-datasource-juggler/lib/model-builder.js:637:16)
    at MongoDataSource.defineClass (/app/node_modules/loopback-datasource-juggler/lib/datasource.js:837:40)
    at WorkshopContactPersonRepository.definePersistedModel (/app/node_modules/@loopback/repository/dist/repositories/legacy-juggler-bridge.js:113:39)
    at new DefaultCrudRepository (/app/node_modules/@loopback/repository/dist/repositories/legacy-juggler-bridge.js:77:32)
    at new EnhancedEntityRepository (/app/dist/repositories-base/enhanced-entity.repository.js:21:9)
    at new AuditableEntityRepository (/app/dist/repositories-base/auditable-entity.repository.js:24:9)
    at new WorkshopContactPersonRepository (/app/dist/repositories/workshop-contact-person.repository.js:22:9)
    at /app/node_modules/@loopback/context/dist/resolver.js:50:16
    at Object.transformValueOrPromise (/app/node_modules/@loopback/context/dist/value-promise.js:227:16)
    at Object.instantiateClass (/app/node_modules/@loopback/context/dist/resolver.js:45:34)
    at /app/node_modules/@loopback/context/dist/binding.js:433:46
    at Binding._getValue (/app/node_modules/@loopback/context/dist/binding.js:319:20)
    at /app/node_modules/@loopback/context/dist/binding.js:215:29
    at /app/node_modules/@loopback/context/dist/resolution-session.js:72:53
    at Object.tryWithFinally (/app/node_modules/@loopback/context/dist/value-promise.js:157:18)

PS: error is for different attribute mobiles, but specs are the same.

@property({
    type: 'array',
    jsonSchema: {
      items: {
        "type": "string",
        "pattern": "^[6789][0-9]{9}$",
        "minLength": 10,
        "maxLength": 10
    },
    uniqueItems: true,
  },
})

@dougal83

This comment has been minimized.

@mastermunj

This comment has been minimized.

@dougal83

This comment has been minimized.

@mastermunj

This comment has been minimized.

@dougal83

This comment has been minimized.

@mastermunj
Copy link
Contributor Author

mastermunj commented Feb 17, 2020

I have created an example repo that can showcase the issue better.

Please have a look at schema generated for attributes mobiles1, mobiles2 & mobiles3 of model Person.

  • mobiles1 uses @property.array which internally maps jsonSchema to items property of generated schema.
    Please note, how uniqueItems property also becomes part of items, which is expected.

  • mobiles2 uses @property.array again, with the difference being uniqueItems property being outside jsonSchema, completely ignored and not to be seen in the final schema.

  • mobiles3 uses @property with type:array and jsonSchema having definition for items and uniqueItems. This generates the correct schema. However, when one tries to run npm run migrate an error is generated as below.

Migrating schemas (alter existing schema)
Cannot migrate database schema Error: Invalid type for property mobiles3
    at Function.ModelClass.registerProperty (/Users/23197096D/lb4-property-decorator/node_modules/loopback-datasource-juggler/lib/model-builder.js:556:13)
    at ModelBuilder.defineClass [as define] (/Users/23197096D/lb4-property-decorator/node_modules/loopback-datasource-juggler/lib/model-builder.js:637:16)
    at MemoryDataSource.defineClass (/Users/23197096D/lb4-property-decorator/node_modules/loopback-datasource-juggler/lib/datasource.js:837:40)
    at PersonRepository.definePersistedModel (/Users/23197096D/lb4-property-decorator/node_modules/@loopback/repository/dist/repositories/legacy-juggler-bridge.js:113:39)
    at new DefaultCrudRepository (/Users/23197096D/lb4-property-decorator/node_modules/@loopback/repository/dist/repositories/legacy-juggler-bridge.js:77:32)
    at new PersonRepository (/Users/23197096D/lb4-property-decorator/dist/repositories/person.repository.js:21:9)
    at /Users/23197096D/lb4-property-decorator/node_modules/@loopback/context/dist/resolver.js:50:16
    at Object.transformValueOrPromise (/Users/23197096D/lb4-property-decorator/node_modules/@loopback/context/dist/value-promise.js:227:16)
    at Object.instantiateClass (/Users/23197096D/lb4-property-decorator/node_modules/@loopback/context/dist/resolver.js:45:34)
    at /Users/23197096D/lb4-property-decorator/node_modules/@loopback/context/dist/binding.js:433:46
    at Binding._getValue (/Users/23197096D/lb4-property-decorator/node_modules/@loopback/context/dist/binding.js:319:20)
    at /Users/23197096D/lb4-property-decorator/node_modules/@loopback/context/dist/binding.js:215:29
    at /Users/23197096D/lb4-property-decorator/node_modules/@loopback/context/dist/resolution-session.js:72:53
    at Object.tryWithFinally (/Users/23197096D/lb4-property-decorator/node_modules/@loopback/context/dist/value-promise.js:157:18)
    at Function.runWithBinding (/Users/23197096D/lb4-property-decorator/node_modules/@loopback/context/dist/resolution-session.js:72:32)
    at Binding.getValue (/Users/23197096D/lb4-property-decorator/node_modules/@loopback/context/dist/binding.js:213:67)
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! [email protected] migrate: `node ./dist/migrate`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the [email protected] migrate script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

@dougal83
Copy link
Contributor

Ref: Openapi Spec - Array, Unique items

@mastermunj I see what you're aiming for here, an array with unique items but I'm not sure that Loopback supports this particular route to achieve it. Although you can update the schema to add uniqueItems, the database might not apply any restriction, hence migration failing. Would an alternative with restriction on the model level work? As follows:

  • Create a model for the items with the unique restriction.
  • Use relationships to connect data into your current model.

@agnes512 @hacksparrow PTAL

@mastermunj
Copy link
Contributor Author

@dougal83 Loopback does support this validation at rest level through ajv.

The point I am trying to make here is that if I use @property.array wrong schema gets generated. If I use @property with type:array migration fails with the error shared earlier.

Also, migration error states that type is invalid for property and not that property schema has an extra attribute.

@achrinza
Copy link
Member

From what I can gather:

  1. The following code results in a nested items which is invalid schema. This is due to jsonSchema being mapped directly to items. This is expected behaviour. Though we could use some better typings to prevent this from being accepted.
    @property.array(String, {
        jsonSchema: {
            items: {
                "type": "string",
                "format": "email",
                "minLength": 5,
                "maxLength": 50,
                "transform": ["toLowerCase"]
                },
            },
            uniqueItems: true,
    })
    emails?: string[];
  2. uniqueItems outside of jsonSchema is ignored. This is expected behaviour.
  3. uniqueItems is placed within items in the generated openapi.json. This is expected behaviour
  4. npm run migrate on @property({type: 'array'}) fails. This is unexpected behaviour.

Hence, the issue at hand is regarding npm run migrate not working with @property({type: 'array'})

achrinza added a commit to achrinzafork/loopback-next that referenced this issue Feb 21, 2020
achrinza added a commit to achrinzafork/loopback-next that referenced this issue Feb 21, 2020
achrinza added a commit to achrinzafork/loopback-next that referenced this issue Feb 21, 2020
@dougal83 dougal83 assigned achrinza and unassigned dougal83 Feb 22, 2020
@InvictusMB
Copy link
Contributor

  • uniqueItems is placed within items in the generated openapi.json. This is expected behaviour

@achrinza Why is this an expected behaviour? If uniqueItems is placed within items it is not enforced by the validator.

There seems to be no way to ensure unique items right now.
If I use property.array() or provide itemType to property() it generates a nested items spec.
If I don't use property.array() or provide itemType to property() it generates a proper spec but the ORM explodes.

@achrinza
Copy link
Member

achrinza commented Jun 19, 2020

@InvictusMB Sorry for the delayed reply,

If uniqueItems is placed within items it is not enforced by the validator.

My understanding was that because the generated OpenAPI spec passed Swagger editor validation, that it would result in expected behaviour.

However, it seems like that's not the case.

but the ORM explodes.

Could you clarify? The ORM works independently of the OpenAPI spec.


Just to make sure we're on the same page, uniqueItems should only enforce uniqueness within the request itself, and will not check against the database. Database-level uniqueness should be enforced by id: true.

I'll need to investigate this further.

@InvictusMB
Copy link
Contributor

@achrinza

My understanding was that because the generated OpenAPI spec passed Swagger editor validation, that it would result in expected behaviour.
However, it seems like that's not the case.

Correct, when uniqueItems is within items it is not enforced: https://www.jsonschemavalidator.net/s/cW5hOu01
It has to be outside items for JSON Schema validator to enforce it: https://www.jsonschemavalidator.net/s/RofAfL0r

If I don't use property.array() or provide itemType to property() it generates a proper spec but the ORM explodes.

Could you clarify? The ORM works independently of the OpenAPI spec.

The ORM depends on the same metadata from property decorator. If I don't use @property.array() or provide itemType to @property() it is not able to map that property as array.

A proper fix would be to handle uniqueItems outside of jsonSchema and put it in the appropriate place in output OpenAPI spec.

@property.array(String, {
  uniqueItems: true,
  jsonSchema: {
    type: "string",
    enum: ["foo", "bar"],
  }
})
things: string[];

should result in

{
  ...
  "properties": {
    ...
    "things": {
      "type": "array",
      "items": {
        "type": "string",
        "enum": ["foo", "bar"],
      },
      "uniqueItems": true
    }
  }
}

@achrinza
Copy link
Member

achrinza commented Aug 15, 2020

Thanks for the breakdown, @InvictusMB. I agree with most points, though I'd like to take a different approach to fixing the issue:

IIRC, properties inside jsonSchema are considered for REST-layer validation only. Meaning the ORM does not look inside jsonSchema for validation, only AJV. In contrast, properties outside of jsonSchema are intended for REST and ORM level validation.

Hence with that in mind, I'd like to propose to keep uniqueItems inside jsonSchema, and have LoopBack automatically move uniqueItems outside of items.

This applies to other properties that may have the same issue.

@fabiohbarbosa
Copy link

I've got a similar issue, but my issue is related to open API spec generator that I've been using to validate REST requests.
It's been impossible adding the minItems validation to the types of array. Any workaround for that? I tried to set the jsonSchema manually over the @model decorator, but it replaces the all spec of the object and I need to write the object spec manually :/

@achrinza
Copy link
Member

achrinza commented Oct 5, 2020

A workaround is to use @property() and create the OAS3 Parameter Object from there.

@stale
Copy link

stale bot commented Sep 7, 2021

This issue has been marked stale because it has not seen activity within six months. 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. This issue will be closed within 30 days of being stale.

@stale stale bot added the stale label Sep 7, 2021
@achrinza achrinza removed the stale label Sep 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants