Include optional keys when serializing objects

[rahij]

What happened?

According to the conjure spec, it is not required to include keys for optional and collection types in the serialized response if they are empty. However, this means that conjure-typescript clients will break at runtime when accessing empty collections e.g (response.items.map(...)) will throw an error like (cannot access map of undefined).

See here for the conjure-typescript issue: palantir/conjure-typescript#78

What did you want to happen?

To overcome this limitation, conjure-java already allows including these keys regardless of if they are empty (and is also the default): https://github.com/palantir/conjure-java/blob/develop/conjure-java/src/main/java/com/palantir/conjure/java/cli/ConjureJavaCli.java#L206-L209. Could we do the same in conjure-rust?

[sfackler]

What is the behavior of the Go backend?

This seems like something that should be fixed on the typscript side of things, yeah??

[rahij]

Not sure about the go backend, but unfortunately it can't be handled in typescript because when the response comes back to the browser, there is only javascript with no type information - so unless all the collection types are also marked as optional in the typescript code (which would be quite bad ergonomics), it would fail at runtime? I'm reasonably sure all internal apps had this issue when conjure java started omitting these in the response, which was the reason they switched back.

[rahij]

found this issue in conjure-go that alludes to the same problem :palantir/conjure-go#73. It looks like they do include keys for missing fields but send them as null instead of a zero element collection, which still has the issue in typescript.

[sfackler]

The Typescript that Conjure codegens knows the types - can't it patch the objects properly?

If it's truly not possible for Typescript to support the Conjure spec, then it seems like we should actually change that to prohibit empty collection omission rather than patching every implementation to do the permitted-but-not-recommended thing.

[rahij]

On the typescript side, the generated interface currently look like this:

ISomeObject {
  foo: string,
  items: ISomeItem[];
}

From my understanding, in order for a response like this to be safely read:

{"foo": "bar"}

The generated object needs to be:

ISomeObject {
  foo: string,
  items: ISomeItem[] | undefined;
}

which requires all consumer sites to do an additional nullability check.

I agree that changing the spec seems to be the easiest solution here, which is also what the last comment in the conjure-typescript issue as well as the conjure-go issue allude do. However until then, since technically, not omitting does not violate the spec either, it would be very helpful for us to make the change in conjure-rust.

[sfackler]

Can you not have the deserialization glue have a postprocessing step that ensures that items is not undefined?

[rahij]

I think that's what the "serde layer" in palantir/conjure-typescript#78 (comment) alludes to, but I am not sure how straightforward that change is to make.

[rahij]

Hi @sfackler, Just wanted to follow up on this to see if there's appetite to make the change in conjure-rust, as changing the spec or adding a serde layer in conjure-typescript requires a larger stream of work/building consensus.

[sfackler]

I think we should build consensus on how to handle this appropriately instead of tweaking codegen in individual languages.