Skip to content

Harmless select in case of invalid insert attempt#203

Merged
brianc merged 1 commit into
brianc:masterfrom
paytm:insert_issue
Jan 19, 2015
Merged

Harmless select in case of invalid insert attempt#203
brianc merged 1 commit into
brianc:masterfrom
paytm:insert_issue

Conversation

@Hiteshm01
Copy link
Copy Markdown
Contributor

Regarding #197, As suggested by @spion This patch will override previous fix and will return with harmless select statement with "WHERE (1=2)"

@spion
Copy link
Copy Markdown
Contributor

spion commented Jan 16, 2015

Wow that was fast :D Looking great - I'll try it out later tonight

Comment thread lib/node/query.js
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removing fix placed in #197 which inserts default values.

@brianc
Copy link
Copy Markdown
Owner

brianc commented Jan 18, 2015

Off! Sorry for that break. Let me know if this looks good to y'all & I'll merge it & push out a new fix asap.

@brianc
Copy link
Copy Markdown
Owner

brianc commented Jan 18, 2015

Also - I'm fine with a breaking change if that will make the query do the right thing ™️

@spion
Copy link
Copy Markdown
Contributor

spion commented Jan 19, 2015

@Hiteshm01 @brianc this is fully backward compatible and fast now :D

@brianc I'm afraid that there is no right way. What would the correct behavior be for an insert query that is passed an empty list of objects? I would say its inserting nothing at all, which is what this patch does. Its not an insert query however - SQL doesn't support an insert query that inserts nothing at all, so thats not ideal. The closest is a query that inserts a row with default values... which doesn't really make sense to me for an empty array argument.

Which one do you think makes more sense?

@Hiteshm01
Copy link
Copy Markdown
Contributor Author

@spion , @brianc , I suggest to merge and close it as its definitely a better fix then previous one. I don't want someone to haunt me down for inserting default values in their production database :)

@brianc
Copy link
Copy Markdown
Owner

brianc commented Jan 19, 2015

@Hiteshm01 Agreed - I'll merge it & push out a new minor version right now containing this fix and this epic PR giving us support for mssql. I really appreciate you and @spion helping to shepherd this issue through and lend clarity to it.

brianc added a commit that referenced this pull request Jan 19, 2015
Harmless select in case of invalid insert attempt
@brianc brianc merged commit b26d805 into brianc:master Jan 19, 2015
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.

3 participants