-
Notifications
You must be signed in to change notification settings - Fork 138
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
db: add allowPrimaryKeyInInput option #2082
Conversation
Signed-off-by: Matteo Collina <hello@matteocollina.com>
|
||
{ | ||
"id": 1, |
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.
IMO, if allowIdInInput
is set to false
it should generate a route schema without an id
prop and here it should throw a 400 because data has an ddditional property. And if you do it programatically (with sql-mapper) it also should throw an error. Otherwise it looks like a place for a very nasty bug. wdyt?
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.
That’s how the schemas are generated. However Ajv/Fastify do not error in this case, but they rather filter the additional property.
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.
- Then IMHO it should throw an error on a
sql-mapper
level. - It's easy to create a schema that would fail if some known property exist. Just set
false
for a property schema.
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.
My problem is that it would be really hard to notice the problem if you pass an id and allowPrimaryKeyInInput
set to false. If you think that skipping is the best behavior it should at least log a warn message.
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.
It’s not possible to do it in sql-mapper because I’ve implemented .save() as an upsert.
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.
It's easy to create a schema that would fail if some known property exist. Just set false for a property schema.
Oh I’ll try this!
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.
Oh I’ll try this!
Or you can do this. Choose the error message you like more.
{
"type": "object",
"not": {
"required": ["foo"]
}
}
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.
Done with a slightly different syntax that gave a better error message.
Co-authored-by: Ivan Tymoshenko <ivan@tymoshenko.me>
Co-authored-by: Ivan Tymoshenko <ivan@tymoshenko.me>
Co-authored-by: Ivan Tymoshenko <ivan@tymoshenko.me>
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.
lgtm
No description provided.