[Rails5] Fix `User#manageable_groups`
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?
-
Changelog entry added, if necessary -
Documentation created/updated -
API support added -
Tests added for this feature/bug - Review
-
Has been reviewed by UX -
Has been reviewed by Frontend -
Has been reviewed by Backend -
Has been reviewed by Database
-
-
Conform by the merge request performance guides -
Conform by the style guides -
Squashed related commits together -
Internationalization required/considered -
End-to-end tests pass ( package-and-qa
manual pipeline job)
What are the relevant issue numbers?
Edited by Yorick Peterse