Skip to content

feat(notification): support $current_user placeholder in rule conditions - #277

Merged
sleidig merged 9 commits into
mainfrom
claude/zen-davinci-r5lvq6
Nov 5, 2026
Merged

sleidig merged 9 commits into
mainfrom
claude/zen-davinci-r5lvq6

Conversation

@sleidig

@sleidig sleidig commented Nov 4, 2026 •

Copy link
Copy Markdown
Member

Backend part of Aam-Digital/ndb-core#4545: notification rule conditions can keep the "$current_user" placeholder (e.g. {"assignedTo": {"$elemMatch": "$current_user"}}). The backend resolves it, so the frontend no longer needs to replace it with the user's entity ID when it creates a NotificationConfig from the template.

Changes

New generic common/placeholder package (not tied to conditions, notifications or Keycloak)

  • Placeholders: the placeholder names, shared with the frontend's PLACEHOLDERS enum. For now this is only $current_user.
  • PlaceholderResolver.resolve(json, userIdentifier): one call that replaces all placeholders in any JSON tree with their values for that user. $current_user is the entity linked to the account, taken from the UserProfileProvider.
    • The user is only looked up when the tree actually uses a placeholder.
    • Only string values are replaced, including inside arrays. Keys and operators are left alone, as are strings that only contain a placeholder and unknown $… strings.
    • When placeholders can't be resolved, it throws a PlaceholderResolutionException instead of putting in null (in a query, {"field": null} also matches documents that don't have the field). There are two cases:
      • UnresolvedPlaceholderException: the placeholder has no value for this user, e.g. no entity is linked to the account.
      • PlaceholderValueLookupException: the lookup itself failed, e.g. Keycloak is unreachable.
    • Without a UserProfileProvider (no Keycloak admin access) there are no values, and it logs a warning the first time a placeholder is actually used.

common/keycloak: generic user profile lookup

  • UserProfileProvider / KeycloakUserProfileProvider return a user's profile (email and linked entity). It knows nothing about placeholders, and profiles are cached for 5 minutes (failed lookups are not cached).
    • The linked entity comes from the user attribute exact_username. A value without an entity type gets the legacy User: prefix, as in the frontend.
  • @ConditionalOnKeycloakAdminAccess can now also be put on @Bean methods.

Notification emails use the same lookup

  • EmailCreateNotificationHandler gets the address from UserProfileProvider. UserEmailProvider, KeycloakUserEmailProvider and the Spring user-emails cache are removed.
  • A failed email lookup (e.g. Keycloak unreachable) is now a TransientNotificationException, so the outbox retries it. Before, the email was silently skipped.

Notification config cache

  • Each enabled rule's conditions are parsed as documentConditionEngine.parse(placeholderResolver.resolve(conditions, user)). This happens once, when a config is loaded or refreshed, not on every change.
  • A rule whose placeholders can't be resolved (no linked entity, no Keycloak, or Keycloak unreachable) is skipped with a warning. The user's other rules still work, and the skipped rule comes back with the next reload or when the config changes.
  • Disabled rules are not cached at all (matching already ignored them), so their placeholders are not looked up.
  • The rule identifier is derived from the resolved conditions, so it is the same whether the frontend or the backend replaced the placeholder.
  • The whole cache is reloaded once a day (notification.config-cache.ttl-millis, default 86400000), so a changed user–entity link takes effect without a restart.
  • If that reload fails (e.g. CouchDB is unreachable), the previous configs keep being used and the reload is retried after 5 minutes. This uses a new opt-in retryAfterFailedReload option on LazySnapshot.

Docs: the notification module README and docs/modules/notification.md.

Not included

  • $now, which isn't needed for notification conditions yet.
  • An /actuator/features flag: frontend and backend versions are matched during rollout.
  • Frontend changes: once this is deployed, the frontend can stop replacing $current_user in notification-settings.component.ts. Configs saved earlier with concrete entity IDs keep working unchanged.

Testing

  • Unit tests for PlaceholderResolver, KeycloakUserProfileProvider (including its cache), LazySnapshot, the email handler's transient lookup failure, and the bean wiring (PlaceholderConfigurationTest, NotificationEmailHandlerWiringTest).
  • DefaultNotificationConfigCacheTest:
    • placeholder resolved;
    • same rule ID as a frontend-resolved rule;
    • only the affected rule skipped when there is no value or the lookup fails;
    • no lookup without placeholders or for disabled rules;
    • reload after the TTL, and the previous configs kept when that reload fails.
  • The reviewer tested an earlier version on a full local stack, before the email and user-profile refactors.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Fb2hmoP1JTL1ea9sPYBsYL

Notification rule conditions can now keep the "$current_user" placeholder
instead of the frontend replacing it with the user's entity id when the
config is created. The backend resolves it once when loading a config into
the NotificationConfigCache, from the Keycloak user attribute
"exact_username" (with the legacy "User:" prefix fallback).

- new generic common/placeholder package: PlaceholderResolver replaces
  placeholder values in any JSON tree; PlaceholderValueProvider looks up
  values per user account (Keycloak, or none without Keycloak admin access)
- a rule whose placeholder has no value for its user is skipped with a
  warning, without affecting the user's other rules
- a temporary Keycloak failure fails the cache load (retried with the next
  change); a single config refresh keeps the previously cached version
- the notification config cache is reloaded once a day
  (notification.config-cache.ttl-millis) so changed user links are applied

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Fb2hmoP1JTL1ea9sPYBsYL
@sleidig
sleidig requested a review from Abhinegi2 November 4, 2026 18:10
@coderabbitai

coderabbitai Bot commented Nov 4, 2026 •

Copy link
Copy Markdown

Explain Change →

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • Trigger review
Finishing Touches

Generate docstrings

  • Commit to this branch
  • Create a new PR
Generate unit tests (beta)

  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

@sleidig sleidig moved this to Technical Review in All Tasks & Issues Nov 4, 2026
- allow @ConditionalOnKeycloakAdminAccess on @bean methods
- wrap long method chains and arguments in tests as ktlint requires
- give the placeholder value copy an explicit type

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Fb2hmoP1JTL1ea9sPYBsYL
@Abhinegi2

Copy link
Copy Markdown
Member

tested this on a full local stack, and seems to working as expected, wasn't able to break anything
Code review skill highlighted two optional suggestions (neither is a blocker):

  • Fallback on failed reload after TTL: If the cache expires while Keycloak is unreachable, the reload fails and all changes are dropped for all users, including rules that don't contain placeholders. Falling back to the previously cached data would be more robust. However, the impact is limited today since the permission check already blocks everything during a Keycloak outage.
  • Skip disabled rules during lookup: Disabled rules still trigger a Keycloak lookup and generate the Skipping… WARN log. Adding a small filter to exclude disabled rules would eliminate this unnecessary lookup and log noise.

Both are minor improvements and can be addressed separately.

@Abhinegi2 Abhinegi2 moved this from Technical Review to In Progress in All Tasks & Issues Nov 5, 2026
…led rules

- LazySnapshot can now serve the previous copy when a reload fails
  (retryAfterFailedReload), instead of throwing to every reader
- the notification config cache uses it: if the daily reload fails (e.g.
  Keycloak unreachable), notifications keep working with the previous
  configs and the reload is tried again after 5 minutes
- disabled rules are not cached, so their placeholders are not looked up
  and do not log warnings

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Fb2hmoP1JTL1ea9sPYBsYL

sleidig commented Nov 5, 2026

Copy link
Copy Markdown
Member Author

Thanks for testing! I addressed both suggestions in this PR (b5f1367):

  • Fallback on a failed reload: LazySnapshot has a new opt-in retryAfterFailedReload. When a reload of an expired copy fails, it keeps serving the previous copy and tries again later, instead of throwing to every reader. A failed first load still throws, because there is nothing to serve. The notification config cache uses it with a 5-minute retry, so a Keycloak outage during the daily reload no longer stops notifications for anyone.
  • Disabled rules: these are no longer cached (the matching step already ignored them), so their placeholders are not looked up and don't log the "Skipping…" warning.

Both have new tests in LazySnapshotTest and DefaultNotificationConfigCacheTest.


Generated by Claude Code

@sleidig sleidig self-assigned this Nov 5, 2026
@sleidig sleidig moved this from In Progress to Technical Review in All Tasks & Issues Nov 5, 2026
…ak service

Keycloak no longer knows about placeholders:

- common/keycloak: new UserProfileProvider (KeycloakUserProfileProvider)
  returns a user's profile, including the entity linked to the account
- common/placeholder: UserPlaceholderValueProvider maps the profile to
  placeholder values; without a UserProfileProvider (no Keycloak admin
  access) it has no values, replacing UnavailablePlaceholderValueProvider

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Fb2hmoP1JTL1ea9sPYBsYL
- the email notification handler gets the address from UserProfileProvider;
  UserEmailProvider, KeycloakUserEmailProvider and the Spring email cache
  are removed. KeycloakUserProfileProvider caches profiles for 5 minutes
  instead (failed lookups are not cached).
- a failed email address lookup (e.g. Keycloak unreachable) is now a
  transient failure that the outbox retries, instead of silently skipping
  the email
- PlaceholderValueProvider is a single concrete class that works out all
  placeholders, replacing the interface and UserPlaceholderValueProvider

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Fb2hmoP1JTL1ea9sPYBsYL
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Fb2hmoP1JTL1ea9sPYBsYL
PlaceholderResolver.resolve(node, userIdentifier) now finds the placeholders,
looks up their values (only if any are used) and replaces them, merging
PlaceholderValueProvider into it. The notification config cache only wraps
its existing condition parsing with that one call.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Fb2hmoP1JTL1ea9sPYBsYL
…solved

A failed user profile lookup (e.g. Keycloak unreachable) no longer fails the
whole cache load or keeps a stale config on refresh: like a missing value,
it is logged as a warning and only the affected rule is skipped. Both are
PlaceholderResolutionException, so the cache catches them in one place.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Fb2hmoP1JTL1ea9sPYBsYL
@sleidig
sleidig merged commit 11a3f1d into main Nov 5, 2026
18 checks passed
@sleidig
sleidig deleted the claude/zen-davinci-r5lvq6 branch November 5, 2026 10:14
@github-project-automation github-project-automation Bot moved this from Technical Review to Done in All Tasks & Issues Nov 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants