[Sealed unions] Make `toString` backward compatible #2753

mpritham · 2025-12-15T19:49:51Z

Before this PR

The union objects generated when the sealedUnions flag is set create toString methods that are not backward-compatible with objects generated without the sealedUnions flag. While in general toString method implementation is not part of the Conjure API, it's current use in the serialization of error parameters makes rolling out changes to toString a wire break.

After this PR

==COMMIT_MSG==
[Sealed unions] Make toString backward compatible
==COMMIT_MSG==

Possible downsides?

Clunky and inaccurate. The toString representation of union objects no longer accurately represents the generated object: there will be <Variant>Wrapperstrings in thetoString` representation which do not correspond to any class in the sealed class implementation.

mpritham · 2025-12-15T19:57:26Z

conjure-java-core/src/main/java/com/palantir/conjure/java/types/UnionGenerator.java

                .addMethod(MethodSpecs.createToString(
                                unionClass.simpleName(),
-                                fields.stream()
+                                valueFieldList.stream()


Unneeded traversal.

Not sure I understand this comment, nor why you made the above change, as it doesn't seem like you're using valueField anywhere?

aldexis · 2025-12-16T14:19:41Z

conjure-java-core/src/main/java/com/palantir/conjure/java/types/UnionGenerator.java

                .addMethod(MethodSpecs.createToString(
                                unionClass.simpleName(),
-                                fields.stream()
+                                valueFieldList.stream()


Not sure I understand this comment, nor why you made the above change, as it doesn't seem like you're using valueField anywhere?

aldexis · 2025-12-16T14:27:18Z

conjure-java-core/src/main/java/com/palantir/conjure/java/types/UnionGenerator.java

+        if (sealedUnions) {
+            return MethodSpec.methodBuilder("toString")
+                    .addAnnotation(Override.class)
+                    .addModifiers(Modifier.PUBLIC)
+                    .returns(ClassName.get(String.class))
+                    .addCode(CodeBlock.builder()
+                            .add(
+                                    "return $S\n",
+                                    baseClassName.simpleName() + '{' + VALUE_FIELD_NAME + ": "
+                                            + wrapperClassName.simpleName() + '{' + VALUE_FIELD_NAME + ": ")
+                            .add(" + $N", VALUE_FIELD_NAME)
+                            .add(" + \"}}\";")
+                            .build())
+                    .addJavadoc(
+                            "This method is not part of Conjure API. Users should not rely on consistent generation of"
+                                    + " the toString method between versions of Conjure.")
+                    .build();
+        } else {
+            return MethodSpec.methodBuilder("toString")
+                    .addAnnotation(Override.class)
+                    .addModifiers(Modifier.PUBLIC)
+                    .returns(ClassName.get(String.class))
+                    .addCode(CodeBlock.builder()
+                            .add("return $S\n", wrapperClassName.simpleName() + '{' + VALUE_FIELD_NAME + ": ")
+                            .add(" + $N", VALUE_FIELD_NAME)
+                            .add(" + '}';")
+                            .build())
+                    .build();
+        }


Something slightly bugs me in that we are hardcoding the expectation that there is only a single field. I'm not seeing this change in the near-term, but I'm wondering whether we could somehow rely on MethodSpecs.createToString or at least have a similar logic?

A few ideas:

Have some nested structure passed to MethodSpecs.createToString that can represent arbitrarily nested lists

sealed interface ToStringField permits SimpleField, NestedField {} record SimpleField(FieldName name) implements ToStringField {} record NestedField(FieldName name, List<ToStringField> children) implements ToStringField {}

I like the design of this, but I didn't follow through with the idea because it made things more complicated for just this one use-case. I think we should explore this if we need to make other nested toString methods.

Pass in a map that provides a CodeBlock toMethodSpecs.createToString containing the representation of the field.

private static CodeBlock toStringConcatenation( String thisClassName, List<FieldName> fieldNames, Map<FieldName, CodeBlock> customFieldBlocks) {...}

This feels like a worse version of the idea above: the caller has to copy the logic in MethodSpecs.toStringConcatenation.

Maybe there's another thing you had in mind?

Having special handling for the legacy case seems fine actually. I can't seem to figure out a good way that isn't over-engineered. Relying on the existing method for the normal case seems better though (which I see you've done).

One thing we could add would be a MethodSpecs.createToStringBuilder(CodeBlock) -> MethodSpec.Builder to encode the

MethodSpec.methodBuilder("toString") .addAnnotation(Override.class) .addModifiers(Modifier.PUBLIC) .returns(ClassName.get(String.class))

part of the logic, but I don't feel strongly about it and it does clutter the code a bit

aldexis · 2025-12-16T14:27:33Z

conjure-java-core/src/main/java/com/palantir/conjure/java/types/UnionGenerator.java

+                    .addJavadoc(
+                            "This method is not part of Conjure API. Users should not rely on consistent generation of"
+                                    + " the toString method between versions of Conjure.")


Should we just always have this javadoc?

I'm not sure we should include this javadoc in the generation, feels a bit out of place just being on sealed union types. As long as we have a plan to migrate away from the old toString, we should just revert this later once safe.

This makes it somewhat explicit that people shouldn't be relying on toString's format in general, rather than just because of this specific case

Adding this javadoc to all object toString methods is quite a large diff and somewhat unrelated to this PR. I can open a separate PR adding the javadoc to the generated code in other object types.

As a counter point, there's a lot of things explicitly encoded in our conjure spec that aren't directly included in code/javadocs. Given this is true for all toStrings, it feels better to document this once as opposed to on every conjure object's toString method. I think generally generated code should be just pure output based on the input conjure.

That makes sense. And it's general bad practice to rely on a specific toString format too, so maybe let's not bother

aldexis · 2025-12-16T14:27:53Z

conjure-java-core/src/main/java/com/palantir/conjure/java/types/UnionGenerator.java

+    /** Creates a backward compatible toString method. While in general, Conjure-Java does not provide any guarantees on
+     * the toString representation of generated objects, the toString method is currently used when serializing error
+     * parameters when JSON is not explicitly used. This is clunky because the sealedUnions implementation does not
+     * generate any classes suffixed with `Wrapper`, yet such classes show up in the `toString` implementation. In a
+     * future Conjure-Java change, we can update the toString method here to better represent the variant in the
+     * sealedUnions implementation.
+     */
+    private static MethodSpec createToString(
+            boolean sealedUnions, ClassName baseClassName, ClassName wrapperClassName) {


Can we add some tests to guarantee that this stays true?

kunalrkak · 2025-12-17T21:53:43Z

conjure-java-core/src/main/java/com/palantir/conjure/java/types/UnionGenerator.java

+                            "This method is not part of Conjure API. Users should not rely on consistent generation of"
+                                    + " the toString method between versions of Conjure.")
+                    .build();
+        } else {


why would we not continue to use MethodSpecs#createToString for at least the old union types?

Updated. I was trying to create a method that handles only the case where we have value as a field, but it ended up being not as clean as just using the old logic in MethodSpecs

kunalrkak · 2025-12-17T21:57:23Z

conjure-java-core/src/main/java/com/palantir/conjure/java/types/UnionGenerator.java

+                    .addJavadoc(
+                            "This method is not part of Conjure API. Users should not rely on consistent generation of"
+                                    + " the toString method between versions of Conjure.")


I'm not sure we should include this javadoc in the generation, feels a bit out of place just being on sealed union types. As long as we have a plan to migrate away from the old toString, we should just revert this later once safe.

aldexis

The current approach seems reasonable to me, provided we do want to do this. If/when we confirm and you remove the do-not-merge, I'm happy to approve

…as legacy unions

changelog-app · 2025-12-18T16:16:23Z

Generate changelog in `changelog/@unreleased`

Type (Select exactly one)

Feature (Adding new functionality)
Improvement (Improving existing functionality)
Fix (Fixing an issue with existing functionality)
Break (Creating a new major version by breaking public APIs)
Deprecation (Removing functionality in a non-breaking way)
Migration (Automatically moving data/functionality to a new system)

Description

[Sealed unions] Make toString backward compatible

Check the box to generate changelog(s)

Generate changelog entry

changelog-app · 2025-12-18T16:16:59Z

✅ Successfully generated changelog entry!

Need to regenerate?

Simply interact with the changelog bot comment again to regenerate these entries.

📋Changelog Preview

💡 Improvements

[Sealed unions] Make toString backward compatible (#2753)

mpritham added the do not merge label Dec 15, 2025

mpritham requested review from aldexis, bjlaub, kunalrkak and parker89 December 15, 2025 19:50

mpritham force-pushed the mpritham/sealed-unions-toString branch from 8f69baf to 79f7c1d Compare December 15, 2025 19:56

mpritham commented Dec 15, 2025

View reviewed changes

aldexis reviewed Dec 16, 2025

View reviewed changes

kunalrkak reviewed Dec 17, 2025

View reviewed changes

mpritham force-pushed the mpritham/sealed-unions-toString-p2 branch 2 times, most recently from dc99860 to 15dc067 Compare December 18, 2025 04:08

aldexis reviewed Dec 18, 2025

View reviewed changes

pmarupaka added 6 commits December 18, 2025 11:15

Add basic test to validate sealed unions are serialized the same way …

978b115

…as legacy unions

review change: move test

9a4be58

review change

9c43fd7

wip

1a84a14

Add docstring

45223b7

review changes

acb0f8d

mpritham force-pushed the mpritham/sealed-unions-toString-p2 branch from 15dc067 to acb0f8d Compare December 18, 2025 16:16

mpritham changed the base branch from mpritham/sealed-unions-toString to develop December 18, 2025 16:16

mpritham changed the title ~~[DNM] [Sealed unions] Make toString backward compatible~~ [Sealed unions] Make toString backward compatible Dec 18, 2025

mpritham added merge when ready autorelease and removed do not merge merge when ready autorelease labels Dec 18, 2025

Update unknown variant toString

bdffd58

[Sealed unions] Make toString backward compatible #2753

Are you sure you want to change the base?

[Sealed unions] Make toString backward compatible #2753

Uh oh!

Conversation

mpritham commented Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before this PR

After this PR

Possible downsides?

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!

mpritham Dec 18, 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!

aldexis left a comment

Choose a reason for hiding this comment

Uh oh!

changelog-app bot commented Dec 18, 2025 • edited by mpritham Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Generate changelog in changelog/@unreleased

Uh oh!

changelog-app bot commented Dec 18, 2025

✅ Successfully generated changelog entry!

Need to regenerate?

📋Changelog Preview

💡 Improvements

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[Sealed unions] Make `toString` backward compatible #2753

[Sealed unions] Make `toString` backward compatible #2753

mpritham commented Dec 15, 2025 •

edited

Loading

mpritham Dec 18, 2025 •

edited

Loading

changelog-app bot commented Dec 18, 2025 •

edited by mpritham

Loading

Generate changelog in `changelog/@unreleased`