-
-
Notifications
You must be signed in to change notification settings - Fork 198
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
perf: replace deepmerge npm-package with @fastify/deepmerge #483
perf: replace deepmerge npm-package with @fastify/deepmerge #483
Conversation
I know this PR should just update to the fastest merge version, but if we touch on this topic, the general merge function is not suitable for schema merging. There are two places now where we are merging schema if/then and allOf, and both of them should be merged according to the rules of the schemes. #453 fixed some cases, but, I guess it doesn't cover all cases. We can take a look at json-schema-merge-allof and use it or take some ideas. |
Wow thats crazy that they wrote that in lodash :D. |
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.
There are two places now where we are merging schema if/then and allOf, and both of them should be merged according to the rules of the schemes
I think this is another codebase aspect and does not relate to this specific dep change
@Eomm |
If you would like to work it, extending the
Regarding this, I was thinking quite the same about the |
created a PR for deepmerge to handle also nullPrototype |
I really think we should merge and publish first the nullPrototype PR and then use the nullPrototype mode for merging Schema. |
I don't think we need null prototype protection at all here. Schemas are considered as safe as code is. |
If we dont need null prototype, then this PR should be ready to merge ;) |
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
I have just one worry: @fastify/deepmerge has prototype protection handling. What happens if in the json schema an type: object has the key prototype or constructor? It would mean that those keys are not merged/carried over. should we change deepmerge to have an option where we create Objects by using Object.create(null) so that a prototype pollution is not possible?