The performance decreased because of a combination of calls that were supposed to increase performance.
We have a mechanism in place which automatically eager loads models needed in the element representers when a collection of them is rendered. This is to avoid N+1 queries of course. But, if eager loading is combined with e.g., a LIMIT, which we do because we paginate, rails automatically falls back to issuing two instead of just one SQL statement. Which makes sense as otherwise LEFT JOINS might mess with the result set.
But Rails does so in a somewhat simple fashion. It uses the first query to get the DISTINCT ids. The second is used to load the values (without a limit). But instead of removing all WHERE statements in the second SQL statement and then apply just the one for the ids, it keeps the original WHERE statement and applies the one for the ids on top. The problem with that is that the database trips on that (I didn't check the why) and uses a less than optimal query plan.
That was the problem here as well. The first query remained reasonable quick (300ms) but the second one took 25s.
The fix is to split the two statements by hand in the representer whenever eager loading is defined. The first query has all the filters but no eager loading and fetches the ids. The second takes the ids, and with eager loading included loads the data. Et voila, second query takes 10ms.
That at least works for relations, work_packages and projects. But there are other representers that also seem to have custom behaviour. I'm looking into whether they can be easily adapted.
Minimal regression coverage for the rest of the WP HAL surface.
These links already pass `id:` kwargs (or go through api_v3_paths
which is pure string concatenation), so they stay numeric in
semantic mode today — but nothing in the test suite proved it.
Verified the :pdf / :atom / :generate_pdf specs fail if the kwargs
are dropped from the representer. The :self spec passes regardless,
since api_v3_paths doesn't go through Rails URL helpers; it stays
in as contract documentation for the API surface.
The classic-mode coverage was either tautological (the :copy href was
re-derived via `work_package_path(work_package, "copy")` — the very
helper that produced the wrong URL in semantic mode before the pin) or
silently ambiguous in classic mode where `to_param == id&.to_s` masks
whether the representer is using the pin.
Add a semantic-mode context for both links that asserts the literal
numeric URL, and tighten the existing classic copy assertion to match
the move assertion's literal-URL style. Verified the new specs fail
without the pin in `lib/api/v3/work_packages/work_package_representer.rb`
and pass with it.
The ancestors/children representer change calls `child.display_id`,
which consults `identifier` in semantic mode. The Hierarchy eager
loader preloads children with a minimal `SELECT id, subject, project_id,
parent_id` for performance, so `identifier` was missing and
`ActiveModel::MissingAttributeError` fired the moment a query endpoint
rendered a work package with visible children in semantic mode.
Add `identifier` to the SELECT. It's one extra short text column per
child row.
In semantic mode, the work package breadcrumb renders numeric IDs
instead of the semantic identifier because ancestor HAL resources are
built from `_links.ancestors[]` entries that only carry `href` and
`title`. With no top-level `displayId` in `$source`, the frontend getter
falls through to the numeric id parsed from the href.
Emit `displayId` alongside `href`/`title` on each ancestor and child
link in the representer, and have the `displayId` getter fall back to
the self link's `displayId` so resources built from a link payload
alone still surface the semantic identifier.
In semantic mode, work packages created before the feature was enabled
have a nil identifier column. Previously display_id returned nil for
these, causing the Ruby representer to serialize displayId as null
while the SQL representer used COALESCE to return the numeric id.
Use identifier.presence to fall back to the numeric id, aligning both
representer paths.
Replace the conditional `semanticId` API field with `displayId` which is
always present in work package responses. In semantic mode it returns the
project-based identifier (e.g. "PROJ-42"), in classic mode it returns the
numeric ID as a string. This gives API consumers (frontend, mobile) a
single field to read without conditional logic.
- Add `WorkPackage#display_id` method that encapsulates the mode check
- Update both representers (JSON and SQL) to render `displayId` unconditionally
- Update OpenAPI schema documentation
Adds the computed semanticId property to the HAL representer,
SQL collection representer, and schema representer. The property is
gated behind the semantic_work_package_ids feature flag and returns the
value from WorkPackage#identifier. Includes OpenAPI docs
and the translation key for the schema name.
This class got broken during what seems to be a
drive-by style-improvement in fbe1215365. That change:
* made it incompatible with frozen strings as error messages
* broke the intended hiding of messages if they came from the
wrong class
All of this went by unnoticed, because there were no specs
for the InternalError class.
Specs have now been added and the previous version of the code
mostly restored. Since there were some callers that always created the
exception with known safe error messages, I added a new class just for these
cases, because they were intended to "just show the message". So we can
keep using the original implementation for rescue_from handling.
Both are rendered through the ProjectRepresenter (because they are technically
implemented as Projects), but haven't been tested against it yet.
This also means that they still included required properties that were already
removed from the project schema (this was an error in the schema).
This is against the schema for definition for links, which
already allow the title to be missing, but don't allow
it to be null. If it's present it must have a string as a value.
Those self-tests are "basic" in the sense that they only validate
their compliance with our documented schema in one representation.
These test cases don't yet cover/validate whether the generated
representation also fulfills the schema under different circumstances,
for example when rendering for a user with fewer privileges, not allowed
to see certain fields.
Where necessary, the schema was changed to reflect the reality, e.g.
when those tests revealed that a "required" field might be missing due to
a lack of permissions.
In a few cases the implementation was adapted to allow for stricter guarantees
of the specified schema, for example links allowed to leave out the title key
already, so its not necessary to emit `title: nil` in cases where a title is
not known.
Oliver Günther