Add route operation callbacks - #167
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4bf8c13b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| this.authenticatedPrincipals = | ||
| Map.copyOf( | ||
| authenticatedPrincipals == null | ||
| ? Map.of() | ||
| : new HashMap<>(authenticatedPrincipals)); |
There was a problem hiding this comment.
When a route uses an authenticator that returns the supported ThingifierApiAuthenticationResult.authenticated() result without a principal, RouteAuthPolicy stores a null map value and this Map.copyOf throws NullPointerException before the callback's failure-policy guard is entered. Consequently, merely registering any operation callback makes otherwise successful principal-less authenticated requests crash; copy the map using an immutable representation that permits null values, or omit null principal entries.
Useful? React with 👍 / 👎.
| return new RouteOperationCallbackApplier(runtime) | ||
| .apply(verb, url, policyResponse, context, lifecycle, request); |
There was a problem hiding this comment.
For HTTP-backed requests, this invokes callbacks after one response-policy application, but ThingifierHttpApi.httpResponseFor applies RouteApiResponsePolicyApplier again afterward. If a success policy changes 200 to an error status, for example, the callback observes that intermediate status while the second pass can select the matching error policy and expose a different status/body to the client, so afterStatus and result.statusCode() do not describe the promised final route-shaped result. Ensure the HTTP path applies response policy only once or move callback execution after its last application.
Useful? React with 👍 / 👎.
| @Test | ||
| void afterSuccessfulOperationReceivesFixedRouteUpdateContextAndInstance() { |
There was a problem hiding this comment.
This test combines several independently failing rules—fixed-route metadata, data-scope and header/body propagation, outcome classification, and affected-instance retrieval—under a name that itself requires “And,” so a failure does not identify which callback contract regressed. Split these into focused tests for context propagation and result semantics as required by the repository's test-design rules.
AGENTS.md reference: AGENTS.md:L6-L13
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🟡 Changes recommended
Direct-call callback semantics appear incomplete/inconsistent (missing request details in some direct helpers and ambiguous POST operationType fallback), which can break the expected callback contract.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds route-level synchronous “after operation” callbacks to Thingifier routes, including callback context/result objects and a failure policy to either fail the request or log-and-continue. This implements the extension point described in #166 so applications can run trusted side effects tied directly to a route definition.
Changes:
- Introduces route callback registration APIs on
ThingifierApiRouteRule(any/success/failure/status) with per-callback failure policy. - Applies callbacks after route response policies (so callbacks observe the shaped
ApiResponse) and before legacy HTTP response hooks. - Adds callback context/result value objects plus a test suite covering ordering, auth/data-scope/principal propagation, and failure handling.
File summaries
| File | Description |
|---|---|
| thingifier/src/test/java/uk/co/compendiumdev/thingifier/api/callbacks/RouteOperationCallbackTest.java | New tests for callback invocation, ordering, context/result contents, and failure policy behavior. |
| thingifier/src/main/java/uk/co/compendiumdev/thingifier/api/ThingifierRestAPIHandler.java | Wires callback execution into the shared direct/HTTP handler pipeline after response policy application. |
| thingifier/src/main/java/uk/co/compendiumdev/thingifier/api/spec/ThingifierApiRouteRule.java | Adds fluent callback registration methods and stores callback definitions in route rules. |
| thingifier/src/main/java/uk/co/compendiumdev/thingifier/api/callbacks/ThingifierApiOperationResult.java | Defines the immutable callback result object (status, operation type, instance accessors, response). |
| thingifier/src/main/java/uk/co/compendiumdev/thingifier/api/callbacks/ThingifierApiOperationContext.java | Defines immutable callback context (route, identifiers, scope/store, principals, request details). |
| thingifier/src/main/java/uk/co/compendiumdev/thingifier/api/callbacks/ThingifierApiOperationCallbackDefinition.java | Stores callback metadata (name/outcome/status) and failure policy, with match logic. |
| thingifier/src/main/java/uk/co/compendiumdev/thingifier/api/callbacks/ThingifierApiOperationCallback.java | Functional interface for callbacks. |
| thingifier/src/main/java/uk/co/compendiumdev/thingifier/api/callbacks/CallbackFailurePolicy.java | Failure policy enum for callback exceptions. |
| thingifier/src/main/java/uk/co/compendiumdev/thingifier/adapter/http/apihandlers/RouteOperationCallbackApplier.java | Implements callback execution and builds callback context/result from runtime/lifecycle/request. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| private String operationTypeFor( | ||
| final RoutingVerb verb, final ThingifierApiLifecycleContext lifecycle) { | ||
| if (lifecycle != null && lifecycle.writeCommand() != null) { | ||
| return operationTypeFor(lifecycle.writeCommand()); | ||
| } | ||
| if (verb == RoutingVerb.QUERY) { | ||
| return "QUERY"; | ||
| } | ||
| if (verb == RoutingVerb.GET || verb == RoutingVerb.HEAD) { | ||
| return "READ"; | ||
| } | ||
| if (verb == RoutingVerb.DELETE) { | ||
| return "DELETE"; | ||
| } | ||
| if (verb == RoutingVerb.PATCH) { | ||
| return "PATCH"; | ||
| } | ||
| if (verb == RoutingVerb.PUT) { | ||
| return "REPLACE"; | ||
| } | ||
| if (verb == RoutingVerb.POST) { | ||
| return "WRITE"; | ||
| } | ||
| return ""; |
| private ApiResponse withAuthorizedResponsePolicy( | ||
| final RoutingVerb verb, | ||
| final String url, | ||
| final ThingifierRequestContext context, | ||
| final ThingifierApiLifecycleContext lifecycle, | ||
| final Supplier<ApiResponse> action) { | ||
| return withAuthorizedResponsePolicy(verb, url, context, lifecycle, null, action); | ||
| } |
| class RouteOperationCallbackTest { | ||
|
|
||
| @Test | ||
| void afterSuccessfulOperationReceivesFixedRouteUpdateContextAndInstance() { |
Summary
Verification
Closes #166