Add operation interceptors #14

Quinn-With-Two-Ns · 2024-10-17T18:03:57Z

Based of the internal proposed interface. See the test case for two example middlewares that are possible with this interface

Quinn-With-Two-Ns · 2024-10-17T18:13:59Z

Here is an example of how these interceptors would be used temporalio/sdk-java#2278

Quinn-With-Two-Ns · 2024-11-25T00:41:21Z

nexus-sdk/src/main/java/io/nexusrpc/handler/OperationMiddleware.java

+public interface OperationMiddleware {
+
+  /** Intercepts the given operation. Called once for each operation invocation. */
+  OperationHandler<Object, Object> intercept(OperationHandler<Object, Object> next);


@cretz I know one of your desires was to allow the interceptor to switch on different operations, right now that is possible inside the wrapped OperationHandler , we could also add a OperationContext as the first parameter here.

Yes, but I also wonder if that's enough for future proofing if we wanted to add something else. Maybe a OperationMiddlewareInterceptContext that contains the operation context and maybe even this next handler (though I'd be ok if they remained separate parameters). Or could just be an inner class of Context here. Also, I wonder if people should be allowed to alter the input information passed to the next middleware similar to how one might in Temporal interceptors today.

wonder if people should be allowed to alter the input information passed to the next middleware similar to how one might in Temporal interceptors today.

That is already possible with this implementation in the exact same way

Can you address the earlier part of the paragraph about adding some kind of contextual object even if it's empty or mostly empty today? As for altering the values, so long as nobody expects to alter the operation name itself, agreed.

Yes, but I also wonder if that's enough for future proofing if we wanted to add something else.

I think if we want to add anything that is common across all operation methods it would go in the OperationContext , if it specific to a certain method like start then the wrapped handler should deal with it.

@bergundy if you have any thoughts here

I agree we can supply some context here. It would contain the service name, operation name, and request header. The rest of the information is method specific, and IMHO, should be intercepted by the wrapped handler.

The obvious downside for instantiating these handlers per request is the extra garbage generated but I'm not too concerned with that.

OK added context as a first parameter

Quinn-With-Two-Ns · 2024-11-25T00:46:19Z

nexus-sdk/build.gradle

@@ -53,7 +53,7 @@ import com.vanniktech.maven.publish.SonatypeHost
 mavenPublishing {
    publishToMavenCentral(SonatypeHost.CENTRAL_PORTAL)

-    signAllPublications()


Note: need to remove this before merging, this line just makes local development impossible

Quinn-With-Two-Ns · 2024-11-25T00:48:05Z

nexus-sdk/src/main/java/io/nexusrpc/handler/ServiceHandler.java

@@ -87,7 +103,7 @@ public HandlerResultContent fetchOperationResult(
    if (handler == null) {
      throw newUnrecognizedOperationException(context.getService(), context.getOperation());
    }
-    Object result = handler.fetchResult(context, details);
+    Object result = getOperationHandler(handler).fetchResult(context, details);


If we did https://github.com/nexus-rpc/sdk-java/pull/14/files#r1855645828 we could also consider switching where we call getOperationHandler to be before the data coversion. This would allow to fail requests before the data converter runs.

Yeah, that'd be great.

Done, this only applied to start

cretz

LGTM only minor things

cretz · 2024-11-25T13:56:49Z

nexus-sdk/src/main/java/io/nexusrpc/handler/OperationMiddleware.java

+public interface OperationMiddleware {
+
+  /** Intercepts the given operation. Called once for each operation invocation. */
+  OperationHandler<Object, Object> intercept(OperationHandler<Object, Object> next);


Yes, but I also wonder if that's enough for future proofing if we wanted to add something else. Maybe a OperationMiddlewareInterceptContext that contains the operation context and maybe even this next handler (though I'd be ok if they remained separate parameters). Or could just be an inner class of Context here. Also, I wonder if people should be allowed to alter the input information passed to the next middleware similar to how one might in Temporal interceptors today.

nexus-sdk/src/main/java/io/nexusrpc/handler/ServiceHandler.java

cretz · 2024-11-25T14:00:23Z

nexus-sdk/src/main/java/io/nexusrpc/handler/OperationMiddleware.java

+public interface OperationMiddleware {
+
+  /** Intercepts the given operation. Called once for each operation invocation. */
+  OperationHandler<Object, Object> intercept(OperationHandler<Object, Object> next);


Suggested change

OperationHandler<Object, Object> intercept(OperationHandler<Object, Object> next);

OperationHandler<Object, Object> apply(OperationHandler<Object, Object> next);

I wonder if we don't need to call this "intercept" or these "interceptors" if we're adopting middleware-ish terminology. But I don't mind intercept either.

Maybe wrap? But intercept also works for me.

Left as intercept for now

bergundy

Agree we should add the context and allow intercepting before deserializing the input.

bergundy · 2024-12-02T18:28:36Z

nexus-sdk/src/main/java/io/nexusrpc/handler/OperationMiddleware.java

+public interface OperationMiddleware {
+
+  /** Intercepts the given operation. Called once for each operation invocation. */
+  OperationHandler<Object, Object> intercept(OperationHandler<Object, Object> next);


I agree we can supply some context here. It would contain the service name, operation name, and request header. The rest of the information is method specific, and IMHO, should be intercepted by the wrapped handler.

bergundy · 2024-12-02T18:29:38Z

nexus-sdk/src/main/java/io/nexusrpc/handler/OperationMiddleware.java

+public interface OperationMiddleware {
+
+  /** Intercepts the given operation. Called once for each operation invocation. */
+  OperationHandler<Object, Object> intercept(OperationHandler<Object, Object> next);


Maybe wrap? But intercept also works for me.

bergundy · 2024-12-02T18:31:24Z

nexus-sdk/src/main/java/io/nexusrpc/handler/OperationMiddleware.java

+public interface OperationMiddleware {
+
+  /** Intercepts the given operation. Called once for each operation invocation. */
+  OperationHandler<Object, Object> intercept(OperationHandler<Object, Object> next);


The obvious downside for instantiating these handlers per request is the extra garbage generated but I'm not too concerned with that.

bergundy · 2024-12-02T18:43:45Z

nexus-sdk/src/main/java/io/nexusrpc/handler/ServiceHandler.java

@@ -87,7 +103,7 @@ public HandlerResultContent fetchOperationResult(
    if (handler == null) {
      throw newUnrecognizedOperationException(context.getService(), context.getOperation());
    }
-    Object result = handler.fetchResult(context, details);
+    Object result = getOperationHandler(handler).fetchResult(context, details);


Yeah, that'd be great.

nexus-sdk/src/test/java/io/nexusrpc/handler/ServiceHandlerTest.java

cretz

LGTM, only minor things

nexus-sdk/src/main/java/io/nexusrpc/handler/ServiceHandler.java

Quinn-With-Two-Ns requested review from bergundy and cretz October 17, 2024 18:03

Quinn-With-Two-Ns mentioned this pull request Oct 17, 2024

Add Nexus Worker interceptor temporalio/sdk-java#2278

Merged

Quinn-With-Two-Ns force-pushed the interceptors branch from bc96d00 to c8881f8 Compare November 24, 2024 23:57

Quinn-With-Two-Ns marked this pull request as ready for review November 25, 2024 00:36

Quinn-With-Two-Ns commented Nov 25, 2024

View reviewed changes

cretz reviewed Nov 25, 2024

View reviewed changes

bergundy reviewed Dec 2, 2024

View reviewed changes

cretz reviewed Dec 4, 2024

View reviewed changes

nexus-sdk/src/main/java/io/nexusrpc/handler/ServiceHandler.java Outdated Show resolved Hide resolved

nexus-sdk/src/main/java/io/nexusrpc/handler/ServiceHandler.java Outdated Show resolved Hide resolved

Quinn-With-Two-Ns added 5 commits December 4, 2024 10:22

Add operation interceptors

5accdd3

Switch to middleware approach

f4d8653

Add test for middleware

454bfa6

Respond to PR comment

1953619

respond to PR comments

2660bba

Quinn-With-Two-Ns force-pushed the interceptors branch from d2134b4 to 2660bba Compare December 4, 2024 18:22

cretz approved these changes Dec 4, 2024

View reviewed changes

Quinn-With-Two-Ns merged commit c9bbb1a into nexus-rpc:main Dec 5, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add operation interceptors #14

Add operation interceptors #14

Quinn-With-Two-Ns commented Oct 17, 2024 •

edited

Loading

Quinn-With-Two-Ns commented Oct 17, 2024

Quinn-With-Two-Ns Nov 25, 2024

cretz Nov 25, 2024 •

edited

Loading

Quinn-With-Two-Ns Nov 25, 2024

cretz Nov 25, 2024

Quinn-With-Two-Ns Nov 25, 2024

Quinn-With-Two-Ns Nov 25, 2024

bergundy Dec 2, 2024

bergundy Dec 2, 2024

Quinn-With-Two-Ns Dec 4, 2024

Quinn-With-Two-Ns Nov 25, 2024

Quinn-With-Two-Ns Nov 25, 2024

bergundy Dec 2, 2024

Quinn-With-Two-Ns Dec 4, 2024

cretz left a comment

cretz Nov 25, 2024 •

edited

Loading

cretz Nov 25, 2024

bergundy Dec 2, 2024

Quinn-With-Two-Ns Dec 4, 2024

bergundy left a comment

bergundy Dec 2, 2024

bergundy Dec 2, 2024

bergundy Dec 2, 2024

bergundy Dec 2, 2024

cretz left a comment

	OperationHandler<Object, Object> intercept(OperationHandler<Object, Object> next);
	OperationHandler<Object, Object> apply(OperationHandler<Object, Object> next);

Add operation interceptors #14

Add operation interceptors #14

Conversation

Quinn-With-Two-Ns commented Oct 17, 2024 • edited Loading

Quinn-With-Two-Ns commented Oct 17, 2024

Choose a reason for hiding this comment

cretz Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz left a comment

Choose a reason for hiding this comment

cretz Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergundy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz left a comment

Choose a reason for hiding this comment

Quinn-With-Two-Ns commented Oct 17, 2024 •

edited

Loading

cretz Nov 25, 2024 •

edited

Loading

cretz Nov 25, 2024 •

edited

Loading