Collection types should be optional

[alexthemark]

What happened?

When a conjure definition contains a collection (e.g. a list), that collection should appear as optional in the generated typescript. This leads to consistency between the java and typescript implementations, where an undefined collection is defaulted to an empty collection.

Take the following example:

MyObjectType:
  id: string
  myField: list<string>

will generate

interface MyObjectType {
  id: string;
  myField: string[];
}

In typescript, the following isn't valid:

const foo: MyObjectType = {
  id: "foo"
}

because it is missing myField. When this is a request body, this isn't accurate -- since the server will happily deserialize the example object above.

Also, in the generated Java, it's valid to write:

MyObjectType example = MyObjectType.builder()
        .id("foo")
        .build();

Because of this difference, a number of our typescript developers want to mark all collections as optional -- and java developers don't want that, as optional collections are a java anti-pattern.

What did you want to happen?

When the conjure-typescript generator generators a collection type in typescript, it should always be marked optional.

[ferozco]

I don't think we should do this. Because of the asymmetry in requests and responses marking all collections as optional would force consumers to do spurious null checks when interacting with any response that includes a collection. Its also a pretty big break since all any client that updates would be forced to handle these hypothetical nulls.

We are able to get away with this in Java, because of the the builder abstraction which defaults absent collection to empty. With out the abstraction, the following would NPE:

MyObjectType example = MyObjectType.builder()
        .id("foo")
        .build();
example.myField.size()

[ferozco]

I think long term introducing a serde layer, that would allow us to do what you suggest, is the right to do. Unfortunately, that would come with a much more heavy weight run-time and verbose code gen

[alexthemark]

Is there a standard best practice on whether collection types should be marked as optional (and Java developers therefore need to deal with an additional optional), or whether typescript developers should be forced to include empty collections in their requests?

[carterkozak]

We should focus on writing our conjure APIs to most accurately describe the data we want to send over the wire. If that results in sub-optimal developer experience, we have the tools to code-gen the problem away in a typesafe way. As @ferozco mentioned, I think a serde layer would solve this nicely, and provide the leverage to solve the integer64 problem as well.

[alexthemark]

For MyObjectType above, is {"id": "foo"} valid over the wire?

It would currently be accepted without issue by a java server (which would add on the empty collection in its deserialization), but would not match the expectations of a typescript client.

[ferozco]

You're correct, it is compatible over the wire. Perhaps as a middle-ground/starting point we could generate simple factory functions while will allow you to omit collections. For example:

interface MyObjectType {
  id: string;
  myField: string[];
}

function createMyObjectType({ id, myField = [] }: MyObjectType): MyObjectType {
   return { id, myField };
}

This approach would not protect against servers that omit empty collections or help solve issues like the integer64 problem but it could alleviate some of the client side pain.

[carterkozak]

I wonder, would it be reasonable to generate ts code which can exclude values for collection fields in types that are only used in requests, not responses? Probably more confusing than it's worth...

[ferozco]

I would like to keep code gen consistent regardless of how the objects are used. Does the above proposal sound reasonable to you @alexthemark?

[ericanderson]

We do not need more code to create simple objects

[ericanderson]

Also that defaults object solution wont even work: http://www.typescriptlang.org/play/#src=interface%20MyObjectType%20%7B%0A%20%20id%3A%20string%3B%0A%20%20myField%3A%20string%5B%5D%3B%0A%7D%0A%0Afunction%20createMyObjectType(%7B%20id%2C%20myField%20%3D%20%5B%5D%20%7D%3A%20MyObjectType)%3A%20MyObjectType%20%7B%0A%20%20%20return%20%7B%20id%2C%20myField%20%7D%3B%0A%7D%0A%0A%0AcreateMyObjectType(%7B%0A%20%20%20%20id%3A%20%22hi%22%0A%7D)%3B%0A

[alexthemark]

I don't think that necessarily works -- I think it's reasonable to say that I could also be running a node server that's dependent on the typescript bindings. At this point, my node server needs to handle the collection being undefined over the wire.

Unless I deserialize the object through some sort of function like that one, I actually do have to worry about doing an undefined check anywhere that I have a collection type.

[iamdanfox]

I don't think the proposal of changing all collections to myField?: string[] is desirable because it would wreck the usability of responses.

For maximum convenience on the FE side, you want the opposite things for requests and responses:

• When building requests, you want the compiler to permit collections to be omitted.
• When interacting with responses, you want collections to always be present so you don't have to litter your codebase with if (foo.myField) checks.
• I'm not sure what the desired behaviour is if you have a type that appears in both requests and responses (e.g. some generic User or Policy).

Given that people already seem to hand-write PartialFoo interfaces and buildFoo(partial: PartialFoo): Foo convenience methods, why don't we just get conjure-typescript to generate these?

function buildStoreRequest(partialRequest: PartialRequest): IStoreArtifactRequest {
  const baseRequest = {
    addRelations: {},
    removeRelations: {},
    removeRelationTypes: [],
    addObjectIds: [],
    removeObjectIds: [],
    removeTags: [],
    tags: [],
  };

  return Object.assign({}, baseRequest, partialRequest);
}

To be fully defensive against the hypothetical node-server example, I think we'd really need a proper serde layer (#48) that recursively fixes the result of JSON.parse to make sure collections are definitely present. This incurs a runtime and code-size cost that we don't have to pay right now because all our java servers happen to always serialize [].

[bmarcaur]

This is causing issues where one of our applications loads a serialized document in both Java and TS. There are cases where the document can be loaded by the Java client no problem, but fails with a runtime error when being loaded from TS.

This behavior is in direct conflict with the conjure spec https://github.com/palantir/conjure/blob/master/docs/spec/wire.md#561-coercing-json-null--absent-to-conjure-types which states:

5.6.1. Coercing JSON null / absent to Conjure types

If a JSON key is absent or the value is null, two rules apply:

• Conjure optional, list, set, map types must be initialized to their empty variants,
• Attempting to coerce null/absent to any other Conjure type must cause an error, i.e. missing JSON keys must cause an error.

This means without some serde layer, or other changes, the generated TS is not a compliant conjure client. The spec is under-defined when it comes to the responsibility of serializing keys for empty collection objects. So it is likely safe to assume that since the spec requires clients to handle the absence of these keys it would be completely valid to omit them. It is an implementation detail that most of our servers are implemented in Java using Jackson which enables WRITE_EMPTY_JSON_ARRAYS, WRITE_NULL_MAP_VALUES, etc by default.

Could we take the opposite approach and make it part of the spec that all non-optional fields are required to be serialized over the wire? This would compensate for various tech stacks that might not be able to support missing keys at runtime.