Bugfix user filters #289
Bugfix user filters #289
Conversation
…race/config-service into filtering-pagination-support-objectstore
…ering-pagination-support-objectstore
…race/config-service into total-attribute-configservice-api
| } | ||
|
|
||
| private Query buildQuery( | ||
| Query buildQuery( |
There was a problem hiding this comment.
nit - don't expose a method just to test it. Test it through the public interface.
There was a problem hiding this comment.
It's bit involved to write cases with public interface, can take it up as next PR.
There was a problem hiding this comment.
Next PR is fine.
As far as what to change, one option would be to extract a query builder class, in which case testing that makes sense in a unit test (while tests on the store would start mocking it). But an even easier option... this test could just call getAllConfigs instead - it passes the same 4 arguments through to the query builder directly. Then just verify the call to collection.query.
There was a problem hiding this comment.
Thanks, since i need to wait for one more day to get it merged, i thought of doing in next PR. Anyway i have addressed all the comments. We should also take decision to remove sorting on ConfigVersion, i don't its needed any more.
config-service-impl/src/main/java/org/hypertrace/config/service/store/DocumentConfigStore.java
Outdated
Show resolved
Hide resolved
|
|
||
| Query query = configStore.buildQuery(configResource, filter, pagination, sortByList); | ||
|
|
||
| assertEquals(2, query.getSorts().size()); |
There was a problem hiding this comment.
nit - compare the whole list rather than each element individually.
There was a problem hiding this comment.
this change will also removes the need to compare the size and applies to each test, both of which we can update here (separate PR is fine if you're following up anyway, no functional impact). The main benefit is that failures show up as a list diff (e.g. "expected [x, y], got []") rather than a more opaque failure like "expected 2, got 0"
aaron-steinfeld
left a comment
aaron-steinfeld
left a comment
There was a problem hiding this comment.
Disabled auto merge (so you can review the comments before deciding whether to merge - please do not use automerge until code has been approved), but if you want to take the comments in a follow up that's fine.
| configStore.getAllConfigs(configResource, filter, pagination, sortByList); | ||
|
|
||
| // Capture the Query object passed to collection.query | ||
| ArgumentCaptor<Query> queryCaptor = ArgumentCaptor.forClass(Query.class); |
There was a problem hiding this comment.
arg captors aren't needed here (they're almost always overkill). Let me post a branch to illustrate what I was suggesting
There was a problem hiding this comment.
If that looks good to you, feel free to merge it into your branch (or just take bits and pieces)
There was a problem hiding this comment.
I have merged your PR. Thanks for that.
Description
Fixed the bug around user filters in document config store