leaves12138 commented on code in PR #17:
URL:
https://github.com/apache/terraform-provider-paimon/pull/17#discussion_r3943504008
##########
internal/provider/resource_table.go:
##########
@@ -85,44 +87,97 @@ func (r *tableResource) Configure(_ context.Context, req
resource.ConfigureReque
}
func (r *tableResource) ModifyPlan(ctx context.Context, req
resource.ModifyPlanRequest, resp *resource.ModifyPlanResponse) {
- if req.State.Raw.IsNull() || req.Plan.Raw.IsNull() {
+ if req.Plan.Raw.IsNull() {
return
}
var config, state, plan tableResourceModel
resp.Diagnostics.Append(req.Config.Get(ctx, &config)...)
- resp.Diagnostics.Append(req.State.Get(ctx, &state)...)
+ if !req.State.Raw.IsNull() {
+ resp.Diagnostics.Append(req.State.Get(ctx, &state)...)
+ }
resp.Diagnostics.Append(req.Plan.Get(ctx, &plan)...)
if resp.Diagnostics.HasError() {
return
}
- var configuredFields, stateFields, plannedFields []tableFieldModel
- resp.Diagnostics.Append(config.Fields.ElementsAs(ctx,
&configuredFields, false)...)
- resp.Diagnostics.Append(state.Fields.ElementsAs(ctx, &stateFields,
false)...)
- resp.Diagnostics.Append(plan.Fields.ElementsAs(ctx, &plannedFields,
false)...)
- if resp.Diagnostics.HasError() {
+ stabilizeTableKeys(ctx, config, state, &plan, &resp.Diagnostics)
+ replacementPaths := make([]path.Path, 0)
+ if tableFieldsInspectable(config.Fields) &&
tableFieldsInspectable(plan.Fields) {
+ var configuredFields, stateFields, plannedFields
[]tableFieldModel
+ resp.Diagnostics.Append(config.Fields.ElementsAs(ctx,
&configuredFields, false)...)
+ if !req.State.Raw.IsNull() {
+ resp.Diagnostics.Append(state.Fields.ElementsAs(ctx,
&stateFields, false)...)
+ }
+ resp.Diagnostics.Append(plan.Fields.ElementsAs(ctx,
&plannedFields, false)...)
+ if resp.Diagnostics.HasError() {
+ return
+ }
+ if len(configuredFields) != len(plannedFields) {
+ resp.Diagnostics.AddError("Unable to stabilize Paimon
field identities", "The configured and planned field lists have different
lengths. Please report this issue to the provider developers.")
+
+ return
+ }
+
+ stabilizePlannedFieldIdentities(configuredFields, stateFields,
plannedFields)
+ if !req.State.Raw.IsNull() {
+ validateAddedFieldIDs(stateFields, configuredFields,
&resp.Diagnostics)
+ }
+ stabilizeFieldNullability(ctx, configuredFields, stateFields,
plannedFields, plan, state, &resp.Diagnostics)
+ plan.Fields = fieldsValueFromModels(ctx, plannedFields,
&resp.Diagnostics)
+ if resp.Diagnostics.HasError() {
+ return
+ }
+ if !req.State.Raw.IsNull() {
+ keyFields := knownTableKeyNames(state.PartitionKeys,
state.PrimaryKeys, plan.PartitionKeys, plan.PrimaryKeys)
+ if compositeFieldTypesRequireReplace(stateFields,
plannedFields) ||
+ keyFieldTypesRequireReplace(stateFields,
plannedFields, keyFields) ||
+ newNonNullableFieldsRequireReplace(stateFields,
plannedFields) {
+ replacementPaths = append(replacementPaths,
path.Root("fields"))
+ }
+ }
+
+ }
+ resp.Diagnostics.Append(resp.Plan.Set(ctx, &plan)...)
+ if req.State.Raw.IsNull() || resp.Diagnostics.HasError() {
return
}
- if len(configuredFields) != len(plannedFields) {
- resp.Diagnostics.AddError("Unable to stabilize Paimon field
identities", "The configured and planned field lists have different lengths.
Please report this issue to the provider developers.")
+ if !plan.PrimaryKeys.IsUnknown() &&
!state.PrimaryKeys.Equal(plan.PrimaryKeys) {
+ replacementPaths = append(replacementPaths,
path.Root("options").AtMapKey("primary-key"))
+ }
+ if !plan.PartitionKeys.IsUnknown() &&
!state.PartitionKeys.Equal(plan.PartitionKeys) {
Review Comment:
**[P1] Enforce replacement opt-in for wholly unknown partition keys**
When the entire `partition_keys` value is unknown, this condition skips the
opt-in check, but the attribute's
`listplanmodifier.RequiresReplaceIfConfigured()` has already requested
replacement. Terraform can consequently destroy/recreate the table with
`allow_replacement = false`; replanning the replacement as a create does not
rescue the check because that path has null prior state.
For example, apply this configuration against an existing `analytics`
database, then change only `terraform_data.partition.input` from `["tenant"]`
to `["region"]`:
```hcl
resource "terraform_data" "partition" {
input = ["tenant"]
}
resource "paimon_table" "events" {
database = "analytics"
name = "events"
fields = [
{ name = "tenant", type = "STRING" },
{ name = "region", type = "STRING" }
]
partition_keys = terraform_data.partition.output
allow_replacement = false
}
```
I reproduced this at `df3f709` using `acceptanceCatalog` and Terraform
1.13.5. The second apply succeeds instead of returning `Destructive table
change is disabled`; the fixture records **two table creates**, confirming that
the existing table was replaced without opt-in. The newly added test only
covers a partially unknown list, for which `IsUnknown()` is false, so it misses
this case.
Please make the opt-in cover attribute-level replacement decisions as well,
including wholly unknown values, and add a protocol acceptance test that
verifies the second apply is rejected without replacing the original table. The
same interaction should also be checked for the `name`/`database` replacement
modifiers.
##########
internal/provider/resource_table.go:
##########
@@ -85,44 +87,97 @@ func (r *tableResource) Configure(_ context.Context, req
resource.ConfigureReque
}
func (r *tableResource) ModifyPlan(ctx context.Context, req
resource.ModifyPlanRequest, resp *resource.ModifyPlanResponse) {
- if req.State.Raw.IsNull() || req.Plan.Raw.IsNull() {
+ if req.Plan.Raw.IsNull() {
return
}
var config, state, plan tableResourceModel
resp.Diagnostics.Append(req.Config.Get(ctx, &config)...)
- resp.Diagnostics.Append(req.State.Get(ctx, &state)...)
+ if !req.State.Raw.IsNull() {
+ resp.Diagnostics.Append(req.State.Get(ctx, &state)...)
+ }
resp.Diagnostics.Append(req.Plan.Get(ctx, &plan)...)
if resp.Diagnostics.HasError() {
return
}
- var configuredFields, stateFields, plannedFields []tableFieldModel
- resp.Diagnostics.Append(config.Fields.ElementsAs(ctx,
&configuredFields, false)...)
- resp.Diagnostics.Append(state.Fields.ElementsAs(ctx, &stateFields,
false)...)
- resp.Diagnostics.Append(plan.Fields.ElementsAs(ctx, &plannedFields,
false)...)
- if resp.Diagnostics.HasError() {
+ stabilizeTableKeys(ctx, config, state, &plan, &resp.Diagnostics)
+ replacementPaths := make([]path.Path, 0)
+ if tableFieldsInspectable(config.Fields) &&
tableFieldsInspectable(plan.Fields) {
+ var configuredFields, stateFields, plannedFields
[]tableFieldModel
+ resp.Diagnostics.Append(config.Fields.ElementsAs(ctx,
&configuredFields, false)...)
+ if !req.State.Raw.IsNull() {
+ resp.Diagnostics.Append(state.Fields.ElementsAs(ctx,
&stateFields, false)...)
+ }
+ resp.Diagnostics.Append(plan.Fields.ElementsAs(ctx,
&plannedFields, false)...)
+ if resp.Diagnostics.HasError() {
+ return
+ }
+ if len(configuredFields) != len(plannedFields) {
+ resp.Diagnostics.AddError("Unable to stabilize Paimon
field identities", "The configured and planned field lists have different
lengths. Please report this issue to the provider developers.")
+
+ return
+ }
+
+ stabilizePlannedFieldIdentities(configuredFields, stateFields,
plannedFields)
+ if !req.State.Raw.IsNull() {
+ validateAddedFieldIDs(stateFields, configuredFields,
&resp.Diagnostics)
+ }
+ stabilizeFieldNullability(ctx, configuredFields, stateFields,
plannedFields, plan, state, &resp.Diagnostics)
+ keyFields := append(stringListFromValue(ctx,
state.PartitionKeys, &resp.Diagnostics), stringListFromValue(ctx,
state.PrimaryKeys, &resp.Diagnostics)...)
+ keyFields = append(keyFields, stringListFromValue(ctx,
plan.PartitionKeys, &resp.Diagnostics)...)
Review Comment:
Verified fixed at df3f709: my original protocol acceptance reproducer with
known fields and `partition_keys = [terraform_data.partition.output]` now
passes. The new null-element validation and mixed known/unknown replacement
coverage also pass. I found a separate whole-list-unknown replacement opt-in
bypass during follow-up testing and reported it in the new review; this
original creation-conversion finding itself is addressed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
