Skip to content

Bug/66611 activerecord statementinvalid in get api v3 queries queriesapi queries id and get api v3 queries queriesbyprojectapi projects id queries default #20590

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 7 commits into
base: dev
Choose a base branch
from
Draft

Bug/66611 activerecord statementinvalid in get api v3 queries queriesapi queries id and get api v3 queries queriesbyprojectapi projects id queries default #20590

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9b0971b
spec baseline with and without allowed_to perf improvement
ulferts Oct 10, 2025
0c104ae
TO BE REVOKED - enable allowed_to perf improvement in all specs
ulferts Oct 10, 2025
00a84bb
rewrite allowed_to to avoid from
ulferts Oct 10, 2025
7a8637a
provide a cte shadowing the work_packages table for historic query
ulferts Oct 10, 2025
3b8c39b
cleanup now unused code
ulferts Oct 10, 2025
826cb85
document the approach
ulferts Oct 13, 2025
d27f1df
simplify super call
ulferts Oct 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
490 changes: 85 additions & 405 deletions app/models/journable/historic_active_record_relation.rb
View file
Open in desktop

Large diffs are not rendered by default.

8 changes: 7 additions & 1 deletion app/models/work_packages/scopes/allowed_to.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,13 @@ def logged_in_non_admin_allowed_to_large_instances(user, permissions)
project_member_projects:,
entity_member_projects_without_duplicates:,
allowed_by_projects_and_work_packages:)
.from("allowed_by_projects_and_work_packages work_packages")
.where(<<~SQL.squish)
EXISTS (
SELECT 1
FROM allowed_by_projects_and_work_packages
WHERE work_packages.id = allowed_by_projects_and_work_packages.id
)
SQL
end

def allowed_to_admin_relation(permissions)
Expand Down
8 changes: 4 additions & 4 deletions spec/controllers/account_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ def reset!
describe "GET #internal_login" do
shared_let(:admin) { create(:admin) }

context "when direct login enabled", with_config: { omniauth_direct_login_provider: "some_provider" } do
context "when direct login enabled", with_settings: { omniauth_direct_login_provider: "some_provider" } do
it "allows to login internally using a special route" do
get :internal_login

Expand Down Expand Up @@ -339,7 +339,7 @@ def reset!
end

context "with direct login and redirecting callback",
with_config: { omniauth_direct_login_provider: "foo" }, with_settings: { login_required?: true } do
with_settings: { omniauth_direct_login_provider: "foo", login_required?: true } do
it "stills call the callback" do
# Set the previous session
session[:foo] = "bar"
Expand Down Expand Up @@ -421,7 +421,7 @@ def reset!
end

describe "#login with omniauth_direct_login enabled",
with_config: { omniauth_direct_login_provider: "some_provider" } do
with_settings: { omniauth_direct_login_provider: "some_provider" } do
describe "GET" do
it "redirects to some_provider" do
get :login
Expand All @@ -441,7 +441,7 @@ def reset!
end

describe "#login with omniauth_direct_login_provider set but empty",
with_config: { omniauth_direct_login_provider: "" } do
with_settings: { omniauth_direct_login_provider: "" } do
describe "GET" do
it "does not redirect to some_provider" do
get :login
Expand Down
10 changes: 5 additions & 5 deletions spec/features/auth/omniauth_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ def translation_substring(translation)
end

context "with direct login",
with_config: { omniauth_direct_login_provider: "developer" } do
with_settings: { omniauth_direct_login_provider: "developer" } do
it "goes directly to the developer sign in and then redirect to the back url" do
visit my_account_path
# requires login, redirects to developer login which is why we see the login form now
Expand All @@ -116,7 +116,7 @@ def translation_substring(translation)
end

describe "sign out a user with direct login and login required",
with_config: { omniauth_direct_login_provider: "developer", login_required: true } do
with_settings: { omniauth_direct_login_provider: "developer", login_required: true } do
it "shows a notice that the user has been logged out" do
visit signout_path

Expand Down Expand Up @@ -195,7 +195,7 @@ def translation_substring(translation)
end

context "with password login disabled",
with_config: { disable_password_login: "true" } do
with_settings: { disable_password_login: "true" } do
it_behaves_like "omniauth user registration"
end
end
Expand Down Expand Up @@ -223,7 +223,7 @@ def translation_substring(translation)
end

context "with direct login enabled and login required",
with_config: { omniauth_direct_login_provider: "developer" } do
with_settings: { omniauth_direct_login_provider: "developer" } do
before do
allow(Setting).to receive(:login_required?).and_return(true)
end
Expand Down Expand Up @@ -261,7 +261,7 @@ def translation_substring(translation)
end

context "with direct login and login required",
with_config: { omniauth_direct_login_provider: "developer" } do
with_settings: { omniauth_direct_login_provider: "developer" } do
before do
allow(Setting).to receive(:login_required?).and_return(true)
end
Expand Down
116 changes: 1 addition & 115 deletions spec/models/journable/historic_active_record_relation_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,12 +124,6 @@
describe "project_id in array (Arel::Nodes::HomogeneousIn)" do
let(:relation) { WorkPackage.where(project_id: [project.id, 1, 2, 3]) }

describe "#to_sql" do
it "transforms the expression to query the correct table" do
expect(subject.to_sql).to include "\"work_package_journals\".\"project_id\" IN (#{project.id}, 1, 2, 3)"
end
end

describe "#to_a" do
it "returns the requested work package" do
expect(subject.to_a).to include work_package
Expand All @@ -140,12 +134,6 @@
describe "project_id not in array (Arel::Nodes::HomogeneousIn)" do
let(:relation) { WorkPackage.where.not(project_id: [9999, 999]) }

describe "#to_sql" do
it "transforms the expression to query the correct table" do
expect(subject.to_sql).to include "\"work_package_journals\".\"project_id\" NOT IN (9999, 999)"
end
end

describe "#to_a" do
it "returns the requested work package" do
expect(subject.to_a).to include work_package
Expand All @@ -156,12 +144,6 @@
describe "id in array (Arel::Nodes::HomogeneousIn)" do
let(:relation) { WorkPackage.where(id: [work_package.id, 999, 9999]) }

describe "#to_sql" do
it "transforms the expression to query the correct table" do
expect(subject.to_sql).to include "\"journals\".\"journable_id\" IN (#{work_package.id}, 999, 9999)"
end
end

describe "#to_a" do
it "returns the requested work package" do
expect(subject.to_a).to include work_package
Expand All @@ -172,12 +154,6 @@
describe "id in subquery (Arel::Nodes::In)" do
let(:relation) { WorkPackage.where(id: [work_package.id, 99, 999, 9999]) }

describe "#to_sql" do
it "transforms the expression to query the correct table" do
expect(subject.to_sql).to include "\"journals\".\"journable_id\" IN (#{work_package.id}, 99, 999, 9999)"
end
end

describe "#to_a" do
it "returns the requested work package" do
expect(subject.to_a).to include work_package
Expand All @@ -188,10 +164,6 @@
describe "sql string (as used by Query#statement)" do
let(:relation) { WorkPackage.where("(work_packages.description ILIKE '%been on Wednesday%')") }

it "transforms the table name" do
expect(subject.to_sql).to include "work_package_journals.description ILIKE"
end

it "returns the requested work package" do
expect(subject).to include work_package
end
Expand All @@ -200,10 +172,6 @@
# This is used, for example, in 'follows' relations.
let(:relation) { WorkPackage.where("(work_packages.id IN (#{work_package.id}))") }

it "transforms the expression to query the correct table" do
expect(subject.to_sql).to include "journals.journable_id IN (#{work_package.id})"
end

it "returns the requested work package" do
expect(subject).to include work_package
end
Expand All @@ -213,10 +181,6 @@
# This is used in the manual-sorting feature.
let(:relation) { WorkPackage.where("(\"work_packages\".\"id\" IN (#{work_package.id}))") }

it "transforms the expression to query the correct table" do
expect(subject.to_sql).to include "\"journals\".\"journable_id\" IN (#{work_package.id})"
end

it "returns the requested work package" do
expect(subject).to include work_package
end
Expand Down Expand Up @@ -266,7 +230,7 @@
expect(subject_sql)
.to include <<~SQL.squish
JOIN customizable_journals ON
customizable_journals.journal_id = journals.id
customizable_journals.journal_id = work_packages.journal_id
AND customizable_journals.custom_field_id = #{custom_field.id}
SQL
expect(subject_sql).to include "customizable_journals.value ILIKE '%Wednesday\\_CV%'"
Expand Down Expand Up @@ -399,12 +363,6 @@
)
end

it "transforms the expression to query the correct table" do
expect(subject.to_sql).to include \
"\"work_package_journals\".\"subject\" = 'Foo' OR " \
"\"work_package_journals\".\"description\" = 'The work package as it has been on Wednesday'"
end

it "returns the requested work package" do
expect(subject).to include work_package
end
Expand All @@ -414,23 +372,13 @@
# as used by spec/features/work_packages/timeline/timeline_dates_spec.rb
let(:relation) { WorkPackage.where("work_packages.updated_at > '2022-01-01'") }

it "transforms the expression to query the correct table" do
expect(subject.to_sql).to include \
"journals.updated_at > '2022-01-01'"
end

it "returns the requested work package" do
expect(subject).to include work_package
end

describe "when using quotation marks" do
let(:relation) { WorkPackage.where("\"work_packages\".\"updated_at\" > '2022-01-01'") }

it "transforms the expression to query the correct table" do
expect(subject.to_sql).to include \
"\"journals\".\"updated_at\" > '2022-01-01'"
end

it "returns the requested work package" do
expect(subject).to include work_package
end
Expand All @@ -439,11 +387,6 @@
describe "when using a hash" do
let(:relation) { WorkPackage.where(work_packages: { updated_at: ("2022-01-01".to_datetime).. }) }

it "transforms the expression to query the correct table" do
expect(subject.to_sql).to include \
"\"journals\".\"updated_at\" >= '2022-01-01"
end

it "returns the requested work package" do
expect(subject).to include work_package
end
Expand All @@ -454,23 +397,13 @@
# as used by spec/features/work_packages/table/queries/filter_spec.rb
let(:relation) { WorkPackage.where("work_packages.created_at > '2022-01-01'") }

it "transforms the expression to query the correct table" do
expect(subject.to_sql).to include \
"journables.created_at > '2022-01-01'"
end

it "returns the requested work package" do
expect(subject).to include work_package
end

describe "when using quotation marks" do
let(:relation) { WorkPackage.where("\"work_packages\".\"created_at\" > '2022-01-01'") }

it "transforms the expression to query the correct table" do
expect(subject.to_sql).to include \
"\"journables\".\"created_at\" > '2022-01-01'"
end

it "returns the requested work package" do
expect(subject).to include work_package
end
Expand All @@ -479,11 +412,6 @@
describe "when using a hash" do
let(:relation) { WorkPackage.where(work_packages: { created_at: ("2022-01-01".to_datetime).. }) }

it "transforms the expression to query the correct table" do
expect(subject.to_sql).to include \
"\"journables\".\"created_at\" >= '2022-01-01"
end

it "returns the requested work package" do
expect(subject).to include work_package
end
Expand All @@ -494,21 +422,13 @@
describe "#order" do
let(:relation) { WorkPackage.order(description: :desc) }

it "transforms the table name" do
expect(subject.to_sql).to include "\"work_package_journals\".\"description\" DESC"
end

it "returns the requested work package" do
expect(subject).to include work_package
end

describe "manual order clause" do
let(:relation) { WorkPackage.order("work_packages.description DESC") }

it "transforms the table name" do
expect(subject.to_sql).to include "work_package_journals.description DESC"
end

it "returns the requested work package" do
expect(subject).to include work_package
end
Expand All @@ -518,10 +438,6 @@
# This is used in the manual-sorting feature.
let(:relation) { WorkPackage.order("work_packages.id DESC") }

it "transforms the table name" do
expect(subject.to_sql).to include "journals.journable_id DESC"
end

it "returns the requested work package" do
expect(subject).to include work_package
end
Expand All @@ -530,10 +446,6 @@
describe "order clause with work_packages.id" do
let(:relation) { WorkPackage.order(id: :desc) }

it "transforms the table name" do
expect(subject.to_sql).to include "\"journals\".\"journable_id\" DESC"
end

it "returns the requested work package" do
expect(subject).to include work_package
end
Expand All @@ -542,11 +454,6 @@
describe "several order clauses" do
let(:relation) { WorkPackage.order(subject: :asc, id: :desc) }

it "transforms the table name" do
expect(subject.to_sql).to include "\"work_package_journals\".\"subject\" ASC"
expect(subject.to_sql).to include "\"journals\".\"journable_id\" DESC"
end

it "returns the requested work package" do
expect(subject).to include work_package
end
Expand All @@ -559,14 +466,6 @@

before { work_package.time_entries << create(:time_entry) }

it "transforms the table name" do
expect(subject.to_sql).to include <<~SQL.squish
INNER JOIN "time_entries"
ON "time_entries"."entity_type" = 'WorkPackage'
AND "time_entries"."entity_id" = "journals"."journable_id"
SQL
end

it "returns the requested work package" do
expect(subject).to include work_package
end
Expand All @@ -579,11 +478,6 @@
.joins("LEFT OUTER JOIN ordered_work_packages ON ordered_work_packages.work_package_id = work_packages.id")
end

it "transforms the table name" do
expect(subject.to_sql).to include \
"LEFT OUTER JOIN ordered_work_packages ON ordered_work_packages.work_package_id = journals.journable_id"
end

it "returns the requested work package" do
expect(subject).to include work_package
end
Expand All @@ -597,14 +491,6 @@
.where(projects: { id: project.id })
end

it "joins the projects table" do
sql = subject.to_sql.tr('"', "")
expect(sql).to include \
"LEFT OUTER JOIN projects ON projects.id = work_package_journals.project_id"
expect(sql).to include \
"WHERE projects.id = #{project.id}"
end

it "returns the requested work package" do
expect(subject).to include work_package
end
Expand Down
14 changes: 0 additions & 14 deletions spec/models/journable/timestamps_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,20 +273,6 @@
describe "when querying with at_timestamp" do
subject { relation.at_timestamp(friday) }

it "joins the journable table rather than the journal-data table" do
expect(subject.to_sql).not_to include <<~SQL.squish
INNER JOIN "time_entries"
ON "time_entries"."entity_type" = 'WorkPackage'
AND "time_entries"."entity_id" = "work_package_journals"."id"
SQL

expect(subject.to_sql).to include <<~SQL.squish
INNER JOIN "time_entries"
ON "time_entries"."entity_type" = 'WorkPackage'
AND "time_entries"."entity_id" = "journals"."journable_id"
SQL
end

it "returns the matching records in their historic states" do
expect(subject.pluck(:description)).to eq ["The work package as it is since Friday"]
expect(subject.pluck(:id)).to eq [work_package.id]
Expand Down
Loading
Loading