Bug #13006

is_a filter doesn't work with remote objects

Added by Lucas Di Pentima 11 months ago. Updated about 7 hours ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
API
Target version:
Start date:
12/13/2018
Due date:
% Done:

100%

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

Description

While working on #12758 I found that the sync-groups tool was failing to retrieve group membership links from remote (federated) user accounts.
Seems that we need to modify the API server to make a substring query on tail_uuid & head_uuid fields, so that the uuid prefix is not fixed.


Subtasks

Task #14610: Review 13006-api-is_a-filter & 13006-sync-groups-is_a-filterResolvedLucas Di Pentima

Associated revisions

Revision 55c142e4
Added by Lucas Di Pentima about 7 hours ago

Merge branch '13006-api-is_a-filter'
Refs #13006

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <>

Revision cd4bb0b5
Added by Lucas Di Pentima about 7 hours ago

Merge branch '13006-sync-groups-is_a-filter'
Closes #13006

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <>

History

#2 Updated by Peter Amstutz 11 months ago

This is implemented in arvados/services/api/lib

          when 'is_a'
            operand = [operand] unless operand.is_a? Array
            cond = []
            operand.each do |op|
              cl = ArvadosModel::kind_class op
              if cl
                cond << "#{ar_table_name}.#{attr} like ?" 
                param_out << cl.uuid_like_pattern
              else
                cond << "1=0" 
              end
            end
            cond_out << cond.join(' OR ')

Instead of using 'like' this should use a substring match:

            operand.each do |op|
              cl = ArvadosModel::kind_class op
              if cl
                cond << "substring(#{ar_table_name}.#{attr}, 6, 5) = ?" 
                param_out << cl.uuid_prefix
              else
                cond << "1=0" 
              end
            end

If 'attr' is 'uuid' I think you can even short circuit the check with "model_class.uuid_prefix == cl.uuid_prefix" (we can assume every value in the uuid column has the same type.)

To ensure the substring match is efficient, we may need to create an expression index:

https://www.postgresql.org/docs/9.4/static/indexes-expressional.html

#3 Updated by Lucas Di Pentima 11 months ago

  • Assigned To deleted (Lucas Di Pentima)

#4 Updated by Tom Morris 11 months ago

  • Target version set to To Be Groomed

#5 Updated by Tom Morris 10 months ago

  • Story points set to 1.0

#6 Updated by Tom Morris 8 months ago

  • Target version changed from To Be Groomed to Arvados Future Sprints

#7 Updated by Tom Morris 2 days ago

  • Target version changed from Arvados Future Sprints to 2018-12-21 Sprint

#8 Updated by Lucas Di Pentima 2 days ago

  • Status changed from New to In Progress

#9 Updated by Lucas Di Pentima 1 day ago

  • Assigned To set to Lucas Di Pentima

#10 Updated by Lucas Di Pentima 1 day ago

Updates at cd7a746df - branch 13006-api-is_a-filter
Test run: https://ci.curoverse.com/job/developer-run-tests/1009/

  • Followed Peter's suggestion by changing the query to use substring(), avoiding the check when attr is "uuid" and prefix_uuid matches.
  • Added expression index on links.[tail|head]_uuid
  • Added test

I also could update the group-sync tool to use the is_a operator again, for enhanced performance.

#11 Updated by Peter Amstutz 1 day ago

Lucas Di Pentima wrote:

Updates at cd7a746df - branch 13006-api-is_a-filter
Test run: https://ci.curoverse.com/job/developer-run-tests/1009/

  • Followed Peter's suggestion by changing the query to use substring(), avoiding the check when attr is "uuid" and prefix_uuid matches.

If attr == 'uuid' but model_class.uuid_prefix != cl.uuid_prefix, should it fail unconditionally?

  • Added expression index on links.[tail|head]_uuid
  • Added test

I also could update the group-sync tool to use the is_a operator again, for enhanced performance.

How about in a separate branch.

#12 Updated by Lucas Di Pentima 1 day ago

Peter Amstutz wrote:

If attr == 'uuid' but model_class.uuid_prefix != cl.uuid_prefix, should it fail unconditionally?

I believe so, fixed.

Update at 96165e8fc
Test run: https://ci.curoverse.com/job/developer-run-tests/1011/

#13 Updated by Lucas Di Pentima 1 day ago

Peter Amstutz wrote:

I also could update the group-sync tool to use the is_a operator again, for enhanced performance.

How about in a separate branch.

Done: 84dcca64f - branch 13006-sync-groups-is_a-filter
Test run: https://ci.curoverse.com/job/developer-run-tests/1012/ (not yet started atm)

#14 Updated by Peter Amstutz about 8 hours ago

Lucas Di Pentima wrote:

Peter Amstutz wrote:

I also could update the group-sync tool to use the is_a operator again, for enhanced performance.

How about in a separate branch.

Done: 84dcca64f - branch 13006-sync-groups-is_a-filter
Test run: https://ci.curoverse.com/job/developer-run-tests/1012/ (not yet started atm)

Is there a 'sync-groups' test that fails without the '13006-api-is_a-filter' fix but passes with it?

#15 Updated by Lucas Di Pentima about 8 hours ago

Yes. Although I didn't tested it now, but it was the means to detect this is_a bug in the API server. The test is TestMembershipRemoval, if you want I can rebase this branch to master and then merge the other 13006 branch to prove that the test passes.

#16 Updated by Peter Amstutz about 7 hours ago

Lucas Di Pentima wrote:

Yes. Although I didn't tested it now, but it was the means to detect this is_a bug in the API server. The test is TestMembershipRemoval, if you want I can rebase this branch to master and then merge the other 13006 branch to prove that the test passes.

Yes, I think that would be a good idea. Go ahead and merge both branches if it goes as expected (test fails without fix → test passes with fix)

#17 Updated by Lucas Di Pentima about 7 hours ago

Ok, so I rebased 13006-sync-groups-is_a-filter (0c21d2bd2d0fd351a4e546002a249b0b748061eb) to master. Ran the sync-groups tests locally and got the failure:

FAIL: sync-groups_test.go:267: TestSuite.TestMembershipRemoval

sync-groups_test.go:308:
    c.Assert(GroupMembershipExists(s.cfg.Client, remoteUserUUID, groupUUID), Equals, false)
... obtained bool = true
... expected bool = false

Then, merged 13006-api-is_a-filter into 13006-sync-groups-is_a-filter (cbf93e8d897448dbd52369afe89fef2392140ff1). Tests now pass.

#18 Updated by Lucas Di Pentima about 7 hours ago

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

Also available in: Atom PDF