Simplify model schema with excluded properties#3895
Conversation
|
Coverage is slightly lower than before. Please introduce additional tests. thx. |
nabdelgadir
left a comment
nabdelgadir
left a comment
There was a problem hiding this comment.
LGTM, but would it be possible to add a test that excludes more than one property?
agnes512
left a comment
agnes512
left a comment
There was a problem hiding this comment.
LGTM, just one question 😄
jannyHou
left a comment
jannyHou
left a comment
There was a problem hiding this comment.
👍 The code change LGTM.
@emonddr good point. Unfortunately I am not able to figure out which new lines are not covered now! The coverage report is confusing. It says that overall coverage fell down, but then it also says that coverage in
Maybe this is some kind of a rounding error, considering that the decrease is Can you perhaps spot which line is not covered? |
LoopBack 4 models are strict by default, they allow only properties explicitly described in model definition. On the other hand, JSON Schema is open by default and allows additional properties to be included in the payload. This change improves our code building JSON Schema from Model definition to correctly set `additionalProperties` flag, honoring `strict` setting. Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>
By default, additional properties are allowed in all objects. Before this change, our code converting JSON Schema to OpenAPI Schema was discarding `additionalProperties` field, therefere making it impossible to describe a schema that does not allow additional props. This change is fixing the conversion code to preserve `additionalProperties` value. Additionaly, we also fix the code generating schema for Filter & Where to set `additionalProperties` and to correctly apply default value of model setting `strict` (which is `true`). Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>
Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>
Remove the "not" section, rely on the fact that additional properties are disabled by default and therefore it's enough to exclude the configured properties from the schema. Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>
bajtos
force-pushed
the
fix/simplify-exclude-schema
branch
from
8a6ed1a to
d46cbe8
Compare
6 participants
In a recent discussion about using openapi-to-graphql with LoopBack apps, we found that the schema created by LoopBack 4 is difficult to interpret. Schema for creating new model instances (e.g.
NewTodo) is excluding the PK property (e.g.id) usingnotandanyOfoperators, see #3297.IMO, we should not need to use such construct, it should be enough to omit the excluded property from the schema.
While investigating such fix, I found out that JSON Schema allows arbitrary additional properties by default (
additionalProperties: true). At the same time, LoopBack 4 models are strict by default and don't allow arbitrary properties.The first commit removes this inconsistency by modifying
repository-json-schemato correctly setadditionalPropertiesbased onstrictmodel setting.The second commits fixes
openapi-v3to preserve boolean values ofadditionalPropertiesfield. (Before my change, only objects were honored.)The last commit finally simplifies the schema with excluded properties.
Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm testpasses on your machinepackages/cliwere updatedexamples/*were updated👉 Check out how to submit a PR 👈