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

perf: replace deepmerge npm-package with @fastify/deepmerge #483

Merged

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Jun 30, 2022

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?

@ivan-tymoshenko
Copy link
Member

ivan-tymoshenko commented Jun 30, 2022

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.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jul 1, 2022

@ivan-tymoshenko

Wow thats crazy that they wrote that in lodash :D.

Copy link
Member

@Eomm Eomm left a 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

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jul 1, 2022

@Eomm
I am currently thinking if I should implement a mode where we create Objects without prototype (Object.create(null)) and then deepmerge can handle keys like prototype and constructor. Should I focus on this next?

@Eomm
Copy link
Member

Eomm commented Jul 1, 2022

Should I focus on this next?

If you would like to work it, extending the @fastify/deepmerge go for it.
In this case we should measure it against rfdc: https://www.npmjs.com/package/rfdc

Object.create(null)

Regarding this, I was thinking quite the same about the fastify repo. We have some {} around that could be a simple object without a prototype and I wonder how much memory benefit the server instance could get from this change

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jul 3, 2022

created a PR for deepmerge to handle also nullPrototype
fastify/deepmerge#2

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jul 3, 2022

I really think we should merge and publish first the nullPrototype PR and then use the nullPrototype mode for merging Schema.

@Eomm Eomm requested a review from mcollina July 3, 2022 09:46
@mcollina
Copy link
Member

mcollina commented Jul 3, 2022

I don't think we need null prototype protection at all here. Schemas are considered as safe as code is.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jul 5, 2022

If we dont need null prototype, then this PR should be ready to merge ;)

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina merged commit 0718aba into fastify:master Jul 5, 2022
@Uzlopak Uzlopak deleted the replace-deepmerge-with-fastify-deepmerge branch July 5, 2022 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants