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

Support custom queries in builder #403

Merged
merged 10 commits into from
May 27, 2020
Merged

Support custom queries in builder #403

merged 10 commits into from
May 27, 2020

Conversation

Serdnad
Copy link
Contributor

@Serdnad Serdnad commented May 21, 2020

This is a first attempt at addressing #402 , and implements support for building simple queries through the use of handwritten SQL like so:

users = Users.where("age > ?", 18) # == where(:age, :gt, 18)

with the intention of providing a way to support more sophisticated queries, such as

users = User.where("LOWER(name) = ?", name)

and even database specific features, like checking for membership in an array using Postgres:

users = User.where("? = ANY(trusted)", id)

The current implementation only allows one argument per query, but it should be rather straightforward to extend this to allow multiple args per query, if that's desired.

A simple spec is included for each DB adapter.

Note: I've opened this PR as a draft because I also intend to include an addition to the docs, but wanted to make this available for review as early as possible.

@Serdnad
Copy link
Contributor Author

Serdnad commented May 22, 2020

Tweaked the implementation to allow for a nil value, to support running a raw SQL string with no parameter substitutions.

@Serdnad Serdnad marked this pull request as ready for review May 22, 2020 02:53
docs/querying.md Outdated Show resolved Hide resolved
docs/querying.md Outdated Show resolved Hide resolved
@Serdnad
Copy link
Contributor Author

Serdnad commented May 22, 2020

Added specs for #where, #or, and #and as requested, and updated Postgres to use $ as a placeholder instead of ?.

Copy link
Member

@drujensen drujensen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. I am wondering if this supports an array as a parameter where("id in ?", [1, 2, 3])

@Blacksmoke16
Copy link
Contributor

@Serdnad Does your @placeholder stuff work? AFAIK $ on its own is not a valid PG placeholder. It would need the index value to work correctly.

@Serdnad
Copy link
Contributor Author

Serdnad commented May 22, 2020

@Blacksmoke16 This placeholder just tells the assembler that there's a placeholder in the query, so that the assembler can then replace this with the proper, final placeholder, such as $1 in the case of Postgres. This way, you can chain several where queries without having to manually keep track of the index. This is also why my previous implementation worked even when ? was used with PG, even though PG doesn't actually use ?, because the assembler would then replace it with the correct placeholder.

@drujensen That's a good point. I just checked, and it seems like the array would have to be formatted as (1, 2, 3) for that to work. Would it make sense to treat this as a separate issue, or do you think that should be included before merging?

Edit: Actually, I'm not so sure about that last point. I'll do some testing.

@drujensen
Copy link
Member

@Serdnad It can be treated as a separate issue. If @Blacksmoke16 approves, we can merge.

@robacarp
Copy link
Member

@Serdnad this looks great!

@Serdnad
Copy link
Contributor Author

Serdnad commented May 22, 2020

Thanks @robacarp ! This turned out very differently than what I was initially looking for, but definitely for the better. Huge thanks for all the input and feedback from all of you :)

@drujensen drujensen requested a review from Blacksmoke16 May 26, 2020 23:39
@drujensen drujensen merged commit b225462 into amberframework:master May 27, 2020
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.

4 participants