Maintain dependencies between required construct properties

[jsteinich]

Fixes #188 Also eliminates #214

• Switch to always return attribute readers.
• Opted to not expose any set value publicly since there isn't a good indication of when it will work in local code.
• Changed behavior of attribute lookup of unknown type so that it has a chance of working correctly once synthesized.
• Use explicit required value from schema rather than assuming inverse of optional.

This ended being a lot more complicated than I expected: partially because providers and resources/data sources shared the same code, but mostly because of the complexities (and inconsistencies) of schema definition.

I'm pretty sure I fixed a couple bugs, but I'm not confident that I didn't introduce new ones, so I want to do some more testing before switching from draft.

[skorfmann]

Cool - thanks for your initiative 👍 Just looked briefly into this and will test it locally later today.

This ended being a lot more complicated than I expected: partially because providers and resources/data sources shared the same code, but mostly because of the complexities (and inconsistencies) of schema definition.

Yes, there's lots of headroom for improvements and refactoring around the schema generation 👍

I'm pretty sure I fixed a couple bugs, but I'm not confident that I didn't introduce new ones, so I want to do some more testing before switching from draft.

Makes sense, will try it as well

[jsteinich]

I checked diffs against all of the pre-built provider repos. Changes appear to be what I expected them to be.
Some updates on the changes:

• jsii-diff will show incompatible because id was generally changed to be immutable. It was already not in the synth, so the change better reflects what it means.
  • In some spots id was actually added to the synth because it is actually mutable.
• I added a public getter of the form propertyNameInput for accessing the raw value for mutable properties. This isn't intended to exist long term: just a workaround until more complete Token support is added.
  • Naming restrictions prevented using _

[skorfmann]

• In some spots id was actually added to the synth because it is actually mutable.

Do you have an example for this?

[jsteinich]

• In some spots id was actually added to the synth because it is actually mutable.

Do you have an example for this?

https://www.terraform.io/docs/providers/docker/d/docker_network.html

[skorfmann]

https://www.terraform.io/docs/providers/docker/d/docker_network.html

Right, and that's now being built correctly?

[jsteinich]

https://www.terraform.io/docs/providers/docker/d/docker_network.html

Right, and that's now being built correctly?

Yep.
diff.txt

[skorfmann]

Apparently there's typescript limitation around differently typed getters / setters :

// In DataAwsRoute53Zone
  // zone_id - computed: true, optional: true, required: false
  private _zoneId?: string;
  public get zoneId() {
    return this.getStringAttribute('zone_id');
  }
  public set zoneId(value: string | undefined) {
    this._zoneId = value;
  }

While the code seems to be correct, the typescript compiler doesn't accept it:

Looks like that both functions have to set and return the same type.

I'm wondering what the best way to handle this would be.

ps: And yes, it's a change compared to what we had before:

[jsteinich]

ps: And yes, it's a change compared to what we had before:

I'm not quite sure how we didn't have the same problem before since the getter and setter certainly seem to have different types.

I'm wondering what the best way to handle this would be.

I'm not sure. Given #233, I don't know if we can disallow setting an optional property to null. If we could, that could be a fairly straightforward option.

[jsteinich]

Also, I'll make sure to add some test cases to pick this up.

[skorfmann]

ps: And yes, it's a change compared to what we had before:

I'm not quite sure how we didn't have the same problem before since the getter and setter certainly seem to have different types.

I'm pretty sure we have similar issues - looks like the setter type wins in this case. It's just the type information getting mixed up.

I'm not sure. Given #233, I don't know if we can disallow setting an optional property to null. If we could, that could be a fairly straightforward option.

Yes, I think being able to set the value to null should be possible.

[jsteinich]

I want to look into it farther, but my initial digging makes it seem like null has primarily been used in module development and isn't used much otherwise. If that's true, perhaps we could making using a null a bit of a special case...

• Convert getter and setter to not allow null. That should make it very easy to use when you don't want to set something to null.
• Add a method for setting to null. This would be just like the current setter so that use cases that support both can still just call a single method for either case.
  • Might be able to name it setX, but that may have issues with jsii and Java

[skorfmann]

I want to look into it farther, but my initial digging makes it seem like null has primarily been used in module development and isn't used much otherwise. If that's true, perhaps we could making using a null a bit of a special case...

How would you remove an attribute which was set somewhere already? I think that's difficult, if you can't provide null.

• Convert getter and setter to not allow null. That should make it very easy to use when you don't want to set something to null.

This wouldn't feel natural to me. Null seems to be totally valid use case to me.

I'll also think a bit about it today, not sure what a good option would be right now.

[jsteinich]

There are several different types of attributes:

1. Required - these must be specified. I don't believe null is supported.
2. Optional - these can be specified, but aren't required to be. Null is allowed and sometimes required
3. Read only - these cannot be specified
4. Conditionally required/optional/disallowed - depending on what value you specify for a different attribute, the requirements change. This isn't represented in the schema and may make sense to let L2 constructs handle it.

HCL allows any attribute to be referenced. There are likely some cases where a reference causes an error at plan/apply time, but that seems outside the scope of cdktf

Given all the properties of attributes, the following seems ideal:

• Required attributes are represented by properties with getter/setter that don't support null/undefined
• Optional attributes are represented by properties with a getter that doesn't support null/undefined and a setter that does
• Read only attributes are represented by properties with only a getter that doesn't support null/undefined
• All attribute getters return a token rather than any value that they may have been set to. Resolution is left to Terraform in order to maintain proper dependencies

All of that is possible except for mixed type getter/setter (see microsoft/TypeScript#2521).
Given that the most natural usage isn't supported, I think we need to make a compromise somewhere.
Some options that I see:

1. Allow required attributes to be set to null / omitted
   • This would allow the incorrect null return of a getter to be passed
   • Would allow setting a value after constructor without having to pass a dummy value into the constructor
   • Would likely cause confusion as required state not enforced until Terraform runs
     • Validation could be added to synth phase
2. Replace the getter with a get function that doesn't return null and keep setter with support
   • Everything would match requirements
   • Rather unnatural to reference other resources
3. Replace the setter with a set function that supports null and make the getter not return null
   • Everything would match requirements
   • Rather unnatural to set properties after the constructor
4. Do nothing
   • Getter doesn't match requirements
   • Forced to assert non-null / null check with using reference
5. Number 2, except also keep a getter that returns null
   • Gives choice between downsides of 2 and 4
6. Number 3, except keep a setter that doesn't take null
   • Only unnatural to set properties when null is involved
7. Number 6, except rather than a set function, a clear function is provided that sets to null and/or code gen default
   • Very explicit about null usage for better or for worse

In my opinion, number 6 is the best option since it works as expected for everything besides using null, which I expect to be a relatively less common usage.

[skorfmann]

@jsteinich thanks for this great summary!

In my opinion, number 6 is the best option since it works as expected for everything besides using null, which I expect to be a relatively less common usage.

In the specific example I wrote earlier this would like this then?

  //  In DataAwsRoute53Zone
  // zone_id - computed: true, optional: true, required: false
  private _zoneId?: string;
  public get zoneId() {
    return this.getStringAttribute('zone_id');
  }
  public set zoneId(value: string) {
    this._zoneId = value;
  }

 // setting null like this? 

  public set resetZoneId {
    this._zoneId = null;
  }
// or
  public set zoneIdReset {
    this._zoneId = null;
  }

[jsteinich]

In the specific example I wrote earlier this would like this then?

Almost. The null support would be a bit different:

  //  In DataAwsRoute53Zone
  // zone_id - computed: true, optional: true, required: false
  private _zoneId?: string;
  public get zoneId() {
    return this.getStringAttribute('zone_id');
  }
  public set zoneId(value: string) {
    this._zoneId = value;
  }

  public setZoneId(value: string | undefined) {
    this._zoeId = value;
  }

The naming of the extra method might need to be adjusted to account for jsii Java code.

 public set resetZoneId {
    this._zoneId = null;
  }

is more like option 7.

[skorfmann]

  public setZoneId(value: string | undefined) {
    this._zoeId = value;
  }

Wouldn't this be invalid in Java or C# ? - I think I saw jsii complain about that naming schema.

[skorfmann]

The naming of the extra method might need to be adjusted to account for jsii Java code.

Sorry - didn't read the full comment :D

[skorfmann]

Why would you prefer something like

  public setZoneId(value: string | undefined) {
    this._zoeId = value;
  }

over this

 public set resetZoneId {
    this._zoneId = null;
  }

when the only use-case seems to be resetting a potentially set value to null?

[jsteinich]

when the only use-case seems to be resetting a potentially set value to null?

I was thinking there would be another use case for modules that optionally set data, but actually they would probably just do so in the props provided in the constructor.

Having the reset would probably work better with jsii naming, so I'm fine going that route.

[skorfmann]

I was thinking there would be another use case for modules that optionally set data, but actually they would probably just do so in the props provided in the constructor.

👍

Having the reset would probably work better with jsii naming, so I'm fine going that route.

Yes, I agree. That's probably the best solution within the given constraints.

[skorfmann]

Tried it in several local demos of mine, looks pretty good 👍

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days. This helps our maintainers find and focus on the active issues. If you've found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.