cdklabs/json2jsii

Union types accept "any" value

Closed this issue · 4 comments

When referencing the value of a generated union type, such as k8s.Quantity, the value is an any type. This can lead to unexpected output that does not meet the schemas being rendered at runtime, due to the lack of type checking.

For example, given a k8s.EnvVar like:

const cpu = Quantity.fromNumber(2)
{
  name: "GOMAXPROCS",
  // EnvVar.value is of type 'string | undefined'
  // cpu.value is of type 'any', which fulfills 'undefined'
  value: cpu.value,
},

Because type checking is skipped for an any, and cpu was created from a number, when this value is rendered to JSON it is still a number, and thus the rendered JSON is invalid:

// Actual, invalid result:
{
  "name": "GOMAXPROCS",
  "value": 2
}
// Desired, valid result:
{
  "name": "GOMAXPROCS",
  "value": "2"
}

In order to provide useful type data, instead of any, I propose that the type of the union's value should be a union of the possible input types.

With this change, the example from above now will fail type checking, and provide a useful error:

// This code now fails to compile
const cpu = Quantity.fromNumber(2)
{
  name: "GOMAXPROCS",
  // EnvVar.value is of type 'string | undefined'
  // cpu.value is of type 'string | number', which cannot fulfill the expected type
  value: cpu.value,
},
// This code compiles and results in valid JSON that meets the schemas
const cpu = Quantity.fromNumber(2)
{
  name: "GOMAXPROCS",
  value: cpu.value.toString(),
},

Hi @heckler1 I've seen your PR. For this particular issue, wouldn't it be much better if we handled undefined correctly. That should also work nicely in all jsii languages?

Hi there!

Allowing the use of undefined implicitly, due to the any type, leaves all typing guardrails up to the user and results in a lossy conversion from the original JSON Schemas to JSII types. By leveraging the support for unions of primitive types that's already part of the JSII spec, we can restrict the values to their actual valid type, as given originally by the schema.

For types or fields where one possible value is undefined (e.g. optional), if a user wants to set an undefined value, that will still be handled correctly. In this instance, properly setting a union type also gets us the benefit of the type system restricting when an undefined value is set for a required field, again more closely representing the original source material.

If I've misunderstood what you intended here, happy to continue discussing.

again more closely representing the original source material.

Could you share the source material for the scenario of this issue?

This case is, as shown in the original example, to help handle parsing of the special-case union types that are part of the Kubernetes API spec. There are two that I'm aware of right now, Quantity (as seen above) and IntOrString. Both can be submitted to the API as either a number or a string: https://github.com/cdk8s-team/cdk8s/blob/master/kubernetes-schemas/v1.25.0/_definitions.json#L15888

These special union types are only valid as a string or as a number per the original spec, and so an any type is not particularly representative of a value that would be considered valid in the schema.