Skip to content

Conversation

@aars
Copy link

@aars aars commented Jan 14, 2015

Removes keys with undefined values from data before creating SQL query.
This prevents sending NULL values to Postgres for undefined values,
which prevents the Postgres from handling default values.

(Postgre)SQL does support undefined values. If you want to use NULL
values you should define them as such.

Removes keys with undefined values from data before creating SQL query.
This prevents sending NULL values to Postgres for undefined values,
which prevents the Postgres from handling default values.

(Postgre)SQL does support undefined values. If you want to use NULL
values you should define them as such.
@slnode
Copy link

slnode commented Jan 14, 2015

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@aars
Copy link
Author

aars commented Jan 14, 2015

I know this might be a bit invasive, since this will be used on all queries, but I see no reason to add an option/switch or other way to circumvent this behavior.

@aars
Copy link
Author

aars commented Jan 14, 2015

If accepted and agreed on sanity of behavior I wouldn't mind doing this for the mysql connector as well.

@aars
Copy link
Author

aars commented Jan 14, 2015

Running into issues now. Added WIP label.

@aars aars changed the title [Issue #50] Removes keys with undefined values. [WIP] [Issue #50] Removes keys with undefined values. Jan 14, 2015
@aars aars changed the title [WIP] [Issue #50] Removes keys with undefined values. [Issue #50] Removes keys with undefined values. Jan 14, 2015
@aars
Copy link
Author

aars commented Jan 14, 2015

Should be all fine now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please use plain for-loop to avoid performance penalties. The closure is heavy.

@raymondfeng
Copy link
Contributor

@aol-nnov
Copy link
Contributor

BTW, @raymondfeng, I suppose, this issue has something to do with #44 or #45.
It seems to me, this line https://github.com/strongloop/loopback-connector-postgresql/blob/master/lib/postgresql.js#L1512 is forcing such a behaviour.

If I'm right, may be it's better to fix the root cause instead of adding one more function?
May be, it would be better to remove item from props.nonIdsInData if data[key] is undefined instead of pushing null into queryParams.

//Sorry for the braindump. I need to study the code more to make my thoughts clear ;)

@aol-nnov
Copy link
Contributor

I went deeper, lol.

A one line fix for this issue is in my last commit for #54. Seems, this one is no longer needed. Please, verify.

aol-nnov added a commit to aol-nnov/loopback-connector-postgresql that referenced this pull request Jan 23, 2015
aol-nnov added a commit to aol-nnov/loopback-connector-postgresql that referenced this pull request Jan 23, 2015
raymondfeng pushed a commit that referenced this pull request Jan 23, 2015
1.4.0

 * Remove the usage of `CREATE SCHEMA IF NOT EXISTS' for compatibility (Raymond Feng)

 * one-line fix for #51 (Andrey Loukhnov)

 * basic tests for PR #53 (Andrey Loukhnov)

 * basic tests for PR #54 (Andrey Loukhnov)

 * provide database column default values via Loopback model description (Andrey Loukhnov)

 * autocreate schema if it does not exist during migration/update (Andrey Loukhnov)

 * Use connection pooling from the driver (Raymond Feng)
@ritch ritch added the waiting label Mar 2, 2015
@raymondfeng
Copy link
Contributor

@aol-nnov Do you think this PR is still relevant?

@aol-nnov
Copy link
Contributor

@raymondfeng I think, no. #54 supersedes it.

@0candy
Copy link
Contributor

0candy commented Oct 5, 2016

@aars Can this be closed in favor of #54?

@slnode
Copy link

slnode commented Oct 5, 2016

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@slnode
Copy link

slnode commented Oct 5, 2016

Can one of the admins verify this patch?

1 similar comment
@slnode
Copy link

slnode commented Oct 5, 2016

Can one of the admins verify this patch?

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.

9 participants