diff --git a/app/models/news.rb b/app/models/news.rb index 202723aee466..7bc5b95481d3 100644 --- a/app/models/news.rb +++ b/app/models/news.rb @@ -51,8 +51,7 @@ class News < ApplicationRecord after_create :add_author_as_watcher scope :visible, ->(*args) do - includes(:project) - .references(:projects) + left_joins(:project) .merge(Project.allowed_to(args.first || User.current, :view_news)) end diff --git a/app/models/queries/actions/action_query.rb b/app/models/queries/actions/action_query.rb index d00b821998d6..2b1217fc2939 100644 --- a/app/models/queries/actions/action_query.rb +++ b/app/models/queries/actions/action_query.rb @@ -40,8 +40,6 @@ def results end def default_scope - Action - .default - .distinct + Action.default end end diff --git a/app/models/queries/base_query.rb b/app/models/queries/base_query.rb index 7f949bd59fa3..f5ebe24a707b 100644 --- a/app/models/queries/base_query.rb +++ b/app/models/queries/base_query.rb @@ -64,14 +64,10 @@ def lookup_ancestors def results if valid? - filter_scope = model - .with(filtered: apply_filters(default_scope)) - .joins("JOIN filtered ON #{model.table_name}.id = filtered.id") - scope = if selects.any? - apply_selects(apply_orders(filter_scope.select("#{model.table_name}.*"))) + apply_selects(apply_orders(filtered_results_scope.select("#{model.table_name}.*"))) else - apply_orders(filter_scope) + apply_orders(filtered_results_scope) end merge_with_values(scope) @@ -158,6 +154,12 @@ def ordered? protected + def filtered_results_scope + model + .with(filtered: apply_filters(default_scope).reselect(:id)) + .where(model.select(1).from("filtered").where("#{model.table_name}.id = filtered.id").arel.exists) + end + def model self.class.model end diff --git a/app/models/queries/capabilities/capability_query.rb b/app/models/queries/capabilities/capability_query.rb index deeefb56151f..b84f84fcacb1 100644 --- a/app/models/queries/capabilities/capability_query.rb +++ b/app/models/queries/capabilities/capability_query.rb @@ -49,6 +49,11 @@ def default_scope private + def filtered_results_scope + # Due to the lack of an id, using CTEs would be hard to implement. + apply_filters(default_scope) + end + def minimum_filters_set any_required = filters.any? do |filter| [Queries::Capabilities::Filters::PrincipalIdFilter, diff --git a/app/models/queries/principals/filters/member_filter.rb b/app/models/queries/principals/filters/member_filter.rb index bda7af54f09c..76cc8e53df04 100644 --- a/app/models/queries/principals/filters/member_filter.rb +++ b/app/models/queries/principals/filters/member_filter.rb @@ -57,7 +57,7 @@ def apply_to(query_scope) def member_included_scope(scope) scope .visible - .includes(:members) + .left_joins(:members) .merge(Member.of_any_project) end end diff --git a/app/models/wiki_page.rb b/app/models/wiki_page.rb index 37810f31de5b..afe4924a182c 100644 --- a/app/models/wiki_page.rb +++ b/app/models/wiki_page.rb @@ -78,8 +78,7 @@ class WikiPage < ApplicationRecord } scope :visible, ->(user = User.current) { - includes(:project) - .references(:project) + left_joins(:project) .merge(Project.allowed_to(user, :view_wiki_pages)) } diff --git a/lib/api/v3/notifications/notifications_api.rb b/lib/api/v3/notifications/notifications_api.rb index 7e7c0d0671e4..0975b0a576a8 100644 --- a/lib/api/v3/notifications/notifications_api.rb +++ b/lib/api/v3/notifications/notifications_api.rb @@ -52,7 +52,12 @@ def notification_scope def bulk_update_status(attributes) if notification_query.valid? - notification_query.results.update_all({ updated_at: Time.zone.now }.merge(attributes)) + # The simpler notification_query.results.update_all(...) does not work as + # rails does not yet retain CTEs in the update_all method. + # https://github.com/rails/rails/pull/47193 + Notification + .where(id: notification_query.results.select(:id)) + .update_all({ updated_at: Time.zone.now }.merge(attributes)) status 204 else raise_query_errors(notification_query) diff --git a/modules/documents/app/models/document.rb b/modules/documents/app/models/document.rb index 7acb4e0c3d0a..6ac4d543a443 100644 --- a/modules/documents/app/models/document.rb +++ b/modules/documents/app/models/document.rb @@ -48,8 +48,7 @@ class Document < ApplicationRecord validates_length_of :title, maximum: 60 scope :visible, ->(user = User.current) { - includes(:project) - .references(:projects) + left_joins(:project) .merge(Project.allowed_to(user, :view_documents)) } diff --git a/modules/meeting/app/models/meeting.rb b/modules/meeting/app/models/meeting.rb index 9116281ae333..bba501a767f8 100644 --- a/modules/meeting/app/models/meeting.rb +++ b/modules/meeting/app/models/meeting.rb @@ -56,8 +56,7 @@ class Meeting < ApplicationRecord .includes({ participants: :user }, :author) } scope :visible, ->(*args) { - includes(:project) - .references(:projects) + left_joins(:project) .merge(Project.allowed_to(args.first || User.current, :view_meetings)) } diff --git a/spec/models/queries/news/news_query_spec.rb b/spec/models/queries/news/news_query_spec.rb index cb1b72db6757..d6d1a3eb4e74 100644 --- a/spec/models/queries/news/news_query_spec.rb +++ b/spec/models/queries/news/news_query_spec.rb @@ -29,37 +29,41 @@ require "spec_helper" RSpec.describe Queries::News::NewsQuery do - let(:user) { build_stubbed(:user) } - let(:base_scope) { News.visible(user).order(id: :desc) } let(:instance) { described_class.new } - before do - login_as(user) + shared_let(:role) { create(:project_role, permissions: [:view_news]) } + shared_let(:project) { create(:project) } + shared_let(:visible_news) { create(:news, project:) } + shared_let(:other_project) { create(:project) } + shared_let(:other_visible_news) { create(:news, project: other_project) } + shared_let(:invisible_news) { create(:news) } + + current_user do + create(:user, + member_with_roles: { + project => [role], + other_project => [role] + }) end context "without a filter" do describe "#results" do it "is the same as getting all the visible news" do - expect(instance.results.to_sql).to eql base_scope.to_sql + expect(instance.results) + .to eq [other_visible_news, visible_news] end end end context "with a project filter" do before do - allow(Project) - .to receive_message_chain(:visible, :pluck) - .with(:id) - .and_return([1]) - instance.where("project_id", "=", ["1"]) + instance.where("project_id", "=", [project.id]) end describe "#results" do - it "is the same as handwriting the query" do - expected = base_scope - .where(["news.project_id IN (?)", ["1"]]) - - expect(instance.results.to_sql).to eql expected.to_sql + it "returns the news from the project" do + expect(instance.results) + .to contain_exactly(visible_news) end end @@ -78,8 +82,8 @@ context "with an order by id asc" do describe "#results" do it "returns all visible news ordered by id asc" do - expect(instance.order(id: :asc).results.to_sql) - .to eql base_scope.except(:order).order(id: :asc).to_sql + expect(instance.order(id: :asc).results) + .to eq [visible_news, other_visible_news] end end end diff --git a/spec/requests/api/v3/principals/principals_resource_spec.rb b/spec/requests/api/v3/principals/principals_resource_spec.rb index c7ec8c0ae163..47a0183ef05e 100644 --- a/spec/requests/api/v3/principals/principals_resource_spec.rb +++ b/spec/requests/api/v3/principals/principals_resource_spec.rb @@ -322,7 +322,6 @@ end it "contains each user element only once" do - pending "This is just a fix to note that we have the bug, the fix will be done in the frontend at first, then we can come back here" expect(last_response.body).to be_json_eql(expected.to_json) end end