Query job ids #434
Query job ids #434
Conversation
This lets you use an API query like: api/v1/jobs?ids=5062,5061,5060 I noticed while working on this that in query_jobs, 'state' and 'ids' are handled inconsistently, and this seemed messy. So I figured I'd clean that up too. To avoid breaking any current usage, you can now pass *either* an array ref *or* a comma- separated string for each.
For 'state' and 'ids', this allows this style: /jobs?ids=5062&ids=5061&ids=5060 as well as the comma-separated list style: /jobs?ids=5062,5061,5060 the former is fairly common practice, AIUI, so it seems a good idea to allow it.
AdamWill
force-pushed
the
query-job-ids
branch
from
d8817b1
to
0385dee
Compare
| for my $arg (qw/state ids/) { | ||
| next unless defined $self->param($arg); | ||
| if (index($self->param($arg), ',') != -1) { | ||
| $args{$arg} = [split(',', $self->param($arg))]; |
There was a problem hiding this comment.
Why are you doing split also here? I would like to keep it in one place in case we change our minds in the future.
There was a problem hiding this comment.
my thought was that it's clearer if this function is known to always pass an array ref. If we don't split here, it sometimes passes an array ref (if the parameter is specified more than once), otherwise it passes a string. That seems messy.
If I was writing it from scratch I'd have query_jobs only accept array refs, but I figured it'd be a bit rude to change that now (plus we'd have to change a boatload of current usage of 'state' elsewhere in the code).
Oh, also, note you can't just simply pass in $self->every_param($arg) unconditionally, because that always creates an array ref, even if the param is only present one time - so if the query string has a comma-separated list, query_jobs gets an array ref with a single member that is a comma-separated string, and it doesn't know what to do with that. If we want to accept both comma-separated and multiple-instances styles, we have to have at least some kind of logic here to cover both cases, I don't think there's a simple one-size-fits-all option.
|
for the sake of changing minds, it would be good to have test cases btw |
|
sigh, because if there's one thing more fun than writing perl it's writing test cases in perl...fiiiine. I'll try. |
AdamWill
force-pushed
the
query-job-ids
branch
4 times, most recently
from
355a5cc
to
2fff2bb
Compare
This goes along with the previous two commits, it adds some tests to make sure both array ref and comma-list searches work as expected.
AdamWill
force-pushed
the
query-job-ids
branch
from
2fff2bb
to
5efb66b
Compare
The main thing I wanted to do here was allow for specifying job IDs when searching. Sometimes we just want to query some specific set of jobs; this lets us do it with one query instead of running a single query for each job.
While I was working on it I noticed there's kind of an inconsistency in how
query_jobs()handles args which can be lists of values (idsandstate, for now). Forstateit expects a comma-separated list, but foridsit expects (I think) an array reference. I thought it would be a good idea to clean that up a bit: the safest thing to do seemed to be to handle both of the styles for both cases (and make it so others can be added if wanted). With this, comma-separated lists get turned into array refs right at the start, everything further down can assume it's getting an array ref. (Single-value strings will become single-member arrays).NOTE: I'm not totally sure if this
ref($args{$arg}) eq 'ARRAY'test is the best, let me know if anyone knows of any pitfalls.While I was working on this it made me curious about what the conventions for URL querying lists of values actually are, and it seems like one of the standard ways to do it is to allow the same query name to be passed multiple times -
?ids=1&ids=2&ids=3.... So I thought it might be a good idea to support that; the second commit adds it. Without the second commit, you have to use the comma-separated style when doing queries via the web API.