-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
fix: array decorator #5559
fix: array decorator #5559
Conversation
@@ -251,6 +251,34 @@ describe('build-schema', () => { | |||
expectValidJsonSchema(jsonSchema); | |||
}); | |||
|
|||
it('properly converts nested array property when json schema provided', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any tests on querying/filtering the nested arrays?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@agnes512 @loopback/repository-json-schema
doesn't have it. If we add tests, need to do them in the acceptance tests for connectors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mentioned it is because when I was triaging loopbackio/loopback-connector-postgresql#441, I realized that not all connectors support arrays/nested arrays. It'd be good if we can have them documented and tested. Just some thoughts 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized that not all connectors support arrays/nested arrays.
Yeah that's the problem, the original issue #4754 is not trying to support the full feature, it's a fix for the misuse of decorator.
It'd be good if we can have them documented and tested.
It's not a trivial thing to test across connectors, let's do it in a separate story.
jsonSchema: { | ||
type: 'array', | ||
items: {type: 'string'}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have another question of the design. Do we need to specify these two fields? if so, why do we need type: 'array'
as we already use the decorator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@agnes512 the jsonSchema
here is for the sub array property,
e.g. For Array<Array<string>>
, the json schema describes Array<string>
If the current doc confuses you:
To define a nested array property, you must provide the
jsonSchema
field to describe the sub-array property.
Any suggestion on how to make it more clear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see, then I am good with it 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully we can have some tests for different connectors in the future :D
Fixes #4754
To define a nested array property with
@property.array()
, you must provide a json schema to describe the sub array item:A few issues this PR addresses:
We didn't allow nested array property before due to
https://github.com/strongloop/loopback-next/blob/ef155eeddf11895bac8ddc491cb87c1e5b7398d0/packages/repository-json-schema/src/build-schema.ts#L260
which is not correct. I tested with todo app, when decorate the nested array property well, creating & retrieving an instance works properly.
It's not possible to infer the sub type of nested array property, that's why the json schema field is needed(Thank you @deepakrkris for your suggestion in defining a nested array model property using @property.array decorator crashes the app #4754 (comment)). I added a new check for that field to avoid triggering the subsequent error '"items" property must be present if "type" is an array' instead.
Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated👉 Check out how to submit a PR 👈