Skip to content

Commit

Permalink
adapt to CTE usage
Browse files Browse the repository at this point in the history
  • Loading branch information
ulferts committed May 30, 2024
1 parent a77c4bc commit 5564ae5
Show file tree
Hide file tree
Showing 11 changed files with 46 additions and 37 deletions.
3 changes: 1 addition & 2 deletions app/models/news.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
4 changes: 1 addition & 3 deletions app/models/queries/actions/action_query.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,6 @@ def results
end

def default_scope
Action
.default
.distinct
Action.default
end
end
14 changes: 8 additions & 6 deletions app/models/queries/base_query.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down
5 changes: 5 additions & 0 deletions app/models/queries/capabilities/capability_query.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion app/models/queries/principals/filters/member_filter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
3 changes: 1 addition & 2 deletions app/models/wiki_page.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}

Expand Down
7 changes: 6 additions & 1 deletion lib/api/v3/notifications/notifications_api.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
3 changes: 1 addition & 2 deletions modules/documents/app/models/document.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}

Expand Down
3 changes: 1 addition & 2 deletions modules/meeting/app/models/meeting.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}

Expand Down
38 changes: 21 additions & 17 deletions spec/models/queries/news/news_query_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 5564ae5

Please sign in to comment.