Skip to content

Quote the readable.id in Unread::Reader::Scopes #108

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Aug 25, 2019
Merged

Quote the readable.id in Unread::Reader::Scopes #108

merged 2 commits into from
Aug 25, 2019

Conversation

fotos
Copy link
Contributor

@fotos fotos commented Aug 3, 2019

This PR fixes the generated SQL when the readable.id is a UUID.

For example when running:

> user.have_read?(book)

the generated SQL is:

  Punter Exists (0.7ms)  SELECT  1 AS one FROM "people" LEFT JOIN "read_marks"
                ON "read_marks".readable_type  = 'Book'
               AND ("read_marks".readable_id   = a4691c66-344f-4ab1-8dd4-16614f6cbb52 OR "read_marks".readable_id IS NULL)
               AND "read_marks".reader_id      = "user"."id"
               AND "read_marks".reader_type    = 'User'
               AND "read_marks".timestamp     >= '2019-08-03 10:43:02.138041' WHERE "people"."type" IN ('User') AND ("read_marks".id IS NULL) AND "people"."id" = $1 LIMIT $2  [["id", "cf7144b0-2814-48d3-ba17-b46a00b46246"], ["LIMIT", 1]]

and it fails with the following error:

ActiveRecord::StatementInvalid: PG::SyntaxError: ERROR:  syntax error at or near "f"
LINE 3: ...    AND ("read_marks".readable_id   = a4691c66-344f-4ab1-8dd...
                                                             ^

Quoting the readable.id fixes the problem. Related to #60.

@coveralls
Copy link

coveralls commented Aug 3, 2019 •
edited
Loading

Coverage Status

Coverage remained the same at 100.0% when pulling 8fde098 on fotos:fix-readable-id-for-uuid into 161a609 on ledermann:master.

@fotos
Copy link
Contributor Author

fotos commented Aug 23, 2019

@ledermann is there anything else I need to change in this PR (e.g. changelog, tests)?

We'd love to have the fix merged / released upstream instead of forking the gem for this (small) bug.

Would appreciate some feedback. 🙏

@ledermann
Copy link
Owner

Sorry for the late answer. Before merging your changes I want to have the tests passing. I would like to ask you to rebase the PR against master, because I just made some Travis related fixes.

@fotos fotos force-pushed the fix-readable-id-for-uuid branch from 5d594d1 to 8fde098 Compare August 24, 2019 18:16
@fotos
Copy link
Contributor Author

fotos commented Aug 24, 2019

Great thanks, the tests are passing now! Let me know if there is anything else.

@ledermann ledermann merged commit b3e257b into ledermann:master Aug 25, 2019
@ledermann
Copy link
Owner

Merged, thanks for contribution!

@fotos fotos deleted the fix-readable-id-for-uuid branch August 25, 2019 16:34
HwakyoungLee pushed a commit to grepp/unread that referenced this pull request May 12, 2023
@fotos @HwakyoungLee
Quote the readable.id in Unread::Reader::Scopes (ledermann#108)
10fcf58 
* Quote the readable.id in Unread::Reader::Scopes

Fixes an SQL syntax error when the readable.id is a UUID.

* Add parantheses around quote_bound_value argument in Unread::Reader::Scopes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fotos @coveralls @ledermann