Skip to content

[Rails5] Fix `User#manageable_groups`

blackst0ne requested to merge blackst0ne-rails5-update-user-manageable-groups into master

What does this MR do?

In arel 7.0 (7.1.4 version is used for rails5) there were introduced some changes that break our code in the User#manageable_groups method.

The problem is that

arel_table[:id].in(Arel::Nodes::SqlLiteral)

generates wrong IN () construction.
The selection for IN is missing:

=> "\"namespaces\".\"id\" IN (0)"

That caused such spec errors for the rails5 branch:

4) User groups with child groups #manageable_groups does not include duplicates if a membership was added for the subgroup
    Failure/Error: expect(user.manageable_groups).to contain_exactly(group, subgroup)

      expected collection contained:  [#<Group id:232 @GROUP29>, #<Group id:234 @group29/group30>]
      actual collection contained:    []
      the missing elements were:      [#<Group id:232 @GROUP29>, #<Group id:234 @group29/group30>]
    # ./spec/models/user_spec.rb:699:in `block (5 levels) in <top (required)>'
    # ./spec/spec_helper.rb:188:in `block (2 levels) in <top (required)>'
    # /var/lib/gems/2.3.0/gems/rspec-retry-0.4.6/lib/rspec/retry.rb:112:in `block in run'
    # /var/lib/gems/2.3.0/gems/rspec-retry-0.4.6/lib/rspec/retry.rb:101:in `loop'
    # /var/lib/gems/2.3.0/gems/rspec-retry-0.4.6/lib/rspec/retry.rb:101:in `run'
    # /var/lib/gems/2.3.0/gems/rspec-retry-0.4.6/lib/rspec_ext/rspec_ext.rb:12:in `run_with_retry'
    # /var/lib/gems/2.3.0/gems/rspec-retry-0.4.6/lib/rspec/retry.rb:30:in `block (2 levels) in setup'

This MR changes User#manageable_groups in the way to drop Arel::Nodes::SqlLiteral usage and use raw SQL constructions.

Additional information

I have tested code on the current arel (6.0.4) (rails4), arel (7.1.4) (rails5), and arel (8.0.0) for rails 5.1 (not Gitlab, just a separate application).

The current code works only on arel (6.0.4).

Rails 4 (arel 6.0.4)

user = User.first
  User Load (1.2ms)  SELECT  "users".* FROM "users"  ORDER BY "users"."id" ASC LIMIT 1
=> #<User id:1 @root>

user.groups.ids
   (0.8ms)  SELECT "namespaces"."id" FROM "namespaces" INNER JOIN "members" ON "namespaces"."id" = "members"."source_id" WHERE "members"."source_type" = $1 AND "namespaces"."type" IN ('Group') AND "members"."user_id" = $2 AND "namespaces"."type" IN ('Group') AND "members"."type" IN ('GroupMember') AND "members"."requested_at" IS NULL  [["source_type", "Namespace"], ["user_id", 1]]
=> [1, 3, 4, 5, 6, 7]

union = Gitlab::SQL::Union.new([user.owned_groups.select(:id), user.masters_groups.select(:id)]).to_sql
=> "SELECT \"namespaces\".\"id\" FROM \"namespaces\" INNER JOIN \"members\" ON \"namespaces\".\"id\" = \"members\".\"source_id\" WHERE \"members\".\"source_type\" = 'Namespace' AND \"namespaces\".\"type\" IN ('Group') AND \"members\".\"user_id\" = 1 AND \"namespaces\".\"type\" IN ('Group') AND \"members\".\"access_level\" = 50 AND \"members\".\"type\" IN ('GroupMember') AND \"members\".\"requested_at\" IS NULL\nUNION\nSELECT \"namespaces\".\"id\" FROM \"namespaces\" INNER JOIN \"members\" ON \"namespaces\".\"id\" = \"members\".\"source_id\" WHERE \"members\".\"source_type\" = 'Namespace' AND \"namespaces\".\"type\" IN ('Group') AND \"members\".\"user_id\" = 1 AND \"namespaces\".\"type\" IN ('Group') AND \"members\".\"access_level\" = 40 AND \"members\".\"type\" IN ('GroupMember') AND \"members\".\"requested_at\" IS NULL"

arel_sql = Arel.sql(union)
=> "SELECT \"namespaces\".\"id\" FROM \"namespaces\" INNER JOIN \"members\" ON \"namespaces\".\"id\" = \"members\".\"source_id\" WHERE \"members\".\"source_type\" = 'Namespace' AND \"namespaces\".\"type\" IN ('Group') AND \"members\".\"user_id\" = 1 AND \"namespaces\".\"type\" IN ('Group') AND \"members\".\"access_level\" = 50 AND \"members\".\"type\" IN ('GroupMember') AND \"members\".\"requested_at\" IS NULL\nUNION\nSELECT \"namespaces\".\"id\" FROM \"namespaces\" INNER JOIN \"members\" ON \"namespaces\".\"id\" = \"members\".\"source_id\" WHERE \"members\".\"source_type\" = 'Namespace' AND \"namespaces\".\"type\" IN ('Group') AND \"members\".\"user_id\" = 1 AND \"namespaces\".\"type\" IN ('Group') AND \"members\".\"access_level\" = 40 AND \"members\".\"type\" IN ('GroupMember') AND \"members\".\"requested_at\" IS NULL"

Group.arel_table[:id].in(arel_sql)
=> #<Arel::Nodes::In:0x0000000008c57c10
 @left=
  #<struct Arel::Attributes::Attribute
   relation=
    #<Arel::Table:0x0000000009c5d9e8
     @aliases=[],
     @columns=nil,
     @engine=
      Group(id: integer, name: string, path: string, owner_id: integer, created_at: datetime, updated_at: datetime, type: string, description: string, avatar: string, share_with_group_lock: boolean, visibility_level: integer, request_access_enabled: boolean, description_html: text, lfs_enabled: boolean, parent_id: integer, require_two_factor_authentication: boolean, two_factor_grace_period: integer, cached_markdown_version: integer),
     @name="namespaces",
     @primary_key=nil,
     @table_alias=nil>,
   name=:id>,
 @right=
  #<Arel::Nodes::Casted:0x0000000008c57c38
   @attribute=
    #<struct Arel::Attributes::Attribute
     relation=
      #<Arel::Table:0x0000000009c5d9e8
       @aliases=[],
       @columns=nil,
       @engine=
        Group(id: integer, name: string, path: string, owner_id: integer, created_at: datetime, updated_at: datetime, type: string, description: string, avatar: string, share_with_group_lock: boolean, visibility_level: integer, request_access_enabled: boolean, description_html: text, lfs_enabled: boolean, parent_id: integer, require_two_factor_authentication: boolean, two_factor_grace_period: integer, cached_markdown_version: integer),
       @name="namespaces",
       @primary_key=nil,
       @table_alias=nil>,
     name=:id>,
   @val=
    "SELECT \"namespaces\".\"id\" FROM \"namespaces\" INNER JOIN \"members\" ON \"namespaces\".\"id\" = \"members\".\"source_id\" WHERE \"members\".\"source_type\" = 'Namespace' AND \"namespaces\".\"type\" IN ('Group') AND \"members\".\"user_id\" = 1 AND \"namespaces\".\"type\" IN ('Group') AND \"members\".\"access_level\" = 50 AND \"members\".\"type\" IN ('GroupMember') AND \"members\".\"requested_at\" IS NULL\nUNION\nSELECT \"namespaces\".\"id\" FROM \"namespaces\" INNER JOIN \"members\" ON \"namespaces\".\"id\" = \"members\".\"source_id\" WHERE \"members\".\"source_type\" = 'Namespace' AND \"namespaces\".\"type\" IN ('Group') AND \"members\".\"user_id\" = 1 AND \"namespaces\".\"type\" IN ('Group') AND \"members\".\"access_level\" = 40 AND \"members\".\"type\" IN ('GroupMember') AND \"members\".\"requested_at\" IS NULL">>

Group.where(Group.arel_table[:id].in(arel_sql))
  Group Load (2.5ms)  SELECT "namespaces".* FROM "namespaces" WHERE "namespaces"."type" IN ('Group') AND "namespaces"."id" IN (SELECT "namespaces"."id" FROM "namespaces" INNER JOIN "members" ON "namespaces"."id" = "members"."source_id" WHERE "members"."source_type" = 'Namespace' AND "namespaces"."type" IN ('Group') AND "members"."user_id" = 1 AND "namespaces"."type" IN ('Group') AND "members"."access_level" = 50 AND "members"."type" IN ('GroupMember') AND "members"."requested_at" IS NULL
UNION
SELECT "namespaces"."id" FROM "namespaces" INNER JOIN "members" ON "namespaces"."id" = "members"."source_id" WHERE "members"."source_type" = 'Namespace' AND "namespaces"."type" IN ('Group') AND "members"."user_id" = 1 AND "namespaces"."type" IN ('Group') AND "members"."access_level" = 40 AND "members"."type" IN ('GroupMember') AND "members"."requested_at" IS NULL)
  Route Load (0.7ms)  SELECT "routes".* FROM "routes" WHERE "routes"."source_type" = 'Namespace' AND "routes"."source_id" IN (1, 3, 4, 5, 7)
=> [#<Group id:1 @gitlab-org>, #<Group id:3 @gnuwget>, #<Group id:4 @Commit451>, #<Group id:5 @documentcloud>, #<Group id:7 @h5bp>]

Rails 5.0 (arel 7.1.4)

user = User.first
  User Load (0.9ms)  SELECT  "users".* FROM "users" ORDER BY "users"."id" ASC LIMIT $1  [["LIMIT", 1]]
=> #<User id:1 @root>

user.groups.ids
   (1.1ms)  SELECT "namespaces"."id" FROM "namespaces" INNER JOIN "members" ON "namespaces"."id" = "members"."source_id" WHERE "members"."source_type" = $1 AND "namespaces"."type" IN ('Group') AND "members"."user_id" = $2 AND "namespaces"."type" IN ('Group') AND "members"."type" IN ('GroupMember') AND "members"."requested_at" IS NULL  [["source_type", "Namespace"], ["user_id", 1]]
=> [1, 3, 4, 5, 6, 7]

union = Gitlab::SQL::Union.new([user.owned_groups.select(:id), user.masters_groups.select(:id)]).to_sql
=> "SELECT \"namespaces\".\"id\" FROM \"namespaces\" INNER JOIN \"members\" ON \"namespaces\".\"id\" = \"members\".\"source_id\" WHERE \"members\".\"source_type\" = 'Namespace' AND \"namespaces\".\"type\" IN ('Group') AND \"members\".\"user_id\" = 1 AND \"namespaces\".\"type\" IN ('Group') AND \"members\".\"access_level\" = 50 AND \"members\".\"type\" IN ('GroupMember') AND \"members\".\"requested_at\" IS NULL\nUNION\nSELECT \"namespaces\".\"id\" FROM \"namespaces\" INNER JOIN \"members\" ON \"namespaces\".\"id\" = \"members\".\"source_id\" WHERE \"members\".\"source_type\" = 'Namespace' AND \"namespaces\".\"type\" IN ('Group') AND \"members\".\"user_id\" = 1 AND \"namespaces\".\"type\" IN ('Group') AND \"members\".\"access_level\" = 40 AND \"members\".\"type\" IN ('GroupMember') AND \"members\".\"requested_at\" IS NULL"

arel_sql = Arel.sql(union)
=> "SELECT \"namespaces\".\"id\" FROM \"namespaces\" INNER JOIN \"members\" ON \"namespaces\".\"id\" = \"members\".\"source_id\" WHERE \"members\".\"source_type\" = 'Namespace' AND \"namespaces\".\"type\" IN ('Group') AND \"members\".\"user_id\" = 1 AND \"namespaces\".\"type\" IN ('Group') AND \"members\".\"access_level\" = 50 AND \"members\".\"type\" IN ('GroupMember') AND \"members\".\"requested_at\" IS NULL\nUNION\nSELECT \"namespaces\".\"id\" FROM \"namespaces\" INNER JOIN \"members\" ON \"namespaces\".\"id\" = \"members\".\"source_id\" WHERE \"members\".\"source_type\" = 'Namespace' AND \"namespaces\".\"type\" IN ('Group') AND \"members\".\"user_id\" = 1 AND \"namespaces\".\"type\" IN ('Group') AND \"members\".\"access_level\" = 40 AND \"members\".\"type\" IN ('GroupMember') AND \"members\".\"requested_at\" IS NULL"

Group.arel_table[:id].in(arel_sql)
=> #<Arel::Nodes::In:0x000000000d02d908
 @left=
  #<struct Arel::Attributes::Attribute
   relation=
    #<Arel::Table:0x0000000006c613c0
     @columns=nil,
     @name="namespaces",
     @table_alias=nil,
     @type_caster=
      #<ActiveRecord::TypeCaster::Map:0x0000000006c61410
       @types=
        Group(id: integer, name: string, path: string, owner_id: integer, created_at: datetime, updated_at: datetime, type: string, description: string, avatar: string, share_with_group_lock: boolean, visibility_level: integer, request_access_enabled: boolean, description_html: text, lfs_enabled: boolean, parent_id: integer, require_two_factor_authentication: boolean, two_factor_grace_period: integer, cached_markdown_version: integer)>>,
   name=:id>,
 @right=
  #<Arel::Nodes::Casted:0x000000000d02d9d0
   @attribute=
    #<struct Arel::Attributes::Attribute
     relation=
      #<Arel::Table:0x0000000006c613c0
       @columns=nil,
       @name="namespaces",
       @table_alias=nil,
       @type_caster=
        #<ActiveRecord::TypeCaster::Map:0x0000000006c61410
         @types=
          Group(id: integer, name: string, path: string, owner_id: integer, created_at: datetime, updated_at: datetime, type: string, description: string, avatar: string, share_with_group_lock: boolean, visibility_level: integer, request_access_enabled: boolean, description_html: text, lfs_enabled: boolean, parent_id: integer, require_two_factor_authentication: boolean, two_factor_grace_period: integer, cached_markdown_version: integer)>>,
     name=:id>,
   @val=
    "SELECT \"namespaces\".\"id\" FROM \"namespaces\" INNER JOIN \"members\" ON \"namespaces\".\"id\" = \"members\".\"source_id\" WHERE \"members\".\"source_type\" = 'Namespace' AND \"namespaces\".\"type\" IN ('Group') AND \"members\".\"user_id\" = 1 AND \"namespaces\".\"type\" IN ('Group') AND \"members\".\"access_level\" = 50 AND \"members\".\"type\" IN ('GroupMember') AND \"members\".\"requested_at\" IS NULL\nUNION\nSELECT \"namespaces\".\"id\" FROM \"namespaces\" INNER JOIN \"members\" ON \"namespaces\".\"id\" = \"members\".\"source_id\" WHERE \"members\".\"source_type\" = 'Namespace' AND \"namespaces\".\"type\" IN ('Group') AND \"members\".\"user_id\" = 1 AND \"namespaces\".\"type\" IN ('Group') AND \"members\".\"access_level\" = 40 AND \"members\".\"type\" IN ('GroupMember') AND \"members\".\"requested_at\" IS NULL">>

Group.where(Group.arel_table[:id].in(arel_sql))
  Group Load (0.8ms)  SELECT "namespaces".* FROM "namespaces" WHERE "namespaces"."type" IN ('Group') AND "namespaces"."id" IN (0)
=> []

Are there points in the code the reviewer needs to double check?

Yes. Take a look if there are much nicer arel based constructions.

Why was this MR needed?

Migration to Rails 5.0.

Screenshots (if relevant)

No.

Does this MR meet the acceptance criteria?

What are the relevant issue numbers?

#14286 (closed) and !12841 (closed)

Edited by Yorick Peterse

Merge request reports