Skip to content

Fix /users perf with security: idempotent admin?, drop inverse attrs from auth load#286

Merged
alexskr merged 1 commit intodevelopfrom
fix/users-endpoint-perf-with-security
Apr 28, 2026
Merged

Fix /users perf with security: idempotent admin?, drop inverse attrs from auth load#286
alexskr merged 1 commit intodevelopfrom
fix/users-endpoint-perf-with-security

Conversation

@alexskr
Copy link
Copy Markdown
Member

@alexskr alexskr commented Apr 28, 2026

Summary

  • Make User#admin? idempotent so it doesn't re-load :role per call.
    serialize_filter (added during the agroportal sync for lastLoginAt
    visibility) invokes Thread.current[:remote_user]&.admin? once per
    serialized user. With security enabled, this turned every authenticated
    /users?include=all into an N+1 association reload over the requesting
    user's role.
  • Auth middleware: exclude inverse attributes from the User load.
    attributes(:all) was including :provisionalClasses, which fans out
    across the entire ProvisionalClass graph on every authenticated request.
  • Add test_admin_with_shallow_role_load covering the auth-middleware
    load shape (admin and non-admin) and the idempotency contract.

Partially addresses #285. This PR fixes the
performance issue itself (the per-instance admin? fanout and the inverse-attr fanout
in the auth middleware).

…from auth load

User#admin? was called once per serialized user via serialize_filter (added
during the agroportal sync for lastLoginAt visibility), re-loading the
requesting user's role on every call. With ~21k users and security enabled,
GET /users?include=all took ~15 minutes; with security disabled (where
&.admin? short-circuits on nil), the same call returned in ~10s.

- Make admin? idempotent: bring(role: [:role]) only when role isn't loaded
  at the User level OR the Role objects haven't had their nested :role
  attribute loaded. The two-level guard handles the auth-middleware load
  shape, which loads :role shallowly without the nested embed_values
  pattern.

- Auth middleware: exclude inverse attributes from the User load.
  attributes(:all) included :provisionalClasses, an inverse attribute that
  fans out across the full ProvisionalClass graph on every authenticated
  request.

- Add test_admin_with_shallow_role_load covering the auth-middleware load
  shape (admin and non-admin) and the idempotency contract.

Refs #285
@alexskr alexskr requested a review from mdorf April 28, 2026 08:28
# middleware and access-control checks actually read on every request);
# leaving forward scalars in for now to avoid surprising any callers
# that quietly depend on profile fields being preloaded.
attrs_to_load = LinkedData::Models::User.attributes(:all) - LinkedData::Models::User.attributes(:inverse)
Copy link
Copy Markdown
Member

@mdorf mdorf Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if the existing API already includes createdOntologies as part of this call:

LinkedData::Models::User.attributes(:inverse)

Copy link
Copy Markdown
Member Author

@alexskr alexskr Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — checked.

User.attributes(:inverse) returns just [:provisionalClasses]. :createdOntologies is declared with handler:, not inverse:, so it isn't in that list.

However, the auth load doesn't end up fetching :createdOntologies either, because Goo's where#include silently drops handler-backed attrs at the include stage (goo/lib/goo/base/where.rb:262-264):

if @klass.handler?(opt)
  next
end

So even though attrs_to_load still contains :createdOntologies after the subtraction, the symbol never makes it into the SPARQL pattern and load_created_ontologies is never invoked on the auth path.

@alexskr
Copy link
Copy Markdown
Member Author

alexskr commented Apr 28, 2026

Following up on #286 (comment) — the inline thread answers why :createdOntologies is excluded from the auth load (handler attrs are silently dropped by Goo's where#include), but it raised a more general question I want to flag separately, out of scope for this PR but worth tracking:

Should :createdOntologies be a handler in the first place, or should it be an inverse: attribute?

The same shape — "ontologies where this user is in administeredBy" — could be expressed as:

attribute :createdOntologies, inverse: { on: :ontology, attribute: :administeredBy }

That's how :provisionalClasses is already declared on User. Pros vs the current handler:

  1. Standard ORM semantics: where(...).include(:createdOntologies) would actually load it. As a handler today, where#include silently drops it (goo/lib/goo/base/where.rb:262-264), so ?include=createdOntologies on /users is a no-op — data only loads on .createdOntologies method access.
  2. Caller controls attribute depth: the handler's two-step query hardcodes Ontology.goo_attrs_to_load([:all]), so any consumer pays the full ontology load. With inverse:, the caller passes whatever they need via the regular include syntax.
  3. Avoids the bring-invokes-handler footgun: Goo's recent bring change (ncbo/goo#73) made bring(handler_attr) invoke the handler instead of raising. Any code that calls user.bring_remaining now triggers a heavy two-query load. An inverse attr would just be a normal SPARQL pattern.

The handler does do two things an inverse: wouldn't directly:

  • Scopes the first query to Ontology.uri_type via raw SPARQL — possibly a workaround for named-graph constraints that inverse: doesn't apply.
  • Eager-loads matched ontologies with [:all] attributes, returning fully-hydrated objects.

If those two properties were a real design requirement, the handler stays. If they were convenience (or just inherited from upstream agroportal code without a specific reason), the inverse form is simpler and avoids the issues above.

Does anyone know the original reasoning? If not, I can file a separate issue to track switching to inverse: and seeing what breaks.

@alexskr alexskr merged commit 24c5a6d into develop Apr 28, 2026
10 checks passed
Copy link
Copy Markdown
Member

@mdorf mdorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good aside from the comment about createdOntologies. Ready to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants