Add WHERE escape hatch for job listing#933
Merged
Merged
Conversation
brandur
force-pushed
the
brandur-where-escape-hatch
branch
from
72bab88 to
02676ff
Compare
bgentry
approved these changes
May 28, 2025
bgentry
left a comment
bgentry
left a comment
Contributor
There was a problem hiding this comment.
This seems like a great all-purpose API to bridge the driver feature gap and to help avoid adding lots of specialized ones. My only concern is being explicit about the risk of SQL injection, but otherwise LGTM and ship it!
Comment on lines
+424
to
+446
| // Where is an all-encompassing query escape hatch that adds an arbitrary | ||
| // predicate after a list query's `WHERE ...` clause. Use of other JobListParams | ||
| // filters should be preferred where possible because they're safer and their | ||
| // compatibility between drivers is better guaranteed, but in case none is | ||
| // suitable, Where can be used as a last resort. | ||
| // | ||
| // For example, using Where to query with `jsonb_path_query_first(...)` using a | ||
| // JSON path, a function that's specific to Postgres: | ||
| // | ||
| // listParams = listParams.Where("jsonb_path_query_first(metadata, @json_path) = @json_val", NamedArgs{"json_path": "$.foo", "json_val": `"bar"`}) | ||
| // | ||
| // A JSON path can be used in a query in SQLite as well, but there the `->` or | ||
| // `->>` operators must be used instead: | ||
| // | ||
| // listParams = listParams.Where("metadata ->> @json_path = @json_val", NamedArgs{"json_path": "$.foo", "json_val": "bar"}) | ||
| // | ||
| // Arguments beyond the first are interpreted as named parameters. Each one | ||
| // should be present in the query SQL prefixed with a `@` symbol. Multiple sets | ||
| // of named parameters will be merged together, with values in later sets | ||
| // overwriting those in earlier ones. | ||
| // | ||
| // Calling Where multiple times will add multiple conditions separate by `AND`. | ||
| // Use `OR` instead by stuffing all conditions into a single Where invocation. |
Contributor
There was a problem hiding this comment.
Do we need a clear and explicit warning here about SQL injection and not directly building sql based on user input?
Contributor
Author
There was a problem hiding this comment.
Yeah, works for me. Added this paragraph to the doc string:
// Consider use of this function possibly hazardous! Any time raw SQL is in
// play, an application is opening itself up to SQL injection attacks. Never mix
// unsanitized user input into a SQL string, and use named parameters to curb
// the likelihood of injection.
02676ff to
7ca265e
Compare
#926) Here, resolve #907 by letting an explicit schema be injected into `rivertest.Require*` assertions in a similar way that one can be used in a client. This approach adds a schema in `RequireInsertedOpts`. This comment does a good job of highlight all the potential approaches for adding a schema [1], and unfortunately none of them are all that great. I implemented one other version of this (a variant of option 2 in that list), which as some advantages, but in the end it just ended up ballooning the API out to an uncomfortable degree. The worst part about adding schema to `RequireInsertedOpts` is its interact with the `RequireMany*` functions, where each expectation can set its own schema, and it's not clear what would happen if different expectations set different schemas. I resolved this ambiguity by making it an error to mix and match schemas. Assertions are allowed to send a schema in only the first position like: jobs := requireManyInserted(ctx, bundle.mockT, bundle.driver, []ExpectedJob{ {Args: &Job1Args{String: "foo"}, Opts: bundle.schemaOpts}, {Args: &Job1Args{String: "bar"}}, }) Or send the same schema in all positions: jobs := requireManyInserted(ctx, bundle.mockT, bundle.driver, []ExpectedJob{ {Args: &Job1Args{String: "foo"}, Opts: bundle.schemaOpts}, {Args: &Job1Args{String: "bar"}, Opts: bundle.schemaOpts}, }) But they aren't allowed to set a schema only in position other than the first, or mix and match schemas between expectations. Fixes #907. [1] #907 (comment)
brandur
force-pushed
the
brandur-where-escape-hatch
branch
from
7ca265e to
fd0f925
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Alright, so this one's generally in pursuit of resolving this issue [1].
I added SQLite support a few weeks back, but a caveat of that is that
the
Metadatafilter on job listing is not usable because it dependedon an operator that was highly specific to Postgres. Someone turned out
to be using it, and I had nothing to suggest beyond not using SQLite
because there's no workaround.
Here, I propose we bring in an escape hatch for job listing that lets a
user add an arbitrary predicate in cases where no other APIs will get
the job done. For example, querying Postgres with a JSON path comparison:
Or performing the same action on SQLite instead:
All of Postgres, SQLite, and MySQL support JSON path in some form, so my
first instinct was to add a new job list filter like
MetadataPathEq,but after writing it up I found that what I'd added was so specific to
allow exactly one possible use case. JSON Path is very expressive, and
we'd undoubtedly get requests to reveal more of it, and so I think the
best approach is to give people a way to use any JSON Path expression
and comparison they want.
The trouble with the new
Whereis that it's not database agnostic, butI think the use of raw SQL written by the user helps make it clear that
perfect compatibility is obviously not guaranteed. It's also less safe
(users could refuse to use named args and open themselves up to SQL
injection more easily), so to handle that we recommend they prefer other
filters first and only drop down to
Wherewhen absolutely necessary.[1] #923 (comment)