-
Notifications
You must be signed in to change notification settings - Fork 93
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
More advanced querying on JobListParams.Metadata() #570
Comments
This is roughly where I'm at now too. It's kind of a nice idea to not allow a run-any-SQL-you-want because it's safer from a footgun perspective, but at the end of the day if want to make the list API feature rich enough to do anything, we probably have to. Halfway measures will make the corner cases ever smaller, but they'll always still exist to an extent. @bgentry Any thoughts on that? @arp242 Depending on what stack you're using, it also might not be the worst-ever-idea to skip River's built-in job list API and just use your data access layer (e.g. sqlc) directly. Then you have as much power as you want and will never got boxed in. |
Yeah, that's pretty much what I did for the time being. It's not hard and works well enough. In some ways this is better because it's consistent with the rest of my code. My main worry is stability: new versions of River can change the table layout and break my code. This can be easy to miss, because it won't be a compile error. River is still pre-1.0, so all bets are off. I also hit another problem with Implementing a full-on query builder is probably out of scope. Or at least, I'd consider that out of scope. "A slightly more advanced way to query metadata" is still okay, but "select which columns to return" is getting too much. But like I said, compatibility is my main concern here. |
I wouldn't worry too much about this. I can't guarantee that the schema never changes, but it's not super likely to see significant changes going forward. River's main feature set is largely "done", and migrations can be a major source of pain for large tables, so we try to keep their invasiveness minimal. Also, as long as you've got reasonable test coverage, you should be able to detect any problems in CI before an upgrade. |
Right now
JobListParams.Metadata()
only offers querying viametadata >@ …
, but I'd like to do slightly more advanced stuff. For example if the metadata contains:And I'd like to list all jobs that match any of a set:
I don't mind writing a patch for this; the simplest would be to just allow appending to the condition list and argument map, via the patch below. Although arguably that would give too much control (and the ability to do very stupid things).
Then again, restricting this would probably more trouble than its worth(?) The alternative is writing my own query from scratch, which allows for even more stupid stuff.
Well, let me know what you want and I'll submit a working patch with docs, tests, etc.
The text was updated successfully, but these errors were encountered: