Bug #3692

[API] Eventbus filters parameter should use implicit AND (like REST filters) instead of implicit OR.

Added by Tom Clegg almost 5 years ago. Updated almost 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
API
Target version:
Start date:
10/10/2014
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
0.5

Subtasks

Task #4179: Review 3692-event-bus-fix-andResolvedPeter Amstutz

Task #4176: Write testResolvedPeter Amstutz

Task #4177: Fix itResolvedPeter Amstutz


Related issues

Related to Arvados - Story #3149: [Workbench] Workbench infinite scroll and filter system should do filtering on server side instead of client side.Resolved08/24/2014

Associated revisions

Revision 59331619
Added by Peter Amstutz almost 5 years ago

Merge branch '3692-event-bus-fix-and' closes #3692

Revision 0474aa89
Added by Peter Amstutz almost 5 years ago

Merge branch '3692-event-bus-fix-and' closes #3692

History

#1 Updated by Peter Amstutz almost 5 years ago

  • Target version set to 2014-10-29 sprint

#2 Updated by Peter Amstutz almost 5 years ago

  • Category set to API
  • Status changed from New to In Progress
  • Assigned To set to Peter Amstutz

#3 Updated by Brett Smith almost 5 years ago

Reviewing c088fcf

I am consistently getting UUID mismatches in the new test:

  1) Failure:
WebsocketTest#test_connect,_subscribe,_multiple_filters [/home/brett/repos/arvados/services/api/test/integration/websocket_test.rb:240]:
<"zzzzz-7a9it-hwe3vpcyrk8iv3j"> expected but was
<"zzzzz-j58dm-l6egg61nr8lxalp">.

302 tests, 997 assertions, 1 failures, 0 errors, 0 skips

The last part of the UUIDs are random, but the second part of the mismatch (7a9it expected vs. j58dm) is consistent.

Code-wise, my only concern is that #{cond_out.join ') OR ('} is dead code that could potentially confuse a reader, now that the conditions are already joined earlier. I think you could clear this up by rolling back the changes to the ws.filters.each block, and then changing OR to AND here. But I'd be happy with any change that only has one join on cond_out rather than two.

Thanks.

#4 Updated by Peter Amstutz almost 5 years ago

Brett Smith wrote:

Reviewing c088fcf

I am consistently getting UUID mismatches in the new test:

[...]

The last part of the UUIDs are random, but the second part of the mismatch (7a9it expected vs. j58dm) is consistent.

Fixed.

Code-wise, my only concern is that #{cond_out.join ') OR ('} is dead code that could potentially confuse a reader, now that the conditions are already joined earlier. I think you could clear this up by rolling back the changes to the ws.filters.each block, and then changing OR to AND here. But I'd be happy with any change that only has one join on cond_out rather than two.

I think you're misunderstanding the purpose of the bug fix here. The clauses within a single filter are joined with AND, but if there are multiple subscriptions, the compound filter statements are joined with OR. Added a a couple comments to try and clarify.

#5 Updated by Brett Smith almost 5 years ago

Peter Amstutz wrote:

Brett Smith wrote:

Code-wise, my only concern is that #{cond_out.join ') OR ('} is dead code that could potentially confuse a reader, now that the conditions are already joined earlier. I think you could clear this up by rolling back the changes to the ws.filters.each block, and then changing OR to AND here. But I'd be happy with any change that only has one join on cond_out rather than two.

I think you're misunderstanding the purpose of the bug fix here. The clauses within a single filter are joined with AND, but if there are multiple subscriptions, the compound filter statements are joined with OR. Added a a couple comments to try and clarify.

Okay, I get it now, thanks for explaining. b8fdb90 is good to merge, thanks.

#6 Updated by Anonymous almost 5 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 67 to 100

Applied in changeset arvados|commit:593316198c1647b24f66a64e4a0f7a34e75b54ab.

Also available in: Atom PDF