-
Notifications
You must be signed in to change notification settings - Fork 21
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
Implement raw subqueries #230
Conversation
9892022
to
b8e9b14
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for starting this! 🙏
This looks good and I think we might need to refactor a few things in order to be able to merge this (especially in order to avoid some duplications). I will definitely need to do another review. 🙂
@ellmetha I'll left two comments regarding the connecting object (which is related to the formatting issue), which is why I didn't touch any of the connection findings |
it "raises an error when filtering with a misspelled column in a raw SQL condition", tags: "raw" do | ||
Tag.create!(name: "crystal", is_active: true) | ||
|
||
expect_raises(Exception) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we target a more specific exception class here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could, but I'm not sure it's useful here. All database types throw their own exception. I didn't see a common ancestor of these database exceptions, so I chose this generic exception type instead of checking each individual database exception.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could define specific blocks for every supported backend and verify that the most precise exception is raised (by wrapping #expect_raises
calls within #for_mysql
/ #for_postgresql
/ #for_sqlite
blocks). That way, other types of exceptions wouldn't be "caught" if there is an issue with the underlying code, but that's very unlikely anyway. Let's keep it that way!
Thanks for the work on this @treagod! We are getting closer! I left additional comments and small fix suggestions; we should be able to merge this very soon. 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this!
This commit adds support for raw SQL queries to the
#filter
method and q expressions.#44