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

Why doesn't "update ... where column = ?" take all of a sudden? #525

Closed
forthrin opened this issue Apr 18, 2024 · 14 comments
Closed

Why doesn't "update ... where column = ?" take all of a sudden? #525

forthrin opened this issue Apr 18, 2024 · 14 comments

Comments

@forthrin
Copy link

require 'sqlite3'

db = SQLite3::Database.open("#{Dir.home}/foo.db")
db.results_as_hash = true
db.query('drop table if exists foo')
db.query('create table foo (key, value)')
db.query('insert into foo values (1, ?)', 'NO')
db.query('update foo set value = ? where key = 1', 'MAYBE')
db.query('update foo set value = ? where key = ?', 'YES', 1)
pp db.query('select * from foo').to_a
[{"key"=>1, "value"=>"MAYBE"}]
$ ruby --version
ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [arm64-darwin23]
$ gem info sqlite3
sqlite3 (2.0.0, 1.7.3)
$ sqlite3 --version
3.43.2 2023-10-10 13:08:14 1b37c146ee9ebb7acd0160c0ab1fd11017a419fa8a3187386ed8cb32b709aapl (64-bit)
@forthrin
Copy link
Author

@flavorjones: gem uninstall sqlite3 -v 2.0.0 (leaving 1.7.3 in place) resolves the issue.

So some commit between the two introduces the (major) issue. git log --pretty='%as %s' v1.7.3..v2.0.0

How does one require a local Git repository? Wanted to bisect, but got "couldn't find sqlite3_native".

@flavorjones
Copy link
Member

@forthrin Thanks for opening this issue. I'm looking into it now.

@flavorjones
Copy link
Member

@forthrin OK, as noted in the CHANGELOG for 2.0.0, we removed some deprecated APIs, specifically non-array binding parameters.

If you run this script with sqlite3 v1.7.3 with warnings enabled, you'll see:

./issues/525-update-where.rb:16:in `<main>' is calling `SQLite3::Database#query` with nil or multiple bind params without using an array.  Please switch to passing bind parameters as an array. Support for this will be removed in version 2.0.0.
[{"key"=>1, "value"=>"YES"}]

So if you update the script to wrap the binding parameters in an array:

db.query('update foo set value = ? where key = ?', ['YES', 1])

then you'll see the warning go away, and this code will work with both old and new versions of sqlite3.

@flavorjones flavorjones closed this as not planned Won't fix, can't repro, duplicate, stale Apr 19, 2024
@flavorjones
Copy link
Member

I'm worried that the gem isn't giving a better warning when this happens, so I'm looking into raising an exception if the right number of bind parameters isn't passed.

@forthrin
Copy link
Author

Peanuts

Oh why? Just like Brian Kernighan suddenly deprecated printf for vprintf.

What is the underlying reason? (Link to initial discussion / issue)

Never saw the deprecation notice, as it apparently doesn't display by default.

Wouldn't be surprised if this breaks a lot of systems out there.

@flavorjones
Copy link
Member

@forthrin Original commits deprecating this are from 2010:

and the reason appears to be compatibility with the sequel gem. Maybe @tenderlove remembers the context.

@forthrin
Copy link
Author

Were it only way to ensure some aspect of security, one would have to concede, but ... compatibility with another gem?

Brackets in this context seems superfluous and not in the pristine spirit of Ruby. It just looks and feels bad.

What is the real cost and consequence of reverting the change?

@flavorjones
Copy link
Member

I'm sorry that this surprised you, and I understand that you disagree with the API decision. Further, I acknowledge that this code should be raising an exception and is not -- see #527 for a proposed fix for that.

@tenderlove and I are going to talk about whether to revert this or not.

But: this is a major release. The CHANGELOG stated what changed and provided advice on how to upgrade. This change has been waiting fourteen years for a major release so that we would have the opportunity to finish it. We may choose not to revert and I want you to be prepared for that.

@tenderlove
Copy link
Member

I'd prefer we don't have a *args in the method definition because I want the method to be strict about the parameters it takes. IOW I'd prefer the internals not need to try to figure out what the caller "meant". The previous API allowed users to call like this:

db.query('update foo set value = ? where key = ?', 'YES', 1)

Or like this:

db.query('update foo set value = ? where key = ?', ['YES', 1])

I would like there to be only one way to use a particular method.

@forthrin
Copy link
Author

forthrin commented Apr 20, 2024

Granted, people (with sufficient verbosity settings) have had eight Taylor Swift albums worth of time to prepare.

@tenderlove: Can you share:

  1. How flat variables creates ambiguity (ie. "what the caller meant")?
  2. Pros/cons that went into the choice between Array Vs. Variables?

"What Would Matz Do?"

@flavorjones
Copy link
Member

v2.0.1 will raise an exception in these cases.

@forthrin
Copy link
Author

@tenderlove: Would really appreciate your list of underlying considerations that necessitated this change.

@tenderlove
Copy link
Member

@forthrin sorry, I don't feel I owe you anything more than what I've commented. As I said "I would like there to be only one way to use a particular method." This has been deprecated for years, and this is a major version bump. If you don't want to change your code, don't upgrade.

@forthrin
Copy link
Author

forthrin commented May 2, 2024

It is simply unclear what specific ambiguity you are implying (ie. "what the caller meant"), when no code examples are given to illustrate this. Your reluctance to give a friendly and simple technical explanation is duly noted. I'll stay on upgrades but keep a reversion of this particular change to keep my code clean and Ruby-like. EOF

@sparklemotion sparklemotion locked as resolved and limited conversation to collaborators Aug 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants