Bug/agile 306 backlogs non admin user can t sort or group by backlog bucket#24090
Conversation
b06df44 to
f2f2dae
Compare
f2f2dae to
9830dc2
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
… names Defensive changes to avoid issues in the future.
There was a problem hiding this comment.
Pull request overview
This pull request fixes a regression where non-admin users could not group/sort work packages by backlog bucket when semantic work package identifiers are enabled, caused by a mismatch between Rails’ internal join aliasing and OpenProject’s alias prediction in Query::Results.
Changes:
- Pre-scan raw SQL join strings using a Rails-parity regex to align
Query::Results#include_aliaseswith Rails’AliasTrackeraliasing decisions. - Apply computed association aliases to non-association sort expressions (e.g., replacing hardcoded
projects.identifierwith the actual alias). - Remove an obsolete “explicit projects join” workaround from the project phase definitions query and add regression coverage.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| spec/models/query/results_project_phase_filter_integration_spec.rb | Regression coverage ensuring the project phase filter join no longer needs the explicit projects join under semantic/classic identifiers. |
| spec/models/query/results_alias_tracker_parity_spec.rb | Spec that guards RAW_JOIN_TABLE_SCAN_REGEX parity against Rails’ AliasTracker.initial_count_for. |
| modules/backlogs/spec/models/query_results_group_by_bucket_spec.rb | Regression spec for AGILE-306 (non-admin grouping by backlog bucket under semantic identifiers). |
| app/models/query/results.rb | Core fix: raw-join pre-scan for alias prediction + alias substitution for non-association sortable expressions. |
| app/models/queries/project_phase_definitions/database_queries.rb | Removes the prior explicit LEFT OUTER JOIN "projects" workaround now made unnecessary by corrected alias handling. |
ulferts
left a comment
There was a problem hiding this comment.
Works 👍 . I am not 100% if I was able to follow every detail but the structure was solid and the results speak for themselves.
It would still be good if all this code by now would no longer be necessary.
| build(:query, | ||
| user:, | ||
| project:, | ||
| show_hierarchies: false, |
There was a problem hiding this comment.
Nitpicking: the show_hierarchies parameter is probably not needed.
Ticket
https://community.openproject.org/wp/AGILE-306
Grouping by buckets would fail for non-admin users if (and only if) semantic work package identifiers were enabled.
This happens because:
Query::Resultsalways eagerly loads the project association viaWorkPackage.includes(:project). Rails internally runs AliasTracker to decide whether to alias the resulting join. It does this by text-scanning every raw SQL string already in the query forJOIN ... "table" ONpatterns. If the table was seen at least once, Rails aliases its generated join — soprojectsbecomesprojects_work_packages. So far, no problem.The backlog bucket grouping adds a raw SQL join that embeds a permission subquery (projects_with_view_sprints.to_sql). For non-admin users, that subquery internally contains
INNER JOIN "projects" ON (member permission check). This references the projects table -> table was seen once. Admin users take a different code path that only joinsenabled_modules, so their subquery never mentions projects and aliasing never triggers -> projects table was not seen.For non-admin users, Rails aliases the outmost projects join to
projects_work_packages. As a result, the ORDER BY of the query breaks. The reason is that with semantic identifiers enabled, it will sort via a hardcoded project table name that is only defined deep down in the permission check subquery and thus unknown.Queries::Results#include_aliasestries to predict which alias will be used by Rails for each table name, in order to build the correct ORDER BY expressions. But, it misses table references inside raw SQL join strings, so our aliases and the aliases defined by Rails can diverge in some cases. Here, this diversion caused the bug.What approach did you choose and why?
Query::Results#include_aliasesnow pre-scans all raw SQL join strings (sort criteria joins, group-by join, filter joins) using the same regex pattern asAliasTracker, before computing aliases. This ensures its alias prediction matches what Rails will generate.The regex was copied from Rails (to
Query::Results::RAW_JOIN_TABLE_SCAN_REGEX). To avoid that a change to Rails silently breaks our application, there is a spec that directly compares our regex against the liveAliasTracker.initial_count_forimplementation, so any future change to the Rails implementation will cause it to go red.Query::Results#aliased_sorting_by_column_namenow also applies the computed aliases to non-association sort expressions — replacing hardcoded table names likeprojects.identifierwith their actual aliases likeprojects_work_packages.identifier.As a drive-by cleanup, the project phase definitions join previously worked around the same aliasing issue by adding an explicit
LEFT OUTER JOIN "projects"to keep the table accessible regardless of the alias. This workaround could now be removed 👏Merge checklist