Add tests for #2608 #2623

mpritham · 2025-08-26T20:57:33Z

Separating #2608 to make it easier to review. This PR should be updated to create separate test files instead of using the existing dialogue test case, but using the existing test case makes it easy to see what concretely changes when the new functionality isused.

aldexis · 2025-08-27T11:13:07Z

conjure-java-core/src/integrationInput/java/dialogue/com/palantir/another/ConjureErrors.java

+    public static final class DifferentPackageErrorException extends RemoteException {
+        private DifferentPackageErrorSerializableError error;


RemoteException is serializable, due to being a subclass of Throwable (and contains a serialVersionUid). I'm wondering whether this means we need to make all these exceptions and the AbstractSerializableError also serializable (which would raise the question of how to maintain compatibility upon changes)

I don't think we would be relying on this anywhere, but this might be worth to explicitly acknowledge this?

yeah, I'm not sure of the history here, but we use jackson to deserialize some subclass of AbstractSerializableError, and construct this class - it's never deserialized to directly off the wire. I'm not sure that RemoteException is either.

I can acknowledge this in a comment.

All Throwables are Serializable, so it's really just a matter of whether we care. It sounds like RemoteException actually has a serialVersionUid. It sounds like it was added form the get-go though palantir/conjure-java-runtime-api#9 and I can't imagine this being used as serializable anywhere.

With that said, it might be worth just adding a default serialVersionUid on the subclasses nonetheless? 🤔 (adding it after the fact is a bit annoying after all)

aldexis · 2025-08-27T11:27:23Z

...java-core/src/integrationInput/java/dialogue/com/palantir/product/EmptyPathServiceAsync.java

+                        .exception(
+                                ConjureErrors.CONFLICTING_CAUSE_SAFE_ARG.name(),
+                                new TypeMarker<ConjureErrors.ConflictingCauseSafeArgSerializableError>() {},
+                                new TypeMarker<ConjureErrors.ConflictingCauseSafeArgException>() {})
+                        .exception(
+                                ConjureErrors.CONFLICTING_CAUSE_UNSAFE_ARG.name(),
+                                new TypeMarker<ConjureErrors.ConflictingCauseUnsafeArgSerializableError>() {},
+                                new TypeMarker<ConjureErrors.ConflictingCauseUnsafeArgException>() {})
+                        .exception(
+                                ConjureErrors.ERROR_WITH_COMPLEX_ARGS.name(),
+                                new TypeMarker<ConjureErrors.ErrorWithComplexArgsSerializableError>() {},
+                                new TypeMarker<ConjureErrors.ErrorWithComplexArgsException>() {})
+                        .exception(
+                                ConjureErrors.INVALID_SERVICE_DEFINITION.name(),
+                                new TypeMarker<ConjureErrors.InvalidServiceDefinitionSerializableError>() {},
+                                new TypeMarker<ConjureErrors.InvalidServiceDefinitionException>() {})
+                        .exception(
+                                ConjureErrors.INVALID_TYPE_DEFINITION.name(),
+                                new TypeMarker<ConjureErrors.InvalidTypeDefinitionSerializableError>() {},
+                                new TypeMarker<ConjureErrors.InvalidTypeDefinitionException>() {})
+                        .exception(
+                                ConjureJavaErrors.JAVA_COMPILATION_FAILED.name(),
+                                new TypeMarker<ConjureJavaErrors.JavaCompilationFailedSerializableError>() {},
+                                new TypeMarker<ConjureJavaErrors.JavaCompilationFailedException>() {})


Should this not contain the error from the different package too? It can be thrown by the service that defined it, right?

aldexis · 2025-08-27T11:29:23Z

...a-core/src/integrationInput/java/dialogue/com/palantir/product/EmptyPathServiceBlocking.java

+            private <T> ExceptionDeserializerArgs<T> createExceptionDeserializerArgs(TypeMarker<T> returnType) {
+                return ExceptionDeserializerArgs.<T>builder()
+                        .returnType(returnType)
+                        .exception(
+                                ConjureErrors.CONFLICTING_CAUSE_SAFE_ARG.name(),
+                                new TypeMarker<ConjureErrors.ConflictingCauseSafeArgSerializableError>() {},
+                                new TypeMarker<ConjureErrors.ConflictingCauseSafeArgException>() {})
+                        .exception(
+                                ConjureErrors.CONFLICTING_CAUSE_UNSAFE_ARG.name(),
+                                new TypeMarker<ConjureErrors.ConflictingCauseUnsafeArgSerializableError>() {},
+                                new TypeMarker<ConjureErrors.ConflictingCauseUnsafeArgException>() {})
+                        .exception(
+                                ConjureErrors.ERROR_WITH_COMPLEX_ARGS.name(),
+                                new TypeMarker<ConjureErrors.ErrorWithComplexArgsSerializableError>() {},
+                                new TypeMarker<ConjureErrors.ErrorWithComplexArgsException>() {})
+                        .exception(
+                                ConjureErrors.INVALID_SERVICE_DEFINITION.name(),
+                                new TypeMarker<ConjureErrors.InvalidServiceDefinitionSerializableError>() {},
+                                new TypeMarker<ConjureErrors.InvalidServiceDefinitionException>() {})
+                        .exception(
+                                ConjureErrors.INVALID_TYPE_DEFINITION.name(),
+                                new TypeMarker<ConjureErrors.InvalidTypeDefinitionSerializableError>() {},
+                                new TypeMarker<ConjureErrors.InvalidTypeDefinitionException>() {})
+                        .exception(
+                                ConjureJavaErrors.JAVA_COMPILATION_FAILED.name(),
+                                new TypeMarker<ConjureJavaErrors.JavaCompilationFailedSerializableError>() {},
+                                new TypeMarker<ConjureJavaErrors.JavaCompilationFailedException>() {})
+                        .build();
+            }


It's bugging me a bit that we're defining the exact same method in each service, and then calling it to create new deserializer args for each endpoint (which will be the same if they share a return type).
Do you envision this being some kind of problem or maybe instead a benefit down the line? (e.g. whether on memory allocation, since we're copying the same map of possible hundreds of errors on each endpoint, which might be many as well, or our ability to change the code-gen later on)

Maybe it is better this way, because if we need to later have different exceptions deserializable for different services, we don't need to break the ABI by removing a method that creates the generic deserializer. With that said, I'm not sure when we'd want to not support all errors, but only a subset.

One solution to this could be to create a new file that just has this method which all generated services can use. From the comment above regarding adding all errors in the Conjure definitions, we'd have only one helper method definition in this file.

So concretely, I'm thinking of a file ServiceErrorUtils that is generated which has one method createExceptionDeserializerArgs.

If we end up changing how the deserializers are created, I think we shouldn't have an ABI break?

With that said, I'm not sure when we'd want to not support all errors, but only a subset.

I think this is related to the comment above: whether we filter errors on defined in the same package, or just include every error in the Conjure definition provided. I implemented the former, but I'm sort of learning toward the latter to make migration easier. Some folks end up creating errors in a separate errors package.

So the changes I want to make here:

take out the filtering on package - consider every error defined for every service

add a separate utility file for this helper method

With the separate utility file, we will still end up creating loads of maps. One idea is to introduce a cache from returnType -> exceptionDeserializerArgs.

We chatted about this yesterday. There unfortunately isn't really a great place to put such a file, since it can't be in a predefined package, to avoid conflicts with other dialogue client libraries. I think it's fine to leave it as one method per client for now, and get rid of it if we realize this causes problems. This is also why we have various safe rollout mechanisms after all.

This (and the fact that we can't deserialize exceptions that aren't known at codegen time) makes me wonder however whether we could instead try to discover exceptions at runtime (e.g. through some autoservice or other mechanism, making it possible to build a centralized map of all exceptions, instead of having each client and deserializer to know about them).

aldexis · 2025-08-27T11:35:28Z

...java-core/src/integrationInput/java/dialogue/com/palantir/product/EteBinaryServiceAsync.java

-                                getOptionalBinaryPresentChannel,
-                                _request.build(),
-                                _runtime.bodySerDe().optionalInputStreamDeserializer());
+                        .call(getOptionalBinaryPresentChannel, _request.build(), getOptionalBinaryPresentDeserializer);


side-note: the name of this endpoint is so confusing, because I though getOptionalBinaryPresentChannel and getOptionalBinaryPresentDeserializer were methods (e.g. suppliers), when they were in fact fields

aldexis · 2025-08-27T11:43:31Z

conjure-java-core/src/integrationInput/java/dialogue/test/api/CookieServiceAsync.java

+                return ExceptionDeserializerArgs.<T>builder()
+                        .returnType(returnType)
+                        .build();


Is CookieServiceAsync considered as part of a different conjure project? If not, should it also support deserializing all errors?

This is related to the comments above regarding the filtering around what errors are considered. In the implementation, I added filters so services would be able to handle all errors defined in the same package. After looking at some usages, it's probably safest to include all errors defined - some folks put errors in a separate package com.palantir.x.errors.

Even if they didn't, it's possible, like you said, for a service to throw errors defined in a different package.

aldexis · 2025-08-27T12:23:16Z

conjure-java-core/src/test/java/com/palantir/conjure/java/UndertowServiceEteTest.java

-            //                        .contains("optional-value");
-            //            });
+            // e should be an instance of ErrorWithComplexArgsException, which has rich parameters as well
+            assertThat(e.getCause()).isInstanceOfSatisfying(ErrorWithComplexArgsException.class, exception -> {


Shouldn't this be

Suggested change

assertThat(e.getCause()).isInstanceOfSatisfying(ErrorWithComplexArgsException.class, exception -> {

assertThat(e).isInstanceOfSatisfying(ErrorWithComplexArgsException.class, exception -> {

rather than e.getCause()?

Discussed about this yesterday. This is because of the pattern from https://github.com/palantir/dialogue/blob/c17a28c5673cd3988056a4181d38f5f1eae43f59/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/DefaultClients.java#L148.

We ought to either replicate this pattern with the new remote exceptions, or instead get rid of it entirely (and do what is done for other types of exceptions)

[skip ci]

aldexis

Didn't do a very deep review of the code changes (i.e. I didn't inspect each line, because they seem mostly duplicates of each other), so I could have missed one specific change that is problematic among all the others, but this seems fine to me 👍

mpritham · 2025-09-11T14:38:03Z

I'm going to modify #2608 to create a new test case (instead of setting the flag to an existing test case like I did in this PR). No need to merge this PR into #2608.

stale · 2025-10-18T05:05:28Z

This PR has been automatically marked as stale because it has not been touched in the last 14 days. If you'd like to keep it open, please leave a comment or add the 'long-lived' label, otherwise it'll be closed in 7 days.

mpritham changed the base branch from develop to mpritham/exceptions-with-rich-params August 26, 2025 20:57

mpritham force-pushed the mpritham/rich-exceptions-tests branch from eaab496 to 6e392f4 Compare August 26, 2025 21:00

mpritham added the no changelog label Aug 26, 2025

mpritham changed the title ~~[dnm] Add tests for #2608~~ Add tests for #2608 Aug 26, 2025

palantir deleted a comment from changelog-app bot Aug 26, 2025

aldexis reviewed Aug 27, 2025

View reviewed changes

mpritham force-pushed the mpritham/exceptions-with-rich-params branch 2 times, most recently from 05f862d to 76484ec Compare August 28, 2025 20:59

mpritham force-pushed the mpritham/rich-exceptions-tests branch from 6e392f4 to ed1a75d Compare August 28, 2025 21:18

mpritham force-pushed the mpritham/exceptions-with-rich-params branch from 76484ec to b13b56d Compare August 28, 2025 21:42

mpritham force-pushed the mpritham/rich-exceptions-tests branch from ed1a75d to 1e4f594 Compare September 7, 2025 23:26

Pritham Marupaka and others added 18 commits September 10, 2025 15:22

Respect Accept-Conjure-Error-Parameter-Format

004ab61

clean up handle codepath

84aeda1

remove mavenLocal

e0014be

Add generated changelog entries

39127f6

Release 8.51.0-rc1

35dd7d3

[skip ci]

Testing JSON -> Objects.toString conversion to maintain backcompat

7e791a9

wip code-gen

6e32361

WIP: code generation

2ed8db7

Generate code and add basic test

094d44f

cleanup unneeded files

5e66172

update to use ExceptionDeserializerArgs

5ca715e

Add helper method to construct

0c80221

Use new empty body, input stream and optional input stream deserializers

0d5c0e8

change flag name

5f56b99

update locks and fix merge

b8fdd54

remove unneeded check for 'bodySerDe.errorParameterFormat'

9ddc927

clean up

1b5a899

cleanup after rebase

a935dff

Pritham Marupaka added 4 commits September 10, 2025 15:22

Undo test addition for easier review

bd0f970

review changes

d26910c

Use SerailizableErrorProvider

2c8a4d2

update dialogue version

9394834

mpritham force-pushed the mpritham/exceptions-with-rich-params branch from 58ed0e4 to 9394834 Compare September 10, 2025 21:43

Pritham Marupaka added 4 commits September 10, 2025 17:44

Add tests

c77be1f

review changes

5d2e282

Use SerializableErrorProvider

377e185

update test

948b05d

mpritham force-pushed the mpritham/rich-exceptions-tests branch from 1e4f594 to 948b05d Compare September 10, 2025 21:46

aldexis reviewed Sep 11, 2025

View reviewed changes

mpritham force-pushed the mpritham/exceptions-with-rich-params branch 2 times, most recently from 63549f0 to a2135bf Compare September 18, 2025 14:41

stale bot added the stale label Oct 18, 2025

		public static final class DifferentPackageErrorException extends RemoteException {
		private DifferentPackageErrorSerializableError error;

	assertThat(e.getCause()).isInstanceOfSatisfying(ErrorWithComplexArgsException.class, exception -> {
	assertThat(e).isInstanceOfSatisfying(ErrorWithComplexArgsException.class, exception -> {

Add tests for #2608 #2623

Are you sure you want to change the base?

Add tests for #2608 #2623

Uh oh!

Conversation

mpritham commented Aug 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aldexis Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aldexis left a comment

Choose a reason for hiding this comment

Uh oh!

mpritham commented Sep 11, 2025

Uh oh!

stale bot commented Oct 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aldexis Aug 27, 2025 •

edited

Loading