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

Update isPlainObject to exclude objects with constructors #797

Closed
wants to merge 5 commits into from

Conversation

sds
Copy link

@sds sds commented Dec 5, 2023

Hey all, I'm not sure if this is the right solution to the issue we're observing, so would welcome feedback on a better way to solve.

If you are using a custom type parser (e.g. via pg.types.setTypeParser) that returns a custom class, the camel case plugin will incorrectly serialize that value to a literal object, stripping it of its prototype.

This is problematic if you rely on the ability to call certain methods specified by the custom class' prototype.

To date, isPlainObject is used only in the camel case and JSON parsing plugins. I'm not sure what the original intent of the isPlainObject implementation was, so let me know if we think this fix belongs in the camel case plugin.

Copy link

vercel bot commented Dec 5, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
kysely ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 21, 2024 10:16am

@koskimas
Copy link
Member

koskimas commented Dec 6, 2023

I don't think this works in every case. You need to at least add a test that checks all the old cases we were detecting specifically. And how about cases where there's no constructor? Like isPlainObject(Object.create(null))?

If you are using a custom type parser (via `pg.types.setTypeParser`)
that returns a custom class, the camel case plugin will incorrectly
serialize that value to a literal object, stripping it of its prototype.

This is problematic if you rely on the ability to call certain methods
specified by the custom class' prototype.
@sds
Copy link
Author

sds commented Dec 13, 2023

Thanks for the feedback! Would you want a separate test file for isPlainObject, or tests added to the existing camel case plugin tests?

In the meantime, I pushed an updated implementation that handles the Object.create(null) case.

@igalklebanov
Copy link
Member

@sds let's add a separate test file, and verify there is no regression.

@koskimas koskimas closed this May 25, 2024
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

3 participants