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

Undefined bindings check interfering with custom serializing done in Pg's prepareValue #1898

Closed
moll opened this Issue Feb 5, 2017 · 3 comments

Comments

Projects
None yet
2 participants
@moll
Collaborator

moll commented Feb 5, 2017

Hey,

I'm still using Knex with Pg and Pg's custom type parsers and its serializers (through require("pg/lib/utils").prepareValue). Knex's containsUndefined check it does before compiling RAW queries is interfering with non plain objects that may have undefined properties, but which get entirely custom serializing through prepareValue mentioned above.

I suggest switching to test checking whether the object at hand is a plain one (with its constructor from Object.prototype) or optionally not checking at all.

Knex version in discussion is v0.12.6 and RAW queries are knex.raw("SELECT ?", [customObj]).

Cheers

@wubzz

This comment has been minimized.

Collaborator

wubzz commented Feb 6, 2017

During implementation I naturally didn't give this much thought. I think checking for plain objects is fine, as long as named bindings remain plain objects only as well (raw).

Perhaps it would also be a good idea to make a config switch for this to completely turn it off as well, but on by default. validateUndefinedBindings ?

@moll

This comment has been minimized.

Collaborator

moll commented Feb 6, 2017

Sure. I'm all for disabling. I overwrote the containsUndefined function with a noop as workaround as it was lazily bound in Knex and nothing bad happened. It's probably redundant anyways given how many layers there are checking for these things. :)

Not sure what you meant by "as long as named bindings remain plain objects", but to reiterate, bindings themselves need not remain plain objects as, for example, with the Postgres module there's a later serialization step converting objects to final types anyways.

@wubzz

This comment has been minimized.

Collaborator

wubzz commented Feb 14, 2018

I think we can settle this by only validating the object for undefined values if it is _.isPlainObject or _.isArray.

@wubzz wubzz closed this in #2468 Feb 14, 2018

wubzz added a commit that referenced this issue Feb 14, 2018

containsUndefined should only validate plain objects. Fixes #1898 (#2468
)

* containsUndefined should only validate plain objects. Fixes #1898

* Fix lint issues
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment