-
Notifications
You must be signed in to change notification settings - Fork 96
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
Enrich jobs endpoint with filtering functionality #26
Enrich jobs endpoint with filtering functionality #26
Conversation
jobViews = apiRequest.getJobViews(); | ||
} else { | ||
jobViews = apiRequest.getFilteredJobViews(apiRequest.buildJobStoreFilter(filters)); | ||
} |
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.
Now that I look at this...I am not sure if it's better to do this here or to call a generic getJobViews(filters) and have JobsApiRequest perform this check and call the correct function accordingly
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.
I think it would be better if we did what the DataServlet
does: pass in the filters to the JobsApiRequest
and let it parse the filters internally. Then, when getJobViews()
is invoked, the JobsApiRequest
applies the filters if present. So, in the DataServlet
, the code would look like:
JobsApiRequest apiRequest = new JobsApiRequest (
format,
null, //asyncAfter is null so it behaves like a synchronous request
perPage,
page,
filters, //Note the addition of the filters to the constructor
uriInfo,
jobPayloadBuilder,
apiJobStore,
preResponseStore,
broadcastChannel
);
... //Other stuff
jobViews = apiRequest.getJobViews(); //Inside JobsApiRequest::getJobViews, JobsApiRequest::getFilteredJobViews is invoked if there are filters
@dayamr @archolewa @cdeszaq Please review |
*/ | ||
public Observable<Map<String, String>> getFilteredJobViews(Set<ApiJobStoreFilter> apiJobStoreFilters) { | ||
return apiJobStore.getFilteredRows(apiJobStoreFilters) | ||
.map(this::mapJobRowsToJobViews); |
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.
Can be unwrapped.
Finished my first round. Overall it looks pretty good, though the testing feels a bit sparse. It would be nice if we could have some more tests involving different operations than just |
@archolewa I will add the tests for the filters in #23 as that is where the code was added |
@archolewa Added test for multiple filter query with different operators: a93cb6c Added test for ensuring we return empty list when no filters are satisfied: d01a771 We already have tests that the different filter operators select the correct JobRows: |
String result = makeRequest("/jobs", [filters : ["userId-eq[pikachu]"]]) | ||
|
||
then: "We only get the job payload that satisfies the filter" | ||
GroovyTestUtils.compareJson(result, "{\"jobs\":[]}", JsonSortStrategy.SORT_BOTH) |
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.
If you use single quotes to delimitate the string, you don't have to escape the double quotes around jobs
.
One small comment. 👍 once it's been addressed. |
Observable<Map<String, String>> jobViews; | ||
|
||
if (filters == null || "".equals(filters)) { | ||
jobViews = apiRequest.getJobViews(); |
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.
If you give filters a default value in the parameter list for this method, then you wouldn't have to test the null case.
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.
Sorry to be late bringing this up, but in the other ApiRequest objects we've avoided using the 'get' verb when the method is essentially a factory. Since the ApiRequest continues to be both a data object and a factory, it would be completely obscure whether 'getFoo()' was retrieving a built foo or an accessor for an already built foo.
This applies to getJobViews, but not by any means to just getJobViews in this code now.
d01a771
to
baf6b86
Compare
@michael-mclawhorn This has been rebased on top of master after merging #23 |
* Returns an Observable containing a stream of job payloads for all the jobs in the ApiJobStore. If, for any | ||
* JobRow, the mapping from JobRow to job view fails, an Observable over JobRequestFailedException is returned. If | ||
* the ApiJobStore is empty, we return an empty Observable. | ||
|
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.
Space not needed
String result = makeRequest("/jobs", [filters : ["userId-eq[momo]"]]) | ||
|
||
then: "We only get the job payload that satisfies the filter" | ||
GroovyTestUtils.compareJson(result, expectedResponse, JsonSortStrategy.SORT_BOTH) |
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.
Sorting on Map (the default I think) should be sufficient, right?
👍 after those last couple of small tweaks |
2f3e8f2
to
de1de63
Compare
This PR adds filtering capability to Job endpoint.
The default job payload also contains userId information
Please note: This PR is based on #23
Please ignore the first commit