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

Simplify fk lookup #353

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

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.

None yet

2 participants