From 0c71468a93e042c9a9b62593016e4646ec1e7f2f Mon Sep 17 00:00:00 2001 From: Hendrik Ebbers Date: Mon, 19 Oct 2026 16:38:55 +0200 Subject: [PATCH] feat(events): carry the previous state on OnObjectUpdate OnObjectUpdate gets a second constructor (previous, current) plus isPreviousKnown() and Optional getPrevious(). The one-argument constructor stays for publishers that cannot know the previous state. The event carries no diff and no actor; listeners compare themselves. AbstractDbBackedDataService and AbstractMultitenantDbBackedDataService snapshot toData(entity) before updateEntity, replace postSave(boolean, D) with postInsert(D) / postUpdate(D, D) and publish no event for an update where previous equals current. GenericDataEvent now implements ResolvableTypeProvider: typed listeners such as OnObjectUpdate previously received events for every DTO type. Spec 022; origin open-tasks#36 (TaskChangedEvent). Co-Authored-By: Claude Opus 5.5 --- README.md | 5 +- docs/releases/upgrade-to-1.5.md | 93 +++++++- .../behaviors.md | 120 ++++++++++ .../022-update-event-previous-state/design.md | 223 ++++++++++++++++++ docs/specs/INDEX.md | 1 + .../data/AbstractDbBackedDataService.java | 62 +++-- .../spring/base/data/package-info.java | 7 +- .../spring/base/events/GenericDataEvent.java | 28 ++- .../spring/base/events/OnObjectUpdate.java | 89 ++++++- .../spring/base/events/package-info.java | 17 +- ...tDbBackedDataServicePublishEventsTest.java | 86 +++++-- .../base/events/GenericDataEventTest.java | 116 +++++++++ .../base/events/TypedEventListenerTest.java | 112 +++++++++ ...bstractMultitenantDbBackedDataService.java | 49 ++-- ...titenantDbBackedDataServiceEventsTest.java | 179 ++++++++++++++ 15 files changed, 1121 insertions(+), 66 deletions(-) create mode 100644 docs/specs/022-update-event-previous-state/behaviors.md create mode 100644 docs/specs/022-update-event-previous-state/design.md create mode 100644 spring-services-core/src/test/java/com/openelements/spring/base/events/TypedEventListenerTest.java create mode 100644 spring-services-tenant/src/test/java/com/openelements/spring/base/tenant/AbstractMultitenantDbBackedDataServiceEventsTest.java diff --git a/README.md b/README.md index 52a2fc1..6d4ab22 100644 --- a/README.md +++ b/README.md @@ -220,7 +220,10 @@ from committed configuration. and no status enum — path, authorization and response shape stay with the application. - **Lifecycle Events** — `OnObjectCreate`, `OnObjectUpdate` and `OnObjectDelete` are published synchronously inside the originating transaction; consumers opt into post-commit behaviour - with `@TransactionalEventListener`. + with `@TransactionalEventListener`. `OnObjectUpdate` carries the state before the update + (`getPrevious()`) when the publisher knows it, and the data services publish no event for an + update that changed nothing. Typed listeners (`OnObjectUpdate`) receive only their own + DTO type. - **Application Build & SBOM Info** — `ApplicationInfoService` answers *"which build is running, and what is it made of?"* from build-produced metadata: artifact coordinates and Git commit (from Spring Boot's `BuildProperties` / `GitProperties`) plus a parsed CycloneDX SBOM. diff --git a/docs/releases/upgrade-to-1.5.md b/docs/releases/upgrade-to-1.5.md index 49cffd5..384bf35 100644 --- a/docs/releases/upgrade-to-1.5.md +++ b/docs/releases/upgrade-to-1.5.md @@ -3,12 +3,13 @@ > **Draft — 1.5.0 is unreleased.** This document is written as the release takes shape; sections are > added as features land. Everything described here is on `main` today. -`spring-services` 1.5.0 brings one change so far. Every section below stands on its own — apply only +`spring-services` 1.5.0 brings two changes so far. Every section below stands on its own — apply only the ones you need: | Change | Spec | Nature | | --- | --- | --- | | Optional **object store** `ObjectStore` | — | new optional module, off by default, no schema change | +| **Previous state on `OnObjectUpdate`**, no-op updates suppressed, typed listeners fixed | 022 | source break for `postSave` overrides, behaviour change, no schema change | ## Object store (`spring-services-storage`) @@ -177,3 +178,93 @@ Both are unchecked. - **The AWS SDK comes along even if you only use `type=file`.** `software.amazon.awssdk:s3` is a hard dependency of the module today. Making it optional is planned; until then, budget for it in your dependency footprint. + +--- + +## Previous state on update events (spec 022) + +`OnObjectUpdate` now carries the state **before** the update when the publisher knows it. +`AbstractDbBackedDataService` and `AbstractMultitenantDbBackedDataService` always supply it. +Publishers that cannot know it (for example data from an external system) keep using +`new OnObjectUpdate<>(current)`: + +```java +@EventListener +public void onTaskUpdated(OnObjectUpdate event) { + if (!event.isPreviousKnown()) { + return; // or: treat as "something changed" + } + final TaskDto previous = event.getPrevious().orElseThrow(); + final TaskDto current = event.getData(); + if (!previous.columnId().equals(current.columnId())) { /* moved */ } +} +``` + +The event computes no diff and carries no actor. Compare the fields you care about yourself, and +resolve the actor through `AuthService`, as `AuditLogEventListener` does. + +This file is a self-contained prompt for an agent to run inside a consumer repo. + +### Prompt + +You are working inside a Spring Boot service that depends on `spring-services` 1.5.0. + +#### 1. Replace `postSave` overrides + +`postSave(boolean insert, D data)` no longer exists on either data service base class. Any +`@Override` of it now fails to compile. Split it: + +| Before | After | +| --- | --- | +| `postSave(true, data)` branch | `protected void postInsert(D data)` | +| `postSave(false, data)` branch | `protected void postUpdate(D previous, D current)` | + +Call `super.postInsert(...)` / `super.postUpdate(...)` wherever the old override called +`super.postSave(...)`, otherwise the lifecycle events are no longer published. + +#### 2. Make `toData` / `mapToData` return a snapshot + +On update, the base class calls `toData(entity)` **before** `updateEntity` mutates the entity, to +capture the previous state. A DTO that holds the entity's own collection changes along with the +entity, so the previous state becomes equal to the new one and the update event is **silently +dropped** (see step 3). Search every `toData` / `mapToData` for collections passed through +unchanged and copy them: + +```java +// wrong: shares the entity's mutable set +return new TaskDto(entity.getId(), entity.getAssignees()); +// right +return new TaskDto(entity.getId(), Set.copyOf(entity.getAssignees())); +``` + +Mapping to a new collection (`stream().map(...).toList()`) is already a copy. + +#### 3. Expect no event for no-op updates + +An update where `previous.equals(current)` no longer publishes `OnObjectUpdate`. As a result, no +audit-log row is written and no webhook fires for it. Remove any listener code that only filtered +these events out. DTOs that are not records and do not implement `equals` are never equal and +behave as before. + +#### 4. Check typed listeners + +Before 1.5.0, a listener declared for `OnObjectUpdate` (or `OnObjectCreate<…>` / +`OnObjectDelete<…>`) was invoked for **every** DTO type, because Spring could not resolve the +type parameter. The events now implement `ResolvableTypeProvider`, so such listeners receive only +their own type. Search for listeners that relied on the old behaviour: a typed parameter combined +with an `event.getType()` check for a *different* type. Move them to `GenericDataEvent`. +Listeners on `GenericDataEvent` or raw types are unaffected. + +#### 5. Optional: replace hand-written snapshots + +If the application snapshots an entity before `save(...)` to diff it afterwards (like open-tasks' +`TaskSnapshot`), it can listen to `OnObjectUpdate` and compare `getPrevious()` with `getData()` +instead. + +### Guard rails / Don't do this + +- Do not treat `isPreviousKnown() == false` as "nothing changed". It means the publisher did not + know the previous state. +- Do not persist `getPrevious()` without checking it for personal data that the update just + removed. + diff --git a/docs/specs/022-update-event-previous-state/behaviors.md b/docs/specs/022-update-event-previous-state/behaviors.md new file mode 100644 index 0000000..381ccdb --- /dev/null +++ b/docs/specs/022-update-event-previous-state/behaviors.md @@ -0,0 +1,120 @@ +# Behaviors: Previous state on update events + +Scenarios are unit-level unless marked **[context]** (a real Spring `ApplicationContext` dispatching +the event). + +## `OnObjectUpdate` construction + +### An update event without a previous state + +- **Given** a DTO `current` +- **When** `new OnObjectUpdate<>(current)` is created +- **Then** `getData()` returns `current` +- **And** `isPreviousKnown()` is `false` +- **And** `getPrevious()` is empty + +### An update event with a previous state + +- **Given** two DTOs `previous` and `current` of the same class with the same id +- **When** `new OnObjectUpdate<>(previous, current)` is created +- **Then** `getData()` returns `current` +- **And** `isPreviousKnown()` is `true` +- **And** `getPrevious()` contains `previous` + +### A previous state equal to the current state is accepted + +- **Given** `previous` and `current` that are equal +- **When** `new OnObjectUpdate<>(previous, current)` is created +- **Then** the event is created — suppressing no-ops is not the event's job + +### A null previous state is rejected + +- **When** `new OnObjectUpdate<>(null, current)` is created +- **Then** a `NullPointerException` is thrown + +### A previous state with a different id is rejected + +- **Given** `previous` and `current` of the same class with different ids +- **When** `new OnObjectUpdate<>(previous, current)` is created +- **Then** an `IllegalArgumentException` is thrown + +### A previous state of a different class is rejected + +- **Given** `previous` and `current` of different classes with the same id +- **When** `new OnObjectUpdate<>(previous, current)` is created +- **Then** an `IllegalArgumentException` is thrown + +## Typed listeners + +### The resolvable type carries the DTO type + +- **Given** an `OnObjectUpdate` for a `BookDto` +- **When** `getResolvableType()` is called +- **Then** the result is `OnObjectUpdate` + +### A subclass that fixes the type parameter still resolves + +- **Given** an application event `class BookUpdated extends OnObjectUpdate` +- **When** `getResolvableType()` is called on an instance +- **Then** the result is `BookUpdated` and no exception is thrown + +### A typed listener ignores events for other DTO types [context] + +- **Given** a listener method `onBook(OnObjectUpdate)` +- **When** an `OnObjectUpdate` is published +- **Then** `onBook` is not invoked + +### A typed listener receives events for its DTO type [context] + +- **Given** a listener method `onBook(OnObjectUpdate)` +- **When** an `OnObjectUpdate` is published +- **Then** `onBook` is invoked once with that event + +### A listener on the base type receives every DTO type [context] + +- **Given** a listener method `onAny(GenericDataEvent)` +- **When** an `OnObjectUpdate` and an `OnObjectCreate` are published +- **Then** `onAny` is invoked for both + +## `AbstractDbBackedDataService` + +### An insert publishes a create event + +- **When** `save(dto)` is called with a DTO without id +- **Then** exactly one `OnObjectCreate` carrying the saved DTO is published + +### An update publishes previous and current state + +- **Given** a persisted entity whose name is `"old"` +- **When** `save(dto)` is called with the same id and name `"new"` +- **Then** exactly one `OnObjectUpdate` is published +- **And** its `getPrevious()` has name `"old"` +- **And** its `getData()` has name `"new"` + +### An update that changes nothing publishes no event + +- **Given** a persisted entity whose name is `"same"` +- **When** `save(dto)` is called with the same id and name `"same"` +- **Then** no event is published +- **And** the saved DTO is returned as usual + +### Events stay suppressed when publishing is disabled + +- **Given** a service constructed with `publishEvents == false` +- **When** an insert, a real update and a delete are performed +- **Then** no event is published + +## `AbstractMultitenantDbBackedDataService` + +### An update publishes previous and current state + +- **Given** a persisted entity of the current tenant whose name is `"old"` +- **When** `save(dto)` is called with the same id and name `"new"` +- **Then** exactly one `OnObjectUpdate` is published with previous name `"old"` and current name + `"new"` + +### An update that changes nothing publishes no event + +- **Given** a persisted entity of the current tenant whose name is `"same"` +- **When** `save(dto)` is called with the same id and name `"same"` +- **Then** no event is published diff --git a/docs/specs/022-update-event-previous-state/design.md b/docs/specs/022-update-event-previous-state/design.md new file mode 100644 index 0000000..2981f2a --- /dev/null +++ b/docs/specs/022-update-event-previous-state/design.md @@ -0,0 +1,223 @@ +# Design: Previous state on update events + +## GitHub Issue + +— (origin: [open-tasks#36](https://github.com/OpenElementsLabs/open-tasks/pull/36), which built a +task-specific version of this feature and marked its `change` package as a candidate for +`spring-services`) + +## Summary + +`OnObjectUpdate` carries only the post-update DTO; its Javadoc says so explicitly — *"The previous +state is not available — listeners that need to compute a diff have to load it themselves."* By the +time a listener runs, it cannot: the row is already updated. open-tasks worked around this with a +hand-written `TaskSnapshot` taken before every `save(...)` and a dedicated `TaskChangedEvent`. + +This spec closes the gap in the existing event API instead of next to it. `OnObjectUpdate` learns to +carry the state **before** the update when the publisher knows it, and says honestly when it does +not. `AbstractDbBackedDataService` (and its tenant-aware sibling) capture that state automatically, +and stop publishing updates that changed nothing. What a change *means* — "newly assigned", "moved +column", "description edited" — stays in the application's listener. + +Along the way a latent bug in the existing API is fixed: typed listeners such as +`@EventListener OnObjectUpdate` currently receive events for **every** DTO type. + +## Goals + +- A listener can compare the state before and after an update without loading anything. +- Publishers that cannot know the previous state — an external system, a webhook, a projection — + keep publishing `OnObjectUpdate` and are first-class citizens, not a degraded special case. +- A listener can always tell the two situations apart; "previous unknown" never looks like + "nothing changed". +- Updates that change nothing do not produce an event. +- Typed listeners receive only events of their own DTO type, as the package Javadoc already promises. + +## Non-goals + +- **No diff in the event.** No `PropertyChange`, no `getChanges()`. Comparing two DTOs is a one-liner + for the listener that needs it and the meaning of a change is domain knowledge. Keeping it out + keeps the event a plain value carrier (decided in review of the first draft). +- **No actor in the event.** Who made the change is resolved by the listener through `AuthService`, + exactly as `AuditLogEventListener` does — it runs on the publishing thread, also for + `@TransactionalEventListener(AFTER_COMMIT)`. +- **No notification delivery, debounce or recipient logic.** That is open-tasks' `notification` + package and stays application code. +- **No new event type** for "a related object changed" (open-tasks' `COMMENT` change). A new comment + already publishes `OnObjectCreate`; the application maps it. + +## Technical approach + +### `OnObjectUpdate` + +```java +public class OnObjectUpdate extends GenericDataEvent { + + /** Previous state unknown — e.g. the data comes from an external system. */ + public OnObjectUpdate(@NonNull T current) + + /** Previous state known. Both DTOs must be of the same class and carry the same id. */ + public OnObjectUpdate(@NonNull T previous, @NonNull T current) + + public boolean isPreviousKnown() + @NonNull public Optional getPrevious() +} +``` + +- The one-argument constructor is unchanged; every existing publisher keeps compiling and behaving + as before (`isPreviousKnown() == false`). +- The two-argument constructor rejects `null`, a `previous` of a different class + (`IllegalArgumentException`) and a `previous` with a different id (`IllegalArgumentException`) — + an update event comparing two different objects is a programming error, not data. +- The event does **not** decide whether something changed. A publisher may send an event whose + `previous` equals `current`; suppressing no-ops is the data service's job (below), because only it + knows the event comes from a save. + +### `AbstractDbBackedDataService` + +```java +final E entity = getRepository().findById(data.id()).orElseThrow(...); +final D previous = toData(entity); // snapshot BEFORE updateEntity mutates the entity +updateEntity(entity, data); +final D saved = toData(getRepository().save(entity)); +postUpdate(previous, saved); +``` + +The hooks change from `postSave(boolean insert, D data)` to two methods: + +```java +protected void postInsert(@NonNull D data) // publishes OnObjectCreate +protected void postUpdate(@NonNull D previous, @NonNull D current) // publishes OnObjectUpdate(previous, current) +``` + +`postUpdate` publishes nothing when `previous.equals(current)` (logged at `DEBUG`) or when the +service was constructed with `publishEvents == false`. + +`AbstractMultitenantDbBackedDataService` gets the same snapshot, the same two hooks and the same +no-op rule. Its update branch also loses a duplicated `getRepository().save(entity)` call. + +### `GenericDataEvent` implements `ResolvableTypeProvider` + +```java +@Override +public ResolvableType getResolvableType() { + return ResolvableType.forClassWithGenerics(getClass(), ResolvableType.forClass(getType())); +} +``` + +Without it Spring cannot see `T` on an event instance (erasure) and treats the unresolvable generic +as a wildcard, so `@EventListener OnObjectUpdate` is invoked for an +`OnObjectUpdate` too — and fails with a `ClassCastException` on `getData()`. Verified with +a probe test before this spec was written. `forClassWithGenerics` requires the event class to declare +exactly one type parameter, which holds for all three concrete events; an application subclass that +fixes `T` (`class BookUpdated extends OnObjectUpdate`) has none, so the provider falls back +to `ResolvableType.forClass(getClass())` in that case. + +```mermaid +sequenceDiagram + participant S as Data service + participant E as Entity + participant P as ApplicationEventPublisher + participant L as Listener + S->>E: findById + S->>S: previous = toData(entity) + S->>E: updateEntity(entity, dto) + S->>S: current = toData(save(entity)) + alt previous.equals(current) + S-->>S: no event + else changed + S->>P: OnObjectUpdate(previous, current) + P->>L: only listeners for this DTO type + L->>L: compare getPrevious() with getData() + end +``` + +## Design decisions and rationale + +### D1: `Optional` plus `isPreviousKnown()`, not a nullable getter or a subclass + +`Optional getPrevious()` makes the unknown case impossible to overlook; `isPreviousKnown()` reads +better in a guard clause than `getPrevious().isPresent()`. A subclass (`OnObjectUpdateWithPrevious`) +was considered and rejected: listeners for `OnObjectUpdate` would still get both, so they would +need an `instanceof` — the same check, with a second public type to learn. + +### D2: No diff in the event + +A first draft carried `List getChanges()` computed reflectively over record +components. It was dropped: it ties the event API to records, adds reflection and a JPMS `opens` +requirement, and still could not express what a change means. A listener that needs per-property +changes compares `previous.title()` with `current.title()` — typed, without reflection. + +### D3: No-op updates are suppressed in the data service, by `equals` + +An update that changes nothing is noise for every listener — audit log, webhooks, notifications. +`equals` is the right comparison because it is what the DTO declares as identity of state; for +records it is a deep comparison for free. A non-record DTO without `equals` falls back to identity, +so its two snapshots are never equal and the event is always published — the safe direction. + +Audit timestamps are not a problem: Hibernate only advances `@UpdateTimestamp` on a dirty entity, so +a genuine no-op produces equal DTOs. + +### D4: `postSave(boolean, D)` is replaced, not kept + +The update path now needs the previous state, which `postSave(false, data)` cannot receive. Keeping +`postSave` but no longer calling it for updates would silently stop running subclass overrides; keeping +it and also calling `postUpdate` would publish twice or let overrides that suppress events stop +working. Removing it makes every affected subclass fail to compile at its `@Override` — a loud +break, documented in the 1.5.0 upgrade guide, instead of a silent one. + +### D5: `toData` must return a snapshot + +`previous` is produced from the managed entity before `updateEntity` mutates it. If `toData` hands a +mutable collection of the entity to the DTO (instead of `List.copyOf(...)`), `previous` changes with +the entity, compares equal to `current`, and the update is swallowed by D3. This is the reason +open-tasks needed `TaskSnapshot`. The rule goes into the Javadoc of `toData` and of `OnObjectUpdate`; +all `toData` implementations in this repository are flat records of immutable values and comply. + +### D6: The generics fix ships with this spec + +The feature is only useful through typed listeners, and those are broken today. The fix is one +method and does not change any listener that already filters by `getType()`. + +## Security considerations + +`previous` holds the same fields as `current`; nothing reaches a listener that it could not already +see. Applications that forward events outward (webhooks, notifications) must decide which fields of +`previous` to include — open-tasks deliberately omits description content. `WebhookEventListener` +serialises only `getData()` and is unchanged. + +## Data protection (GDPR / DSGVO) + +The previous state may contain personal data that the update just removed (an old e-mail address). +It lives only in memory for the duration of event dispatch and is not persisted by the library. A +listener that stores it is responsible for that copy. + +## Testing strategy + +- Unit tests for `OnObjectUpdate`: both constructors, rejection of `null`, foreign class and foreign + id; `getPrevious()` / `isPreviousKnown()` in both cases. +- Unit test for `GenericDataEvent.getResolvableType()` and a Spring-context test proving a typed + listener ignores events for other DTO types (the probe that found the bug, made permanent). +- Unit tests for `AbstractDbBackedDataService`: update publishes previous and current; no-op update + publishes nothing; `publishEvents == false` still publishes nothing. +- The same update / no-op pair for `AbstractMultitenantDbBackedDataService`. +- Existing integration tests (audit log, webhooks, `AbstractDbBackedDataServicePublishEventsTest`) + keep passing; the latter is adjusted where it relied on an update without changes. + +## Dependencies + +None. `ResolvableTypeProvider` is part of `spring-core`. + +## Migration / impact on existing applications + +- **Source break** for subclasses of `AbstractDbBackedDataService` / + `AbstractMultitenantDbBackedDataService` that override `postSave(boolean, D)`: split into + `postInsert(D)` and `postUpdate(D, D)`. +- **Behaviour change:** an update that changes nothing no longer publishes `OnObjectUpdate`, so it no + longer creates an audit row or fires a webhook. +- **Behaviour change (bug fix):** typed listeners stop receiving events for other DTO types. + Listeners on `GenericDataEvent` or a raw type are unaffected. +- No schema change, no configuration. + +## Open questions + +None. diff --git a/docs/specs/INDEX.md b/docs/specs/INDEX.md index 072f8e9..6a8d982 100644 --- a/docs/specs/INDEX.md +++ b/docs/specs/INDEX.md @@ -23,3 +23,4 @@ | 019 | 019-authentication-type | Authentication type probe | backend, security, api | Additive `AuthService` pair for authentication *state*: a new `AuthenticationType` enum (`JWT_ACCOUNT`, `API_KEY`, `ANONYMOUS`, `OTHER`, `NONE`) via `getAuthenticationType()`, plus `isAuthenticated()` delegating to Spring Security's `AuthenticationTrustResolver` so it cannot disagree with `@PreAuthorize("isAuthenticated()")` in the same app. Closes the trap that `findAuthentication().isPresent()` is `true` on `permitAll` paths. `JWT_ACCOUNT` is mechanism-named because `principal instanceof Jwt` is the only reliable discriminator (Spring has no mechanism-independent user type); API keys are recognised by their `ApiKeyEntity` principal, never by the `ROLE_API_KEY` authority. SCIM stays `OTHER` (a dedicated constant would need a token type `core` can see — parked in TODO). Fail-closed, no new bean, no configuration; Javadoc mandates allow-list comparisons and declares the constant set extensible. Classification proven through the real chains, the SCIM one in `spring-services-all`. | — | open | | 020 | 020-token-exchange | Token exchange (RFC 8693) | backend, security, api, architecture | Delegated service-to-service calls made explicit. **Inbound** (`core`, no new dependency): `CallerOrigin` (`DIRECT`/`DELEGATED`/`UNKNOWN`) plus `AuthService.getCallerOrigin()`, `findCallerClientId()` and `findActorSubject()` — the discriminator is `azp`/`client_id` (RFC 9068 requires it) so the same code works on Authentik and Keycloak, while RFC 8693's `act` claim is used when present (Authentik 2026.8+) and never required. `UNKNOWN` covers both "no client id" and "own-client-id unconfigured"; Javadoc mandates allow-list comparisons. **Outbound** (new optional module `spring-services-token-exchange`): wires Spring Security's existing `TokenExchangeOAuth2AuthorizedClientProvider` and adds the `audience` parameter, which `TokenExchangeGrantRequest.defaultParameters(...)` does not send — the only real gap. Plus per-IdP setup docs. Replaces plain token forwarding; supersedes PAT (spec 010, parked) for the online case. Notably **not** in scope: enforcing `aud` validation, which stays a `docs/TODO.md` item marked important — until it lands the distinction is a label, not a control. Verified against real Authentik/Keycloak before merge. | — | open | | 021 | 021-db-health-check | Database reachability check | backend, database | One read-only bean in `spring-services-core`: `DbHealthService.isDatabaseReachable()` borrows a pooled connection and executes `SELECT 1`, so an application whose database died after startup reports the truth instead of the cached "up" a bean-level check would give. Never throws — `SQLException` **and** `RuntimeException` (a pool's connection-acquisition timeout) are caught, logged at `WARN` with the stack trace at `DEBUG`, and reported as `false`. No caching, and the cost is documented rather than hidden: each call occupies one pooled connection and blocks up to the pool timeout against a saturated pool. Registered by an explicit `@Bean` in the new `DataConfig` (not a `@ComponentScan`, so it cannot be registered twice) and imported by `FullSpringServiceConfig`. No endpoint, no status enum, no Actuator — path, authorization and response shape stay with the application. **Spec written retroactively** for the implementation in `f0066b7`. | — | done | +| 022 | 022-update-event-previous-state | Previous state on update events | backend, api, architecture | Extends the existing `events` API instead of adding a parallel one (origin: open-tasks#36 `TaskChangedEvent`). `OnObjectUpdate` gets a second constructor `(previous, current)` plus `isPreviousKnown()` / `Optional getPrevious()`; the one-arg constructor stays for publishers that cannot know the previous state (external systems). No diff and no actor in the event — listeners compare and resolve the actor themselves. `AbstractDbBackedDataService` and the tenant variant snapshot `toData(entity)` before `updateEntity`, replace `postSave(boolean, D)` with `postInsert(D)` / `postUpdate(D, D)` (loud source break) and no longer publish updates where `previous.equals(current)`. Also fixes typed listeners receiving events for every DTO type by implementing `ResolvableTypeProvider` on `GenericDataEvent`. | — | done | diff --git a/spring-services-core/src/main/java/com/openelements/spring/base/data/AbstractDbBackedDataService.java b/spring-services-core/src/main/java/com/openelements/spring/base/data/AbstractDbBackedDataService.java index dc3b31c..1114bb1 100644 --- a/spring-services-core/src/main/java/com/openelements/spring/base/data/AbstractDbBackedDataService.java +++ b/spring-services-core/src/main/java/com/openelements/spring/base/data/AbstractDbBackedDataService.java @@ -24,7 +24,8 @@ *
  • Distinguishing inserts from updates based on the DTO's id. *
  • Wrapping all mutating operations in a Spring {@link Transactional} boundary. *
  • Publishing {@link OnObjectCreate}, {@link OnObjectUpdate} and {@link OnObjectDelete} events - * so other components can react asynchronously to data changes. + * so other components can react asynchronously to data changes. Update events carry the state + * before and after the update; an update that changes nothing publishes no event. * * *

    Subclass responsibilities

    @@ -41,11 +42,11 @@ * *

    Hook points

    * - *

    Subclasses may override the {@code preSave} / {@code postSave} / {@code preDelete} / {@code - * postDelete} methods to implement additional validation, projection updates or custom event - * publishing. When overriding {@code postSave} or {@code postDelete}, remember to delegate to - * {@code super} (or to publish equivalent events) if the standard lifecycle events should still be - * fired. + *

    Subclasses may override the {@code preSave} / {@code postInsert} / {@code postUpdate} / {@code + * preDelete} / {@code postDelete} methods to implement additional validation, projection updates or + * custom event publishing. When overriding {@code postInsert}, {@code postUpdate} or {@code + * postDelete}, remember to delegate to {@code super} (or to publish equivalent events) if the + * standard lifecycle events should still be fired. * *

    Example

    * @@ -135,6 +136,12 @@ public AbstractDbBackedDataService( /** * Maps a persisted entity to its DTO representation. * + *

    The returned DTO must be a snapshot: it must not share mutable state (such as a collection) + * with the entity. On update, this method is called once before {@link #updateEntity(DbEntity, + * WithId)} to capture the previous state; a DTO that still references the entity's collections + * would change along with it, compare equal to the updated DTO, and the update event would be + * suppressed. Copy collections with {@code List.copyOf(...)} or {@code Set.copyOf(...)}. + * * @param entity the persisted entity * @return the corresponding DTO */ @@ -180,30 +187,46 @@ protected void postDelete(@NonNull final D data) { } /** - * Hook invoked after a successful save. Default implementation publishes an {@link - * OnObjectCreate} event for inserts and an {@link OnObjectUpdate} event for updates. + * Hook invoked after a successful insert. Default implementation publishes an {@link + * OnObjectCreate} event. * - * @param insert {@code true} if the operation was an insert, {@code false} for an update - * @param data the DTO that was just persisted + * @param data the DTO that was just inserted */ - protected void postSave(final boolean insert, @NonNull final D data) { + protected void postInsert(@NonNull final D data) { Objects.requireNonNull(data, "data must not be null"); if (!publishEvents) { return; } - if (insert) { - eventPublisher.publishEvent(new OnObjectCreate<>(data)); - } else { - eventPublisher.publishEvent(new OnObjectUpdate<>(data)); + eventPublisher.publishEvent(new OnObjectCreate<>(data)); + } + + /** + * Hook invoked after a successful update. Default implementation publishes an {@link + * OnObjectUpdate} event carrying both states — unless they are {@linkplain Object#equals(Object) + * equal}, in which case the update changed nothing and no event is published. + * + * @param previous the DTO as it was before the update + * @param current the DTO that was just persisted + */ + protected void postUpdate(@NonNull final D previous, @NonNull final D current) { + Objects.requireNonNull(previous, "previous must not be null"); + Objects.requireNonNull(current, "current must not be null"); + if (!publishEvents) { + return; + } + if (previous.equals(current)) { + LOG.debug("Update of {} changed nothing, no event published", current); + return; } + eventPublisher.publishEvent(new OnObjectUpdate<>(previous, current)); } /** * {@inheritDoc} * *

    Performs an upsert: a new entity is created when the DTO has no id; otherwise the existing - * entity is loaded, updated and saved. The matching {@code preSave} / {@code postSave} hooks are - * invoked before and after the database round-trip. + * entity is loaded, updated and saved. {@code preSave} is invoked before the database round-trip, + * {@code postInsert} or {@code postUpdate} after it. * * @throws IllegalArgumentException if the DTO carries an id but no entity with that id exists */ @@ -218,7 +241,7 @@ public D save(@NonNull final D data) { updateEntity(entity, data); final D saved = toData(getRepository().save(entity)); LOG.debug("New entity inserted: {}", saved); - postSave(true, saved); + postInsert(saved); return saved; } else { LOG.debug("Update entity: {}", data); @@ -227,10 +250,11 @@ public D save(@NonNull final D data) { .findById(data.id()) .orElseThrow( () -> new IllegalArgumentException("Entity with id " + data.id() + " not found")); + final D previous = toData(entity); updateEntity(entity, data); final D saved = toData(getRepository().save(entity)); LOG.debug("entity updated: {}", data); - postSave(false, saved); + postUpdate(previous, saved); return saved; } } diff --git a/spring-services-core/src/main/java/com/openelements/spring/base/data/package-info.java b/spring-services-core/src/main/java/com/openelements/spring/base/data/package-info.java index 0501fd0..1a7eefa 100644 --- a/spring-services-core/src/main/java/com/openelements/spring/base/data/package-info.java +++ b/spring-services-core/src/main/java/com/openelements/spring/base/data/package-info.java @@ -32,13 +32,14 @@ *

  • {@link com.openelements.spring.base.events.OnObjectCreate} — published after a newly * inserted entity has been persisted. *
  • {@link com.openelements.spring.base.events.OnObjectUpdate} — published after an existing - * entity has been updated. + * entity has been updated and carries its state before and after; an update that changes + * nothing publishes no event. *
  • {@link com.openelements.spring.base.events.OnObjectDelete} — published after an entity has * been removed. * * - *

    Subclasses can hook in by overriding the {@code preSave}, {@code postSave}, {@code preDelete} - * and {@code postDelete} template methods. + *

    Subclasses can hook in by overriding the {@code preSave}, {@code postInsert}, {@code + * postUpdate}, {@code preDelete} and {@code postDelete} template methods. * *

    Example: implementing a data service for a {@code Book} domain object

    * diff --git a/spring-services-core/src/main/java/com/openelements/spring/base/events/GenericDataEvent.java b/spring-services-core/src/main/java/com/openelements/spring/base/events/GenericDataEvent.java index 6f48b31..d6bf510 100644 --- a/spring-services-core/src/main/java/com/openelements/spring/base/events/GenericDataEvent.java +++ b/spring-services-core/src/main/java/com/openelements/spring/base/events/GenericDataEvent.java @@ -6,6 +6,8 @@ import org.jspecify.annotations.NonNull; import org.jspecify.annotations.Nullable; import org.springframework.context.ApplicationEvent; +import org.springframework.core.ResolvableType; +import org.springframework.core.ResolvableTypeProvider; /** * Base class for all lifecycle events fired by the platform's data services. @@ -16,13 +18,19 @@ * parameter at runtime — without an explicit class reference there is no way to ask "is this an * event about a {@code BookDto}?". * + *

    The captured type is also reported to Spring through {@link ResolvableTypeProvider}, so that a + * listener declared for {@code OnObjectUpdate} is only invoked for book events. Without it, + * Spring could not resolve the type parameter of an event instance and would dispatch every {@code + * OnObjectUpdate} to that listener. + * *

    Application code should normally not extend this class directly — use one of the concrete * subtypes {@link OnObjectCreate}, {@link OnObjectUpdate} or {@link OnObjectDelete} so that * listeners can distinguish the kind of change that occurred. * * @param the DTO type this event refers to */ -public abstract class GenericDataEvent extends ApplicationEvent { +public abstract class GenericDataEvent extends ApplicationEvent + implements ResolvableTypeProvider { /** The runtime DTO type this event refers to, captured to survive generic type erasure. */ private final Class type; @@ -89,4 +97,22 @@ public Class getType() { public UUID entityId() { return getData().id(); } + + /** + * Returns the full generic type of this event, e.g. {@code OnObjectUpdate}, so that + * Spring dispatches it only to listeners declared for the matching DTO type. + * + *

    A subclass that fixes the type parameter itself (such as {@code class BookUpdated extends + * OnObjectUpdate}) already carries it in its class signature and is returned as is. + * + * @return the resolvable type of this event + */ + @NonNull + @Override + public ResolvableType getResolvableType() { + if (getClass().getTypeParameters().length != 1) { + return ResolvableType.forClass(getClass()); + } + return ResolvableType.forClassWithGenerics(getClass(), type); + } } diff --git a/spring-services-core/src/main/java/com/openelements/spring/base/events/OnObjectUpdate.java b/spring-services-core/src/main/java/com/openelements/spring/base/events/OnObjectUpdate.java index c57d616..9c7d8d1 100644 --- a/spring-services-core/src/main/java/com/openelements/spring/base/events/OnObjectUpdate.java +++ b/spring-services-core/src/main/java/com/openelements/spring/base/events/OnObjectUpdate.java @@ -1,25 +1,98 @@ package com.openelements.spring.base.events; import com.openelements.spring.base.data.WithId; +import java.util.Objects; +import java.util.Optional; import org.jspecify.annotations.NonNull; +import org.jspecify.annotations.Nullable; /** - * Lifecycle event published after an existing entity has been updated by a data service. + * Lifecycle event published after an existing entity has been updated. * - *

    The event carries the DTO in its post-update state, i.e. with the new field values - * already applied. The previous state is not available — listeners that need to compute a diff have - * to load it themselves. + *

    The event always carries the DTO in its post-update state ({@link #getData()}). + * Whether the state before the update is available depends on the publisher: + * + *

      + *
    • {@link com.openelements.spring.base.data.AbstractDbBackedDataService} captures it for every + * update and publishes the event only if something actually changed. + *
    • Publishers that cannot know the previous state — for example because the data comes from + * an external system — use the one-argument constructor. {@link #isPreviousKnown()} is then + * {@code false}. + *
    + * + *

    The event does not compute a diff. Listeners that care about individual properties compare + * {@link #getPrevious()} with {@link #getData()} themselves: + * + * 

    {@code
+ * @EventListener
+ * public void onBookUpdated(OnObjectUpdate event) {
+ *   event.getPrevious()
+ *       .filter(previous -> !previous.title().equals(event.getData().title()))
+ *       .ifPresent(previous -> notifyTitleChanged(previous.title(), event.getData().title()));
+ * }
+ * }
    + * + *

    Both DTOs are snapshots: they must not share mutable state (such as a collection) with the + * entity they were created from, otherwise the previous state would change along with the entity. * * @param the DTO type that was just updated */ public class OnObjectUpdate extends GenericDataEvent { + /** The state before the update, or {@code null} if the publisher does not know it. */ + @Nullable private final T previous; + + /** + * Creates a new update event whose previous state is unknown. + * + * @param current the updated DTO in its post-update state + */ + public OnObjectUpdate(@NonNull final T current) { + super(current); + this.previous = null; + } + + /** + * Creates a new update event that carries the state before and after the update. + * + * @param previous the DTO in its pre-update state + * @param current the updated DTO in its post-update state + * @throws NullPointerException if {@code previous} or {@code current} is {@code null} + * @throws IllegalArgumentException if both DTOs are of different classes or carry different ids + */ + public OnObjectUpdate(@NonNull final T previous, @NonNull final T current) { + super(current); + Objects.requireNonNull(previous, "previous must not be null"); + if (!previous.getClass().equals(current.getClass())) { + throw new IllegalArgumentException( + "previous is a " + + previous.getClass().getName() + + " but current is a " + + current.getClass().getName()); + } + if (!Objects.equals(previous.id(), current.id())) { + throw new IllegalArgumentException( + "previous has id " + previous.id() + " but current has id " + current.id()); + } + this.previous = previous; + } + + /** + * Returns whether the publisher supplied the state before the update. + * + * @return {@code true} if {@link #getPrevious()} is present + */ + public boolean isPreviousKnown() { + return previous != null; + } + /** - * Creates a new update event for the given DTO. + * Returns the DTO in its pre-update state, if the publisher knows it. * - * @param source the updated DTO in its post-update state + * @return the previous state, or an empty {@link Optional} if it is unknown */ - public OnObjectUpdate(@NonNull T source) { - super(source); + @NonNull + public Optional getPrevious() { + return Optional.ofNullable(previous); } } diff --git a/spring-services-core/src/main/java/com/openelements/spring/base/events/package-info.java b/spring-services-core/src/main/java/com/openelements/spring/base/events/package-info.java index f6d02b4..52cc45f 100644 --- a/spring-services-core/src/main/java/com/openelements/spring/base/events/package-info.java +++ b/spring-services-core/src/main/java/com/openelements/spring/base/events/package-info.java @@ -13,7 +13,9 @@ *

  • {@link com.openelements.spring.base.events.OnObjectCreate} — published after a new entity * has been inserted. *
  • {@link com.openelements.spring.base.events.OnObjectUpdate} — published after an existing - * entity has been updated. + * entity has been updated. It carries the state before the update when the publisher knows it + * (see {@link com.openelements.spring.base.events.OnObjectUpdate#isPreviousKnown()}); the data + * services always supply it and publish no event for an update that changed nothing. *
  • {@link com.openelements.spring.base.events.OnObjectDelete} — published after an entity has * been deleted. * @@ -45,6 +47,19 @@ * } * } * + *

    A listener is only invoked for events of the DTO type it declares — the events report their + * full generic type to Spring. To react to a specific change, compare the previous with the current + * state: + * + * 

    {@code
+ * @EventListener
+ * public void onBookUpdated(OnObjectUpdate event) {
+ *   event.getPrevious()
+ *       .filter(previous -> !previous.author().equals(event.getData().author()))
+ *       .ifPresent(previous -> reindexAuthor(event.getData()));
+ * }
+ * }
    + * *

    To react to any data change regardless of type, listen to {@link * com.openelements.spring.base.events.GenericDataEvent} directly and inspect {@link * com.openelements.spring.base.events.GenericDataEvent#getType()}. diff --git a/spring-services-core/src/test/java/com/openelements/spring/base/data/AbstractDbBackedDataServicePublishEventsTest.java b/spring-services-core/src/test/java/com/openelements/spring/base/data/AbstractDbBackedDataServicePublishEventsTest.java index a8d1652..51dc25b 100644 --- a/spring-services-core/src/test/java/com/openelements/spring/base/data/AbstractDbBackedDataServicePublishEventsTest.java +++ b/spring-services-core/src/test/java/com/openelements/spring/base/data/AbstractDbBackedDataServicePublishEventsTest.java @@ -8,11 +8,14 @@ import com.openelements.spring.base.events.OnObjectDelete; import com.openelements.spring.base.events.OnObjectUpdate; import jakarta.persistence.Entity; +import java.util.HashMap; +import java.util.Map; import java.util.Optional; import java.util.UUID; import org.jspecify.annotations.NonNull; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; import org.springframework.context.ApplicationEventPublisher; /** @@ -24,15 +27,18 @@ * com.openelements.spring.base.events.OnObjectCreate}, {@link * com.openelements.spring.base.events.OnObjectUpdate} and {@link * com.openelements.spring.base.events.OnObjectDelete} on the {@link ApplicationEventPublisher} - * during {@code save(...)} and {@code delete(...)}. Two scenarios are verified: default - * behaviour (events are emitted) and opt-out behaviour (no events at all). + * during {@code save(...)} and {@code delete(...)}. Verified are: default behaviour (events are + * emitted), opt-out behaviour (no events at all), an update carrying the state before and after, + * and an update that changes nothing (no event). * *

    How it is tested

    * *

    Mockito-based unit test. Two collaborators are mocked: {@link ApplicationEventPublisher} * (verified via {@code verify(publisher).publishEvent(any(...))}) and {@link EntityRepository} - * (stubbed to assign a UUID on first save and echo the entity back on subsequent calls — the - * helper {@code repositoryThatPersists()} encapsulates this in-memory pseudo-persistence). + * (backed by an in-memory map that assigns a UUID on first save and hands out the same managed + * instance on {@code findById} — the helper {@code repositoryThatPersists()} encapsulates this + * pseudo-persistence, so {@code updateEntity} mutates the instance the previous state was taken + * from, exactly like a JPA persistence context). * *

    An anonymous {@code FakeService extends AbstractDbBackedDataService} stands in for any * real subclass so the test does not couple to a domain entity. The flag itself is the unit @@ -47,6 +53,7 @@ class AbstractDbBackedDataServicePublishEventsTest { @SuppressWarnings("unchecked") private static EntityRepository repositoryThatPersists() { + final Map store = new HashMap<>(); final EntityRepository repository = mock(EntityRepository.class); when(repository.save(any(TestEntity.class))) .thenAnswer( @@ -55,15 +62,11 @@ private static EntityRepository repositoryThatPersists() { if (entity.getId() == null) { entity.setId(UUID.randomUUID()); } + store.put(entity.getId(), entity); return entity; }); when(repository.findById(any(UUID.class))) - .thenAnswer( - invocation -> { - final TestEntity entity = new TestEntity(); - entity.setId(invocation.getArgument(0)); - return Optional.of(entity); - }); + .thenAnswer(invocation -> Optional.ofNullable(store.get(invocation.getArgument(0)))); return repository; } @@ -77,8 +80,8 @@ void shouldPublishEventsByDefault() { final FakeService service = new FakeService(publisher, repositoryThatPersists()); // WHEN - final TestDto saved = service.save(new TestDto(null)); - service.save(new TestDto(saved.id())); + final TestDto saved = service.save(new TestDto(null, "old")); + service.save(new TestDto(saved.id(), "new")); service.delete(saved); // THEN @@ -97,8 +100,8 @@ void shouldSuppressEventsWhenFlagIsFalse() { final FakeService service = new FakeService(publisher, false, repositoryThatPersists()); // WHEN - final TestDto saved = service.save(new TestDto(null)); - service.save(new TestDto(saved.id())); + final TestDto saved = service.save(new TestDto(null, "old")); + service.save(new TestDto(saved.id(), "new")); service.delete(saved); // THEN @@ -106,10 +109,55 @@ void shouldSuppressEventsWhenFlagIsFalse() { assertThat(saved.id()).isNotNull(); } + @Test + @DisplayName( + "An update publishes one OnObjectUpdate whose previous state is the DTO before the update " + + "and whose data is the DTO after it.") + @SuppressWarnings("unchecked") + void shouldPublishPreviousAndCurrentStateOnUpdate() { + // GIVEN + final ApplicationEventPublisher publisher = mock(ApplicationEventPublisher.class); + final FakeService service = new FakeService(publisher, repositoryThatPersists()); + final TestDto saved = service.save(new TestDto(null, "old")); + clearInvocations(publisher); + + // WHEN + service.save(new TestDto(saved.id(), "new")); + + // THEN + final ArgumentCaptor> captor = + ArgumentCaptor.forClass(OnObjectUpdate.class); + verify(publisher).publishEvent(captor.capture()); + final OnObjectUpdate event = captor.getValue(); + assertThat(event.getPrevious()).contains(new TestDto(saved.id(), "old")); + assertThat(event.getData()).isEqualTo(new TestDto(saved.id(), "new")); + } + + @Test + @DisplayName( + "An update that changes nothing publishes no event but still returns the saved DTO.") + void shouldNotPublishOnNoOpUpdate() { + // GIVEN + final ApplicationEventPublisher publisher = mock(ApplicationEventPublisher.class); + final FakeService service = new FakeService(publisher, repositoryThatPersists()); + final TestDto saved = service.save(new TestDto(null, "same")); + clearInvocations(publisher); + + // WHEN + final TestDto result = service.save(new TestDto(saved.id(), "same")); + + // THEN + verify(publisher, never()).publishEvent(any()); + assertThat(result).isEqualTo(saved); + } + @Entity - static class TestEntity extends AbstractEntity {} + static class TestEntity extends AbstractEntity { - record TestDto(UUID id) implements WithId {} + private String name; + } + + record TestDto(UUID id, String name) implements WithId {} private static class FakeService extends AbstractDbBackedDataService { @@ -136,11 +184,13 @@ private static class FakeService extends AbstractDbBackedDataService { + TestDataUpdated(final TestData current) { + super(current); + } + } + @Nested @DisplayName("OnObjectCreate") class OnObjectCreateTest { @@ -78,6 +87,113 @@ void shouldStoreSourceAndDeriveType() { assertThat(event.getSource()).isSameAs(data); assertThat(event.getType()).isEqualTo(TestData.class); } + + @Test + @DisplayName("Without a previous state, isPreviousKnown() is false and getPrevious() is empty.") + void shouldReportUnknownPreviousState() { + // GIVEN + final TestData current = new TestData(UUID.randomUUID(), "updated"); + + // WHEN + final OnObjectUpdate event = new OnObjectUpdate<>(current); + + // THEN + assertThat(event.getData()).isSameAs(current); + assertThat(event.isPreviousKnown()).isFalse(); + assertThat(event.getPrevious()).isEmpty(); + } + + @Test + @DisplayName("With a previous state, the event carries both states.") + void shouldCarryPreviousState() { + // GIVEN + final UUID id = UUID.randomUUID(); + final TestData previous = new TestData(id, "old"); + final TestData current = new TestData(id, "new"); + + // WHEN + final OnObjectUpdate event = new OnObjectUpdate<>(previous, current); + + // THEN + assertThat(event.getData()).isSameAs(current); + assertThat(event.isPreviousKnown()).isTrue(); + assertThat(event.getPrevious()).containsSame(previous); + } + + @Test + @DisplayName("A previous state equal to the current state is accepted — no-op filtering is not the event's job.") + void shouldAcceptEqualPreviousState() { + // GIVEN + final UUID id = UUID.randomUUID(); + + // WHEN + final OnObjectUpdate event = + new OnObjectUpdate<>(new TestData(id, "same"), new TestData(id, "same")); + + // THEN + assertThat(event.isPreviousKnown()).isTrue(); + } + + @Test + @DisplayName("A null previous state is rejected with NullPointerException.") + void shouldRejectNullPreviousState() { + final TestData current = new TestData(UUID.randomUUID(), "new"); + assertThatThrownBy(() -> new OnObjectUpdate<>(null, current)) + .isInstanceOf(NullPointerException.class); + } + + @Test + @DisplayName("A previous state with a different id is rejected with IllegalArgumentException.") + void shouldRejectPreviousStateWithDifferentId() { + final TestData previous = new TestData(UUID.randomUUID(), "old"); + final TestData current = new TestData(UUID.randomUUID(), "new"); + assertThatThrownBy(() -> new OnObjectUpdate<>(previous, current)) + .isInstanceOf(IllegalArgumentException.class); + } + + @Test + @DisplayName("A previous state of a different class is rejected with IllegalArgumentException.") + void shouldRejectPreviousStateOfDifferentClass() { + final UUID id = UUID.randomUUID(); + final WithId previous = new OtherData(id, "old"); + final WithId current = new TestData(id, "new"); + assertThatThrownBy(() -> new OnObjectUpdate<>(previous, current)) + .isInstanceOf(IllegalArgumentException.class); + } + } + + @Nested + @DisplayName("Resolvable type") + class ResolvableTypeTest { + + @Test + @DisplayName("getResolvableType() reports the event class with the DTO type as generic.") + void shouldReportDtoTypeAsGeneric() { + // GIVEN + final OnObjectUpdate event = + new OnObjectUpdate<>(new TestData(UUID.randomUUID(), "updated")); + + // WHEN + final ResolvableType type = event.getResolvableType(); + + // THEN + assertThat(type.resolve()).isEqualTo(OnObjectUpdate.class); + assertThat(type.getGeneric(0).resolve()).isEqualTo(TestData.class); + } + + @Test + @DisplayName("A subclass that fixes the type parameter resolves to itself without failing.") + void shouldResolveSubclassWithFixedTypeParameter() { + // GIVEN + final TestDataUpdated event = new TestDataUpdated(new TestData(UUID.randomUUID(), "x")); + + // WHEN + final ResolvableType type = event.getResolvableType(); + + // THEN + assertThat(type.resolve()).isEqualTo(TestDataUpdated.class); + assertThat(type.as(OnObjectUpdate.class).getGeneric(0).resolve()).isEqualTo(TestData.class); + } } @Nested diff --git a/spring-services-core/src/test/java/com/openelements/spring/base/events/TypedEventListenerTest.java b/spring-services-core/src/test/java/com/openelements/spring/base/events/TypedEventListenerTest.java new file mode 100644 index 0000000..70a02aa --- /dev/null +++ b/spring-services-core/src/test/java/com/openelements/spring/base/events/TypedEventListenerTest.java @@ -0,0 +1,112 @@ +package com.openelements.spring.base.events; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.openelements.spring.base.data.WithId; +import java.util.ArrayList; +import java.util.List; +import java.util.UUID; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.springframework.context.annotation.AnnotationConfigApplicationContext; +import org.springframework.context.event.EventListener; + +/** + * Verifies that Spring dispatches lifecycle events only to listeners declared for the matching DTO + * type. + * + *

    What is tested

    + * + *

    Generic type erasure hides the DTO type of an event instance. Without {@link + * GenericDataEvent#getResolvableType()}, Spring treats the unresolvable type parameter as a wildcard + * and invokes a listener for {@code OnObjectUpdate} with an {@code + * OnObjectUpdate} too. These tests pin the corrected dispatch. + * + *

    How it is tested

    + * + *

    A minimal {@link AnnotationConfigApplicationContext} with one listener bean; events are + * published through the context, exactly as the data services publish them. + * + *

    Mock-Audit. Zero mocks — the unit under test is Spring's real event dispatch. + */ +@DisplayName("Typed lifecycle event listeners") +class TypedEventListenerTest { + + record BookDto(UUID id) implements WithId {} + + record AuthorDto(UUID id) implements WithId {} + + static class RecordingListener { + + final List books = new ArrayList<>(); + + final List any = new ArrayList<>(); + + @EventListener + public void onBook(final OnObjectUpdate event) { + books.add(event.getData()); + } + + @EventListener + public void onAny(final GenericDataEvent event) { + any.add(event.getData()); + } + } + + private AnnotationConfigApplicationContext context; + + private RecordingListener listener; + + @BeforeEach + void startContext() { + context = new AnnotationConfigApplicationContext(); + context.registerBean(RecordingListener.class); + context.refresh(); + listener = context.getBean(RecordingListener.class); + } + + @AfterEach + void closeContext() { + context.close(); + } + + @Test + @DisplayName("A listener for OnObjectUpdate is not invoked for an author update.") + void shouldIgnoreEventsForOtherDtoTypes() { + // WHEN + context.publishEvent(new OnObjectUpdate<>(new AuthorDto(UUID.randomUUID()))); + + // THEN + assertThat(listener.books).isEmpty(); + } + + @Test + @DisplayName("A listener for OnObjectUpdate is invoked once for a book update.") + void shouldReceiveEventsForItsDtoType() { + // GIVEN + final BookDto book = new BookDto(UUID.randomUUID()); + + // WHEN + context.publishEvent(new OnObjectUpdate<>(book)); + + // THEN + assertThat(listener.books).containsExactly(book); + } + + @Test + @DisplayName("A listener for GenericDataEvent is invoked for every event type and DTO type.") + void shouldDispatchEveryEventToWildcardListener() { + // GIVEN + final BookDto book = new BookDto(UUID.randomUUID()); + final AuthorDto author = new AuthorDto(UUID.randomUUID()); + + // WHEN + context.publishEvent(new OnObjectUpdate<>(book)); + context.publishEvent(new OnObjectCreate<>(author)); + + // THEN + assertThat(listener.any).containsExactly(book, author); + } +} diff --git a/spring-services-tenant/src/main/java/com/openelements/spring/base/tenant/AbstractMultitenantDbBackedDataService.java b/spring-services-tenant/src/main/java/com/openelements/spring/base/tenant/AbstractMultitenantDbBackedDataService.java index e6dfbb2..9713294 100644 --- a/spring-services-tenant/src/main/java/com/openelements/spring/base/tenant/AbstractMultitenantDbBackedDataService.java +++ b/spring-services-tenant/src/main/java/com/openelements/spring/base/tenant/AbstractMultitenantDbBackedDataService.java @@ -9,6 +9,8 @@ import java.util.Objects; import java.util.Optional; import java.util.UUID; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springframework.context.ApplicationEventPublisher; import org.springframework.transaction.annotation.Transactional; @@ -28,8 +30,9 @@ * * *

    As with the non-tenant variant, {@link OnObjectCreate}, {@link OnObjectUpdate} and {@link - * OnObjectDelete} events are published from the {@code preSave} / {@code postSave} / {@code - * preDelete} / {@code postDelete} hooks. + * OnObjectDelete} events are published from the {@code postInsert} / {@code postUpdate} / {@code + * postDelete} hooks. Update events carry the state before and after the update; an update that + * changes nothing publishes no event. * * @param the multitenant JPA entity type * @param the DTO type exposed by this service @@ -39,6 +42,9 @@ public abstract class AbstractMultitenantDbBackedDataService< E extends AbstractMultitenantEntity, D extends WithId> implements DbBackedDataService { + private static final Logger LOG = + LoggerFactory.getLogger(AbstractMultitenantDbBackedDataService.class); + private final ApplicationEventPublisher eventPublisher; /** @@ -85,7 +91,7 @@ public D save(D data) { updateEntity(entity, data); final E savedEntity = getRepository().save(entity); final D savedData = mapToData(savedEntity); - postSave(true, savedData); + postInsert(savedData); return savedData; } else { final E entity = @@ -95,11 +101,11 @@ public D save(D data) { () -> new IllegalArgumentException( "Entity with id " + id + " not found for tenant " + tenantId)); + final D previous = mapToData(entity); updateEntity(entity, data); - getRepository().save(entity); final E savedEntity = getRepository().save(entity); final D savedData = mapToData(savedEntity); - postSave(false, savedData); + postUpdate(previous, savedData); return savedData; } } @@ -193,6 +199,10 @@ public void delete(UUID id) { /** * Maps a persisted entity to its DTO representation. * + *

    The returned DTO must be a snapshot that shares no mutable state (such as a collection) with + * the entity, because on update it is also used to capture the state before {@link + * #updateEntity(AbstractMultitenantEntity, WithId)} runs. + * * @param entity the persisted entity * @return the corresponding DTO */ @@ -252,17 +262,28 @@ protected void postDelete(D data) { } /** - * Hook invoked after a successful save. Default implementation publishes an {@link - * OnObjectCreate} event for inserts and an {@link OnObjectUpdate} event for updates. + * Hook invoked after a successful insert. Default implementation publishes an {@link + * OnObjectCreate} event. * - * @param insert {@code true} if the operation was an insert, {@code false} for an update - * @param data the DTO that was just persisted + * @param data the DTO that was just inserted */ - protected void postSave(final boolean insert, D data) { - if (insert) { - eventPublisher.publishEvent(new OnObjectCreate<>(data)); - } else { - eventPublisher.publishEvent(new OnObjectUpdate<>(data)); + protected void postInsert(final D data) { + eventPublisher.publishEvent(new OnObjectCreate<>(data)); + } + + /** + * Hook invoked after a successful update. Default implementation publishes an {@link + * OnObjectUpdate} event carrying both states — unless they are {@linkplain Object#equals(Object) + * equal}, in which case the update changed nothing and no event is published. + * + * @param previous the DTO as it was before the update + * @param current the DTO that was just persisted + */ + protected void postUpdate(final D previous, final D current) { + if (previous.equals(current)) { + LOG.debug("Update of {} changed nothing, no event published", current); + return; } + eventPublisher.publishEvent(new OnObjectUpdate<>(previous, current)); } } diff --git a/spring-services-tenant/src/test/java/com/openelements/spring/base/tenant/AbstractMultitenantDbBackedDataServiceEventsTest.java b/spring-services-tenant/src/test/java/com/openelements/spring/base/tenant/AbstractMultitenantDbBackedDataServiceEventsTest.java new file mode 100644 index 0000000..587f4bb --- /dev/null +++ b/spring-services-tenant/src/test/java/com/openelements/spring/base/tenant/AbstractMultitenantDbBackedDataServiceEventsTest.java @@ -0,0 +1,179 @@ +package com.openelements.spring.base.tenant; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.clearInvocations; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import com.openelements.spring.base.data.WithId; +import com.openelements.spring.base.events.OnObjectCreate; +import com.openelements.spring.base.events.OnObjectUpdate; +import java.util.HashMap; +import java.util.Map; +import java.util.Optional; +import java.util.UUID; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; +import org.springframework.context.ApplicationEventPublisher; +import org.springframework.data.domain.Page; +import org.springframework.data.domain.Pageable; + +/** + * Unit tests for the lifecycle events published by {@link AbstractMultitenantDbBackedDataService}. + * + *

    What is tested

    + * + *

    An insert publishes {@link OnObjectCreate}; an update publishes {@link OnObjectUpdate} with the + * state before and after the update; an update that changes nothing publishes no event. + * + *

    How it is tested

    + * + *

    Mockito-based unit test. The repository is backed by an in-memory map that hands out the same + * managed instance on every lookup, so {@code updateEntity} mutates the instance the previous state + * was taken from — exactly like a JPA persistence context. The current tenant is fixed by + * overriding {@code getCurrentTenant()}. + * + *

    Mock-Audit. The publisher mock is the assertion target; the repository mock replaces a + * JPA layer that would need Spring and a database, which would dwarf the focused scope of the test. + */ +@DisplayName("AbstractMultitenantDbBackedDataService lifecycle events") +class AbstractMultitenantDbBackedDataServiceEventsTest { + + private static final String TENANT = "tenant-a"; + + @SuppressWarnings("unchecked") + private static RepositoryWithTenantSupport repositoryThatPersists() { + final Map store = new HashMap<>(); + final RepositoryWithTenantSupport repository = + mock(RepositoryWithTenantSupport.class); + when(repository.save(any(TestEntity.class))) + .thenAnswer( + invocation -> { + final TestEntity entity = invocation.getArgument(0); + if (entity.getId() == null) { + entity.setId(UUID.randomUUID()); + } + store.put(entity.getId(), entity); + return entity; + }); + when(repository.findByIdAndTenantId(any(UUID.class), anyString())) + .thenAnswer( + invocation -> + Optional.ofNullable(store.get(invocation.getArgument(0))) + .filter(entity -> entity.getTenantId().equals(invocation.getArgument(1)))); + return repository; + } + + @Test + @DisplayName("An insert publishes exactly one OnObjectCreate carrying the saved DTO.") + void shouldPublishCreateOnInsert() { + // GIVEN + final ApplicationEventPublisher publisher = mock(ApplicationEventPublisher.class); + final FakeService service = new FakeService(publisher, repositoryThatPersists()); + + // WHEN + service.save(new TestDto(null, "new")); + + // THEN + verify(publisher).publishEvent(any(OnObjectCreate.class)); + } + + @Test + @DisplayName( + "An update publishes one OnObjectUpdate with the DTO before the update as previous state.") + @SuppressWarnings("unchecked") + void shouldPublishPreviousAndCurrentStateOnUpdate() { + // GIVEN + final ApplicationEventPublisher publisher = mock(ApplicationEventPublisher.class); + final FakeService service = new FakeService(publisher, repositoryThatPersists()); + final TestDto saved = service.save(new TestDto(null, "old")); + clearInvocations(publisher); + + // WHEN + service.save(new TestDto(saved.id(), "new")); + + // THEN + final ArgumentCaptor> captor = + ArgumentCaptor.forClass(OnObjectUpdate.class); + verify(publisher).publishEvent(captor.capture()); + assertThat(captor.getValue().getPrevious()).contains(new TestDto(saved.id(), "old")); + assertThat(captor.getValue().getData()).isEqualTo(new TestDto(saved.id(), "new")); + } + + @Test + @DisplayName("An update that changes nothing publishes no event.") + void shouldNotPublishOnNoOpUpdate() { + // GIVEN + final ApplicationEventPublisher publisher = mock(ApplicationEventPublisher.class); + final FakeService service = new FakeService(publisher, repositoryThatPersists()); + final TestDto saved = service.save(new TestDto(null, "same")); + clearInvocations(publisher); + + // WHEN + service.save(new TestDto(saved.id(), "same")); + + // THEN + verify(publisher, never()).publishEvent(any()); + } + + static class TestEntity extends AbstractMultitenantEntity { + + private String name; + } + + record TestDto(UUID id, String name) implements WithId {} + + private static class FakeService + extends AbstractMultitenantDbBackedDataService { + + private final RepositoryWithTenantSupport repository; + + FakeService( + final ApplicationEventPublisher publisher, + final RepositoryWithTenantSupport repository) { + super(publisher); + this.repository = repository; + } + + @Override + protected RepositoryWithTenantSupport getRepository() { + return repository; + } + + @Override + protected TenantService getTenantService() { + throw new UnsupportedOperationException("getCurrentTenant() is overridden"); + } + + @Override + protected String getCurrentTenant() { + return TENANT; + } + + @Override + protected TestDto mapToData(final TestEntity entity) { + return new TestDto(entity.getId(), entity.name); + } + + @Override + protected TestEntity updateEntity(final TestEntity entity, final TestDto data) { + entity.name = data.name(); + return entity; + } + + @Override + protected TestEntity createNewEntity() { + return new TestEntity(); + } + + @Override + public Page findAll(final Pageable pageable) { + throw new UnsupportedOperationException("not needed by these tests"); + } + } +}