docs/design: Initial schema blocks support documentation

[bflad]

Reference: #85
See also: #188

[paddycarver]

I think I disagree with the recommendation on this one, explained in the comment I left. I think in my head, I envisioned the "blocks as a separate type" approach, because it more closely aligns with the abstractions we're surfacing.

It may be worth noting that our ultimate goal is removing support for blocks again.

I'm interested in the assertion that the recommended approach will allow for easier conversion from blocks to attributes in the future. Is it just because it can be done via find-and-replace instead of moving a field between two maps? Can we mitigate that advantage through automation?

[paddycarver]

My question here is that I think this could allow providers to define incorrect schemas that we can't model with the protocol. For example, a ListNestedAttributes could contain a tfsdk.Attribute that has a Blocks property on it, which, if memory serves, we can't actually model in the protocol; attributes can only have attributes under them.

I think allowing this in the type system when it's not supported in the protocol may be confusing.

[bflad]

I'm interested in the assertion that the recommended approach will allow for easier conversion from blocks to attributes in the future. Is it just because it can be done via find-and-replace instead of moving a field between two maps? Can we mitigate that advantage through automation?

Correct. Once you move beyond very simple code editing operations such as find-replace, at least with tooling that I've worked with like go/analysis, semgrep and rf, it becomes a bit of a fools errand to enumerate all the possible cases to introduce automation that catches it all. If you are familiar with other tooling, frameworks, or concepts that make this easier, would certainly like to hear more.

For example, its possible to declare things like this:

func (t exampleResourceType) GetSchema(ctx context.Context) (tfsdk.Schema, diag.Diagnostics) {
	return tfsdk.Schema{
		Blocks: map[string]tfsdk.Block{
			"example_block": {
				Attributes: map[string]tfsdk.Attribute{
					"example_attr": {
						Type:     types.StringType,
						Required: true,
					},
				},
				Nesting: tfsdk.BlockNestingList,
			},
		},
	}, nil
}

Or:

var exampleBlock := tfsdk.Block{
	Attributes: map[string]tfsdk.Attribute{
		"example_attr": {
			Type:     types.StringType,
			Required: true,
		},
	},
	Nesting: tfsdk.BlockNestingList,
}

func (t exampleResourceType) GetSchema(ctx context.Context) (tfsdk.Schema, diag.Diagnostics) {
	return tfsdk.Schema{
		Blocks: map[string]tfsdk.Block{
			"example_block": exampleBlock,
		},
	}, nil
}

Or:

func exampleBlock() tfsdk.Block {
	return tfsdk.Block{
		Attributes: map[string]tfsdk.Attribute{
			"example_attr": {
				Type:     types.StringType,
				Required: true,
			},
		},
		Nesting: tfsdk.BlockNestingList,
	}
}

func (t exampleResourceType) GetSchema(ctx context.Context) (tfsdk.Schema, diag.Diagnostics) {
	return tfsdk.Schema{
		Blocks: map[string]tfsdk.Block{
			"example_block": exampleBlock(),
		},
	}, nil
}

Plus others... Now the automation needs to worry about converting multiple types and fields that might be spread across the AST. go/analysis can pick up some of these cases with effort, but rewriting code in that framework generally needs to be able to compile between passes (in my experience).

There's also code like this that even further complicates things:

var listBlockMode := tfsdk.BlockNestingList
var exampleBlock := tfsdk.Block{
	Attributes: map[string]tfsdk.Attribute{
		"example_attr": {
			Type:     types.StringType,
			Required: true,
		},
	},
	NestingMode: listBlockMode,
}

func (t exampleResourceType) GetSchema(ctx context.Context) (tfsdk.Schema, diag.Diagnostics) {
	return tfsdk.Schema{
		Blocks: map[string]tfsdk.Block{
			"example_block": exampleBlock,
		},
	}, nil
}

The tooling then needs to know to introspect the value of the variable to extract its value. Worse would be a function call, which could get into SSA-land. Experimental tooling like rf might be able to do some things that are harder in go/analysis, but then you will likely need to mix and match tools, which is its own complexity.

To summarize the differences in terms of potential operations:

Blocks as New Attribute Field:

• Rename Blocks field identifier to Attributes
• Rename ListNestedBlocks/SetNestedBlocks call expressions to ListNestedAttributes/SetNestedAttributes
• Rename ListNestedBlocksOptions/SetNestedBlocksOptions declarations to ListNestedAttributesOptions/SetNestedBlocksOptions
• Either default to adding Required/Optional/Computed field identifier(s) and true value(s) or its a manual step for provider developers

Blocks as a Separate Type:

• Convert map[string]Block declaration to map[stringAttribute
• Convert Block declaration to Attribute
  • Wrap Attributes with call expression (e.g. ListNestedAttributes) based on NestingMode
    • Migrate MinItems and MaxItems to options fields (e.g. ListNestedAttributesOptions) or Validators
  • Remove NestingMode field identifier and value
• Rename Blocks field identifier to Attributes
  • If Attributes exists already, append the block's attributes to existing Attributes map (yikes)
• Either default to adding Required/Optional/Computed field identifier(s) and true value(s) or its a manual step for provider developers

All these differences are also reflected in the complexity required to work with both concepts in a provider, meaning multiple mental models are necessary to work with code using both. Whether that's a good or bad thing, I think is debatable, since they are conceptually different in many regards. However, this is all predicated on my presumption that since block support is deprecated, we do not need to worry about drastic changes to the protocol regarding them, which would in turn make the abstraction difficult to maintain over time. If that presumption is wrong, would love to know that.

---

My question here is that I think this could allow providers to define incorrect schemas that we can't model with the protocol. ... I think allowing this in the type system when it's not supported in the protocol may be confusing.

This can be solved with an additional schema validation, but it is annoying that it wouldn't be caught at compile time.

---

All this said, the current recommendation is not a hill I'm looking to defend strongly. I do, however, worry much about the necessary complexity for provider developers working with both Attributes and Blocks and any future migration strategies we want to employ.

[paddycarver]

If you are familiar with other tooling, frameworks, or concepts that make this easier, would certainly like to hear more.

Nope, I think your breakdown matches my understanding of the situation. I think I'm wagering that most cases will not be that complicated, though, and can be automated, and for sufficiently common cases we'll be able to automate it, even if it's annoying. That may be wrong, but I think you're right that it's worth saying out loud, and it certainly is a trade off being made here.

Convert Block declaration to Attribute

I think this is making some assumptions about how the design of the Block type that may or may not apply. Could we not make the Block type similar enough to the Attribute type to mitigate many of these migrations?

If Attributes exists already, append the block's attributes to existing Attributes map (yikes)

Judging by the "(yikes)" I think you're considering this a bigger deal than I am, and I'd be interested to hear more about that.

All these differences are also reflected in the complexity required to work with both concepts in a provider, meaning multiple mental models are necessary to work with code using both. Whether that's a good or bad thing, I think is debatable, since they are conceptually different in many regards.

I think this is the thrust of it, to me. I think they're separate concepts, and that complexity is necessary (see hashicorp/terraform-plugin-sdk#201, etc.) and so making them interchangeable--while it is beneficial for the mechanics of migration--risks making the concepts less distinct and makes it harder to illustrate their capabilities and differences.

However, this is all predicated on my presumption that since block support is deprecated, we do not need to worry about drastic changes to the protocol regarding them, which would in turn make the abstraction difficult to maintain over time. If that presumption is wrong, would love to know that.

I wouldn't anticipate drastic changes to the protocol, no.

This can be solved with an additional schema validation, but it is annoying that it wouldn't be caught at compile time.

I think that last bit is my concern. Catching things only at runtime, when you have a working provider, lengthens the feedback cycle and means that mistakes are caught further from where they are made, which I think makes it harder to understand what the mistake was and makes it more frustrating to learn.

I think you're right that my proposal is a harder migration path. I think the trade off I'm advocating for--which may be wrong--is that the harder migration path is worth the clearer delineation of concepts and capabilities, because the migration path is less frequently used (I think?) than the non-migration path that the clearer delineation of concepts and capabilities will benefit.

I think the reason I'm okay with making the migration path harder is because I think I'm already considering it a path that has an active human operator, because it's creating breaking changes for users. Maybe we should work with the core team to make that not the case, and maybe that changes the calculus. 🤔 I'm also curious how much of that pain (in practice, I agree in theory it's intractable) can be pragmatically wiped away with automation. I don't know that I have an answer to that offhand, just a suspicion, and I'm not sure if it's worth the effort to find the answer (which, for providers in the registry at least, is theoretically findable?).

I don't know. What do you think?

[bflad]

Let's go with a clean abstraction recommendation so the framework matches other similar design decisions that lean towards compile-time feedback of capabilities, which probably should outweigh any migration considerations as that space could improve over time and since you correctly mention would likely require human developer interaction anyways. I'll update this shortly.

[bflad]

I have updated the design documentation and its worth noting that it currently states:

data access will be no different than existing List or Set of Object handling (similar to the nested attribute counterparts)

I think this is doable, but I do need to note in this document that we need to introduce internal schema validation that prevents an attribute and block of the same name, since they would be separate maps.

[paddycarver]

I think this is doable, but I do need to note in this document that we need to introduce internal schema validation that prevents an attribute and block of the same name, since they would be separate maps.

I think they are represented the same way in the request bodies that Terraform sends over, so I'm suspicious that Terraform would complain about it even if we didn't, but I think that's a good callout.

[paddycarver]

Think I'm on board here.

I think there's an interesting direction to be explored in making Blocks behave like NestedAttributes; i.e., instead of having a NestingMode, have ListOfBlocks, SetOfBlocks, and SingleBlock, or whatever. But I think that's a small enough design detail that it can 1) be left up to the implementer to play around with and 2) be talked about as part of the PR review.

[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 contributions.
If you have 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.