[Discussion] Remove type field from union types

[raiju]

Conjure's union types can end up being extremely inefficient in terms of bytes in the wire format, for example when encoding ASTs (where there is generally a lot of polymorphic branching). The double-encoding of the type for union types is the main first order cause, as for small-ish ASTs it will almost double the size:

Lit(Int(42)):
{"type":"lit","lit":{"type":"int","int":42}}
vs
{"lit":{"int":42}}

while not actually adding any new information. @markelliot believes the second form can be implemented for all currently-known targets.

Has this already been discussed? Is this worth the extra work to change?

@markelliot We discussed this a little bit ago, in the context of the Conjure encoding overhead (in terms of bytes used). As it will be significantly more expensive to make this change once this specification has been released (as it's a wire format change), I wanted to start a discussion now.

[raiju]

Actions:

• Check which implementations require the type field

Moving forward:

• union2 or a deprecation "concise" flag (when all deserialization can decode concise form)
• Mime-type? Header containing conjure version?

[raiju]

• Typescript: Uses type
  • https://github.com/palantir/conjure-typescript/blob/78d8185005711b9ba1132d8da339a0fccba3b9cf/src/commands/generate/typeGenerator.ts#L221-L248
  • Should be easy to work around
• Java: Uses type
  • https://github.com/palantir/conjure-java/blob/develop/conjure-java-core/src/main/java/com/palantir/conjure/java/types/UnionGenerator.java#L310
  • Should be possible using include as: https://github.com/FasterXML/jackson-docs/wiki/JacksonPolymorphicDeserialization#methods-for-embedding-instance-type-information
• Go: Uses type
  • Estimate: "Wouldn't be too hard"
• Python: Uses type
  • https://github.com/palantir/conjure-python-client/blob/develop/conjure_python_client/_serde/decoder.py#L87

[sfackler]

The proposed layout is pretty trivial to support on the Rust side of things as well.

[markelliot]

On the Java end: I think we can revisit the unions-as-objects-with-optionals PR I put up a little while back, it didn't need the type field and generated code had fewer bonus classes.

[nmiyake]

There is actually a case that is a bit difficult to handle without the type field (for sure in Go, but I believe this will also apply for Java and any other implementation that doesn't parse the raw JSON directly) -- a union type where the value is an optional which is absent.

Consider the union:

MyUnion:
  union:
    foo: optional<boolean>
    bar: optional<string>

If we have an instance whose content is bar with Optional.empty(), then the current serialized form is:

{"type":"bar","bar":null}

If we remove the type key, then the serialized form is:

{"bar":null}

If we just unmarshal this into a union object, in many languages it won't be possible to distinguish the type -- for example, if you unmarshal it into a Java file with 2 fields (Optional<Boolean> and OptionalInt), both values will just be null/absent. This is the same for Go. The existence of the type field does make handling thins case easier.

You can obviously get around this by inspecting the actual JSON (look for the presence/absence of the specific key), but that will probably be more overhead/work in the different languages. The same is true for handling the "unknown" case.

This isn't a hard blocker, but wanted to note it since this does make things more complicated than my initial assessment.

[raiju]

That's a good point, I hadn't thought of that case.

In Typescript, absent & null are different values (undefined/null), so that should just work. I think it's possible in Java/Python as null values end up with a null/None map entry (as opposed to no entry), but would have to double-check. Honestly not sure about Go & Rust though.

[sfackler]

It's not a problem in Rust - we have very low level access to the JSON structure so a null value is distinguishable from a missing one.

[maciekf]

+1 from Flows.

We have a DSL which allows users to define their workflows and we use unions in a few places (workflow state parameter types, expressions using the aforementioned parameters). The produced JSON/YAML is much bigger than necessary which makes it harder to read or edit.

[ericanderson]

Why not do something more like { __type: string, ...typesProperties }?

[sfackler]

That either requires us to guarantee that the __type field comes first in the serialized form, which can be hard to enforce through various layers of glue, or buffer all of the properties until we figure out what type to deserialize into.

[carterkozak]

That would not work for primitive types, not everything has properties.

[markelliot]

All unions encode the value in a specific field name, and the field name can be used to identify the type of its value (defitionally).

One might model union types as a shorthand for a single type with many optional fields and a guarantee that exactly one of those fields is non-empty (and thus present in the wire format).

[tom-s-powell]

Has there been any further discussion on this topic? Have a use-case where users will be constructing JSON queries that heavily involve union types and the non-type proposal would be far less verbose. Could something like this be a feature flag that a service can configure based on what best fits the use-case and intended client?

[markelliot]

@tom-s-powell I think we'd need to do some work to ensure that all deserializers across all the languages get to the point where type is ignored and then do a major rev on the protocol in order to stop producing the extra key. To my knowledge, none of the languages have the requisite changes in to use the field key as a differentiator for type instead of examining the type key..

[sfackler]

Rust implementation: palantir/conjure-rust@181e0a6