Skip to content

Conversation

@gjtorikian
Copy link

Hiya @magnusvk 👋 Thanks for this great gem.

In my project, my Postgres database uses binary ULIDs—they deserialize as strings, but are stored as binary data.

I noticed that my foreign key counters stopped working, and I tracked it down to the one line change in this PR. There are a couple of problems with the "#{klass.table_name}.#{relation_primary_key(first, source: obj, was: was)} = ?" format, the biggest being that the value is not quoted, which can cause security problems. Instead, we can lean heavily into where figuring out all the details for communicating with the database.

While grepping around, I found two other places (1, 2) where this exact pattern is used, rather than #{string} = ?. That plus the passing tests gives me confidence that this is a safe change to make.

@gjtorikian
Copy link
Author

@magnusvk Is this project still maintained?

@magnusvk
Copy link
Owner

magnusvk commented Oct 12, 2022

👋🏻 sorry for the radio silence, Github notifications let me down.

I agree that this change should be fine. In fact, though, I believe it doesn't change behavior at all. I disagree with your assertion that values aren't quoted, where("something = ?", some_value) does quote the value if required. I don't mind merging this, per se, but if this is changing behavior then I'd love to see a failing test case first. Do you think that's feasible to provide?

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.

2 participants