From 94454e969d32d50dfafa3747f2bcf9cd4295e48e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yohann=20Br=C3=A9doux?= Date: Thu, 25 Sep 2025 14:57:35 +0200 Subject: [PATCH 1/8] fix: handle profiles warehouse schemaName Only send schemaName to API if it differs from the remote state This prevents API failures when the schema name already exists in the warehouse. The Segment API fails if we send a schemaName that matches the current configuration, even though it should be a no-op --- docs/resources/profiles_warehouse.md | 12 + .../provider/profiles_warehouse_resource.go | 24 +- .../profiles_warehouse_resource_test.go | 295 ++++++++++++++++++ 3 files changed, 330 insertions(+), 1 deletion(-) diff --git a/docs/resources/profiles_warehouse.md b/docs/resources/profiles_warehouse.md index 22a0735..beb6188 100644 --- a/docs/resources/profiles_warehouse.md +++ b/docs/resources/profiles_warehouse.md @@ -53,6 +53,16 @@ resource "segment_profiles_warehouse" "example" { } ``` +## Schema Name Handling + +The `schema_name` attribute includes special handling to prevent API failures: + +- **When updating**: The `schema_name` is only sent to the Segment API if it differs from the current remote value +- **When unchanged**: If the local and remote `schema_name` values are the same (including both being null/undefined), the field is omitted from the API request +- **When different**: If either the local or remote value is null/undefined while the other has a value, or if they have different values, the field is included in the API request + +**Important**: The Segment API fails when you send a `schema_name` that already exists in the warehouse configuration, even though it should be a no-operation. This behavior prevents those API failures while still allowing legitimate schema name changes. + ## Schema @@ -75,6 +85,8 @@ resource "segment_profiles_warehouse" "example" { - `enabled` (Boolean) Enable to allow this Warehouse to receive data. Defaults to true. - `name` (String) An optional human-readable name for this Warehouse. - `schema_name` (String) The custom schema name that Segment uses on the Warehouse side. The space slug value is default otherwise. + + **Note:** When updating a profiles warehouse, the `schema_name` is only sent to the Segment API if it differs from the current remote value. This prevents API failures that occur when sending a schema name that already exists in the warehouse configuration. ### Read-Only diff --git a/internal/provider/profiles_warehouse_resource.go b/internal/provider/profiles_warehouse_resource.go index bf9d47a..92ab582 100644 --- a/internal/provider/profiles_warehouse_resource.go +++ b/internal/provider/profiles_warehouse_resource.go @@ -15,6 +15,7 @@ import ( "github.com/hashicorp/terraform-plugin-framework/resource/schema" "github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier" "github.com/hashicorp/terraform-plugin-framework/resource/schema/stringplanmodifier" + "github.com/hashicorp/terraform-plugin-framework/types" "github.com/segmentio/public-api-sdk-go/api" ) @@ -228,11 +229,21 @@ func (r *profilesWarehouseResource) Update(ctx context.Context, req resource.Upd return } + // Only send schemaName to API if it differs from the remote state + // This prevents API failures when the schema name already exists in the warehouse. + // The Segment API fails if we send a schemaName that matches the current configuration, + // even though it should be a no-op. This handles all cases: + // 1. Both null/undefined: Equal() returns true, schemaName stays nil (not sent) + // 2. Both have same value: Equal() returns true, schemaName stays nil (not sent) + // 3. One null, other has value: Equal() returns false, schemaName gets the plan value (sent) + // 4. Both have different values: Equal() returns false, schemaName gets the plan value (sent) + schemaName := determineSchemaNameForUpdate(plan.SchemaName, state.SchemaName) + out, body, err := r.client.ProfilesSyncAPI.UpdateProfilesWarehouseForSpaceWarehouse(r.authContext, state.SpaceID.ValueString(), state.ID.ValueString()).UpdateProfilesWarehouseForSpaceWarehouseAlphaInput(api.UpdateProfilesWarehouseForSpaceWarehouseAlphaInput{ Enabled: plan.Enabled.ValueBoolPointer(), Settings: settings, Name: plan.Name.ValueStringPointer(), - SchemaName: plan.SchemaName.ValueStringPointer(), + SchemaName: schemaName, }).Execute() if body != nil { defer body.Body.Close() @@ -352,3 +363,14 @@ func findProfileWarehouse(authContext context.Context, client *api.APIClient, id return nil, nil } + +// determineSchemaNameForUpdate determines whether schemaName should be sent to the API +// based on comparing the plan and state values. This prevents API failures when the +// schema name already exists in the warehouse configuration. +func determineSchemaNameForUpdate(planSchemaName, stateSchemaName types.String) *string { + // Only send schemaName to API if it differs from the remote state + if !planSchemaName.Equal(stateSchemaName) { + return planSchemaName.ValueStringPointer() + } + return nil +} diff --git a/internal/provider/profiles_warehouse_resource_test.go b/internal/provider/profiles_warehouse_resource_test.go index 975eea2..1581f8e 100644 --- a/internal/provider/profiles_warehouse_resource_test.go +++ b/internal/provider/profiles_warehouse_resource_test.go @@ -5,6 +5,7 @@ import ( "net/http/httptest" "testing" + "github.com/hashicorp/terraform-plugin-framework/types" "github.com/hashicorp/terraform-plugin-testing/helper/resource" ) @@ -230,3 +231,297 @@ func TestAccProfilesWarehouseResource(t *testing.T) { }, }) } + +func TestAccProfilesWarehouseResource_SchemaNameHandling(t *testing.T) { + // Test the schemaName handling that prevents API failures when the schema + // name already exists in the warehouse configuration + t.Parallel() + + updateCount := 0 + fakeServer := httptest.NewServer( + http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + w.Header().Set("content-type", "application/json") + + payload := "" + if req.URL.Path == "/spaces/my-space-id/profiles-warehouses" && req.Method == http.MethodPost { + // Initial create response + payload = ` + { + "data": { + "profilesWarehouse": { + "id": "my-warehouse-id", + "spaceId": "my-space-id", + "workspaceId": "my-workspace-id", + "enabled": true, + "metadata": { + "id": "my-metadata-id", + "slug": "snowflake", + "name": "Snowflake", + "description": "Data warehouse built for the cloud", + "logos": { + "default": "https://cdn.filepicker.io/api/file/JrQWOYvMRRCVvSHp4HL0", + "mark": "https://cdn.filepicker.io/api/file/OBhrGoCRKaSyvAhDX3fw", + "alt": "" + }, + "options": [] + }, + "settings": { + "name": "My warehouse name", + "token": "my-token" + }, + "schemaName": "my-schema-name" + } + } + } + ` + } else if req.URL.Path == "/spaces/my-space-id/profiles-warehouses/my-warehouse-id" && req.Method == http.MethodPatch { + // Update response - schemaName should only be sent when it changes + updateCount++ + payload = ` + { + "data": { + "profilesWarehouse": { + "id": "my-warehouse-id", + "spaceId": "my-space-id", + "workspaceId": "my-workspace-id", + "enabled": false, + "metadata": { + "id": "my-metadata-id", + "slug": "snowflake", + "name": "Snowflake", + "description": "Data warehouse built for the cloud", + "logos": { + "default": "https://cdn.filepicker.io/api/file/JrQWOYvMRRCVvSHp4HL0", + "mark": "https://cdn.filepicker.io/api/file/OBhrGoCRKaSyvAhDX3fw", + "alt": "" + }, + "options": [] + }, + "settings": { + "name": "My new warehouse name", + "token": "my-other-token" + }, + "schemaName": "my-new-schema-name" + } + } + } + ` + } else if req.URL.Path == "/spaces/my-space-id/profiles-warehouses" && req.Method == http.MethodGet { + // Read response + payload = ` + { + "data": { + "profilesWarehouses": [ + { + "id": "my-warehouse-id", + "spaceId": "my-space-id", + "workspaceId": "my-workspace-id", + "enabled": true, + "metadata": { + "id": "my-metadata-id", + "slug": "snowflake", + "name": "Snowflake", + "description": "Data warehouse built for the cloud", + "logos": { + "default": "https://cdn.filepicker.io/api/file/JrQWOYvMRRCVvSHp4HL0", + "mark": "https://cdn.filepicker.io/api/file/OBhrGoCRKaSyvAhDX3fw", + "alt": "" + }, + "options": [] + }, + "settings": { + "name": "My warehouse name", + "token": "my-token" + }, + "schemaName": "my-schema-name" + } + ] + } + } + ` + } + + _, _ = w.Write([]byte(payload)) + }), + ) + defer fakeServer.Close() + + providerConfig := ` + provider "segment" { + url = "` + fakeServer.URL + `" + token = "abc123" + } + ` + + resource.Test(t, resource.TestCase{ + ProtoV6ProviderFactories: testAccProtoV6ProviderFactories, + Steps: []resource.TestStep{ + // Create with schema_name + { + Config: providerConfig + ` + resource "segment_profiles_warehouse" "test" { + space_id = "my-space-id" + metadata_id = "my-metadata-id" + name = "My warehouse name" + enabled = true + settings = jsonencode({ + "token": "my-token" + }) + schema_name = "my-schema-name" + } + `, + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("segment_profiles_warehouse.test", "schema_name", "my-schema-name"), + ), + }, + // Update with same schema_name - should not send schemaName to API (prevents API failure) + { + Config: providerConfig + ` + resource "segment_profiles_warehouse" "test" { + space_id = "my-space-id" + metadata_id = "my-metadata-id" + name = "My updated warehouse name" + enabled = false + settings = jsonencode({ + "token": "my-other-token" + }) + schema_name = "my-schema-name" + } + `, + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("segment_profiles_warehouse.test", "schema_name", "my-schema-name"), + ), + }, + // Update with different schema_name - should send schemaName to API (legitimate change) + { + Config: providerConfig + ` + resource "segment_profiles_warehouse" "test" { + space_id = "my-space-id" + metadata_id = "my-metadata-id" + name = "My final warehouse name" + enabled = true + settings = jsonencode({ + "token": "my-final-token" + }) + schema_name = "my-new-schema-name" + } + `, + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("segment_profiles_warehouse.test", "schema_name", "my-new-schema-name"), + ), + }, + }, + }) +} + +func TestDetermineSchemaNameForUpdate(t *testing.T) { + // Test the determineSchemaNameForUpdate function that prevents API failures + // when the schema name already exists in the warehouse configuration + tests := []struct { + name string + planSchemaName types.String + stateSchemaName types.String + expectedResult *string + description string + }{ + { + name: "both_null", + planSchemaName: types.StringNull(), + stateSchemaName: types.StringNull(), + expectedResult: nil, + description: "Both null - should not send schemaName to API (prevents API failure)", + }, + { + name: "both_unknown", + planSchemaName: types.StringUnknown(), + stateSchemaName: types.StringUnknown(), + expectedResult: nil, + description: "Both unknown - should not send schemaName to API (prevents API failure)", + }, + { + name: "both_same_value", + planSchemaName: types.StringValue("my-schema"), + stateSchemaName: types.StringValue("my-schema"), + expectedResult: nil, + description: "Both have same value - should not send schemaName to API (prevents API failure)", + }, + { + name: "plan_null_state_has_value", + planSchemaName: types.StringNull(), + stateSchemaName: types.StringValue("my-schema"), + expectedResult: nil, // null pointer, not the actual value + description: "Plan null, state has value - should send schemaName to API", + }, + { + name: "plan_has_value_state_null", + planSchemaName: types.StringValue("my-schema"), + stateSchemaName: types.StringNull(), + expectedResult: stringPtr("my-schema"), + description: "Plan has value, state null - should send schemaName to API", + }, + { + name: "plan_unknown_state_has_value", + planSchemaName: types.StringUnknown(), + stateSchemaName: types.StringValue("my-schema"), + expectedResult: nil, // unknown becomes null pointer + description: "Plan unknown, state has value - should send schemaName to API", + }, + { + name: "plan_has_value_state_unknown", + planSchemaName: types.StringValue("my-schema"), + stateSchemaName: types.StringUnknown(), + expectedResult: stringPtr("my-schema"), + description: "Plan has value, state unknown - should send schemaName to API", + }, + { + name: "different_values", + planSchemaName: types.StringValue("new-schema"), + stateSchemaName: types.StringValue("old-schema"), + expectedResult: stringPtr("new-schema"), + description: "Different values - should send schemaName to API", + }, + { + name: "empty_string_vs_null", + planSchemaName: types.StringValue(""), + stateSchemaName: types.StringNull(), + expectedResult: stringPtr(""), + description: "Empty string vs null - should send schemaName to API", + }, + { + name: "null_vs_empty_string", + planSchemaName: types.StringNull(), + stateSchemaName: types.StringValue(""), + expectedResult: nil, + description: "Null vs empty string - should send schemaName to API", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Test the actual function + result := determineSchemaNameForUpdate(tt.planSchemaName, tt.stateSchemaName) + + // Check if the result matches expected + if !compareStringPointers(result, tt.expectedResult) { + t.Errorf("Test case '%s' failed: %s\nExpected: %v, but got: %v", + tt.name, tt.description, tt.expectedResult, result) + } + }) + } +} + +// Helper function to create string pointers for test cases +func stringPtr(s string) *string { + return &s +} + +// Helper function to compare string pointers +func compareStringPointers(a, b *string) bool { + if a == nil && b == nil { + return true + } + if a == nil || b == nil { + return false + } + return *a == *b +} From 8fe8bc112e63a5ad2294f44ac5b9e6be0e46c577 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yohann=20Br=C3=A9doux?= Date: Tue, 30 Sep 2025 22:42:36 +0200 Subject: [PATCH 2/8] lint --- internal/provider/profiles_warehouse_resource.go | 3 ++- internal/provider/profiles_warehouse_resource_test.go | 10 ++++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/internal/provider/profiles_warehouse_resource.go b/internal/provider/profiles_warehouse_resource.go index 92ab582..a63f77e 100644 --- a/internal/provider/profiles_warehouse_resource.go +++ b/internal/provider/profiles_warehouse_resource.go @@ -368,9 +368,10 @@ func findProfileWarehouse(authContext context.Context, client *api.APIClient, id // based on comparing the plan and state values. This prevents API failures when the // schema name already exists in the warehouse configuration. func determineSchemaNameForUpdate(planSchemaName, stateSchemaName types.String) *string { - // Only send schemaName to API if it differs from the remote state + // Only send schemaName to API if it differs from the remote state. if !planSchemaName.Equal(stateSchemaName) { return planSchemaName.ValueStringPointer() } + return nil } diff --git a/internal/provider/profiles_warehouse_resource_test.go b/internal/provider/profiles_warehouse_resource_test.go index 1581f8e..daaff70 100644 --- a/internal/provider/profiles_warehouse_resource_test.go +++ b/internal/provider/profiles_warehouse_resource_test.go @@ -416,7 +416,7 @@ func TestAccProfilesWarehouseResource_SchemaNameHandling(t *testing.T) { func TestDetermineSchemaNameForUpdate(t *testing.T) { // Test the determineSchemaNameForUpdate function that prevents API failures - // when the schema name already exists in the warehouse configuration + // when the schema name already exists in the warehouse configuration. tests := []struct { name string planSchemaName types.String @@ -501,7 +501,7 @@ func TestDetermineSchemaNameForUpdate(t *testing.T) { // Test the actual function result := determineSchemaNameForUpdate(tt.planSchemaName, tt.stateSchemaName) - // Check if the result matches expected + // Check if the result matches expected. if !compareStringPointers(result, tt.expectedResult) { t.Errorf("Test case '%s' failed: %s\nExpected: %v, but got: %v", tt.name, tt.description, tt.expectedResult, result) @@ -510,18 +510,20 @@ func TestDetermineSchemaNameForUpdate(t *testing.T) { } } -// Helper function to create string pointers for test cases +// Helper function to create string pointers for test cases. func stringPtr(s string) *string { return &s } -// Helper function to compare string pointers +// Helper function to compare string pointers. func compareStringPointers(a, b *string) bool { if a == nil && b == nil { return true } + if a == nil || b == nil { return false } + return *a == *b } From b22cee24fc7343e07a4d73d76377bcee5b1d72f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yohann=20Br=C3=A9doux?= Date: Tue, 30 Sep 2025 22:54:49 +0200 Subject: [PATCH 3/8] lint --- .../provider/profiles_warehouse_resource.go | 8 +++--- .../profiles_warehouse_resource_test.go | 28 +++++++++++-------- 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/internal/provider/profiles_warehouse_resource.go b/internal/provider/profiles_warehouse_resource.go index a63f77e..3942d2f 100644 --- a/internal/provider/profiles_warehouse_resource.go +++ b/internal/provider/profiles_warehouse_resource.go @@ -145,10 +145,10 @@ func (r *profilesWarehouseResource) Create(ctx context.Context, req resource.Cre return } - // This is to satisfy terraform requirements that the returned fields must match the input ones because new settings can be generated in the response + // This is to satisfy terraform requirements that the returned fields must match the input ones because new settings can be generated in the response. state.Settings = plan.Settings - // Set state to fully populated data + // Set state to fully populated data. diags = resp.State.Set(ctx, state) resp.Diagnostics.Append(diags...) if resp.Diagnostics.HasError() { @@ -229,7 +229,7 @@ func (r *profilesWarehouseResource) Update(ctx context.Context, req resource.Upd return } - // Only send schemaName to API if it differs from the remote state + // Only send schemaName to API if it differs from the remote state. // This prevents API failures when the schema name already exists in the warehouse. // The Segment API fails if we send a schemaName that matches the current configuration, // even though it should be a no-op. This handles all cases: @@ -269,7 +269,7 @@ func (r *profilesWarehouseResource) Update(ctx context.Context, req resource.Upd return } - // This is to satisfy terraform requirements that the returned fields must match the input ones because new settings can be generated in the response + // This is to satisfy terraform requirements that the returned fields must match the input ones because new settings can be generated in the response. state.Settings = plan.Settings diags = resp.State.Set(ctx, &state) diff --git a/internal/provider/profiles_warehouse_resource_test.go b/internal/provider/profiles_warehouse_resource_test.go index daaff70..f2d476e 100644 --- a/internal/provider/profiles_warehouse_resource_test.go +++ b/internal/provider/profiles_warehouse_resource_test.go @@ -162,7 +162,7 @@ func TestAccProfilesWarehouseResource(t *testing.T) { resource.Test(t, resource.TestCase{ ProtoV6ProviderFactories: testAccProtoV6ProviderFactories, Steps: []resource.TestStep{ - // Create and Read testing + // Create and Read testing. { Config: providerConfig + ` resource "segment_profiles_warehouse" "test" { @@ -186,7 +186,7 @@ func TestAccProfilesWarehouseResource(t *testing.T) { resource.TestCheckResourceAttr("segment_profiles_warehouse.test", "schema_name", "my-schema-name"), ), }, - // ImportState testing + // ImportState testing. { ResourceName: "segment_profiles_warehouse.test", Config: providerConfig + ` @@ -204,7 +204,7 @@ func TestAccProfilesWarehouseResource(t *testing.T) { ImportState: true, ImportStateId: "my-space-id:my-warehouse-id", }, - // Update and Read testing + // Update and Read testing. { Config: providerConfig + ` resource "segment_profiles_warehouse" "test" { @@ -227,14 +227,14 @@ func TestAccProfilesWarehouseResource(t *testing.T) { resource.TestCheckResourceAttr("segment_profiles_warehouse.test", "settings", "{\"token\":\"my-other-token\"}"), resource.TestCheckResourceAttr("segment_profiles_warehouse.test", "schema_name", "my-new-schema-name")), }, - // Delete testing automatically occurs in TestCase + // Delete testing automatically occurs in TestCase. }, }) } func TestAccProfilesWarehouseResource_SchemaNameHandling(t *testing.T) { // Test the schemaName handling that prevents API failures when the schema - // name already exists in the warehouse configuration + // name already exists in the warehouse configuration. t.Parallel() updateCount := 0 @@ -244,7 +244,7 @@ func TestAccProfilesWarehouseResource_SchemaNameHandling(t *testing.T) { payload := "" if req.URL.Path == "/spaces/my-space-id/profiles-warehouses" && req.Method == http.MethodPost { - // Initial create response + // Initial create response. payload = ` { "data": { @@ -275,7 +275,7 @@ func TestAccProfilesWarehouseResource_SchemaNameHandling(t *testing.T) { } ` } else if req.URL.Path == "/spaces/my-space-id/profiles-warehouses/my-warehouse-id" && req.Method == http.MethodPatch { - // Update response - schemaName should only be sent when it changes + // Update response - schemaName should only be sent when it changes. updateCount++ payload = ` { @@ -307,7 +307,7 @@ func TestAccProfilesWarehouseResource_SchemaNameHandling(t *testing.T) { } ` } else if req.URL.Path == "/spaces/my-space-id/profiles-warehouses" && req.Method == http.MethodGet { - // Read response + // Read response. payload = ` { "data": { @@ -356,7 +356,7 @@ func TestAccProfilesWarehouseResource_SchemaNameHandling(t *testing.T) { resource.Test(t, resource.TestCase{ ProtoV6ProviderFactories: testAccProtoV6ProviderFactories, Steps: []resource.TestStep{ - // Create with schema_name + // Create with schema_name. { Config: providerConfig + ` resource "segment_profiles_warehouse" "test" { @@ -374,7 +374,7 @@ func TestAccProfilesWarehouseResource_SchemaNameHandling(t *testing.T) { resource.TestCheckResourceAttr("segment_profiles_warehouse.test", "schema_name", "my-schema-name"), ), }, - // Update with same schema_name - should not send schemaName to API (prevents API failure) + // Update with same schema_name - should not send schemaName to API (prevents API failure). { Config: providerConfig + ` resource "segment_profiles_warehouse" "test" { @@ -392,7 +392,7 @@ func TestAccProfilesWarehouseResource_SchemaNameHandling(t *testing.T) { resource.TestCheckResourceAttr("segment_profiles_warehouse.test", "schema_name", "my-schema-name"), ), }, - // Update with different schema_name - should send schemaName to API (legitimate change) + // Update with different schema_name - should send schemaName to API (legitimate change). { Config: providerConfig + ` resource "segment_profiles_warehouse" "test" { @@ -415,6 +415,8 @@ func TestAccProfilesWarehouseResource_SchemaNameHandling(t *testing.T) { } func TestDetermineSchemaNameForUpdate(t *testing.T) { + t.Parallel() + // Test the determineSchemaNameForUpdate function that prevents API failures // when the schema name already exists in the warehouse configuration. tests := []struct { @@ -498,7 +500,9 @@ func TestDetermineSchemaNameForUpdate(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - // Test the actual function + t.Parallel() + + // Test the actual function. result := determineSchemaNameForUpdate(tt.planSchemaName, tt.stateSchemaName) // Check if the result matches expected. From f8d16c5ca1ca58a22f9a95c29be3805dc5802a10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yohann=20Br=C3=A9doux?= Date: Tue, 30 Sep 2025 22:59:50 +0200 Subject: [PATCH 4/8] fmt --- internal/provider/profiles_warehouse_resource.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/provider/profiles_warehouse_resource.go b/internal/provider/profiles_warehouse_resource.go index 3942d2f..72ee3cd 100644 --- a/internal/provider/profiles_warehouse_resource.go +++ b/internal/provider/profiles_warehouse_resource.go @@ -234,7 +234,7 @@ func (r *profilesWarehouseResource) Update(ctx context.Context, req resource.Upd // The Segment API fails if we send a schemaName that matches the current configuration, // even though it should be a no-op. This handles all cases: // 1. Both null/undefined: Equal() returns true, schemaName stays nil (not sent) - // 2. Both have same value: Equal() returns true, schemaName stays nil (not sent) + // 2. Both have same value: Equal() returns true, schemaName stays nil (not sent) // 3. One null, other has value: Equal() returns false, schemaName gets the plan value (sent) // 4. Both have different values: Equal() returns false, schemaName gets the plan value (sent) schemaName := determineSchemaNameForUpdate(plan.SchemaName, state.SchemaName) From 4c0ba596d6525ebe294ecda1f55f730cd4508512 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yohann=20Br=C3=A9doux?= Date: Tue, 30 Sep 2025 23:02:27 +0200 Subject: [PATCH 5/8] remove documentation from md --- docs/resources/profiles_warehouse.md | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/docs/resources/profiles_warehouse.md b/docs/resources/profiles_warehouse.md index beb6188..22a0735 100644 --- a/docs/resources/profiles_warehouse.md +++ b/docs/resources/profiles_warehouse.md @@ -53,16 +53,6 @@ resource "segment_profiles_warehouse" "example" { } ``` -## Schema Name Handling - -The `schema_name` attribute includes special handling to prevent API failures: - -- **When updating**: The `schema_name` is only sent to the Segment API if it differs from the current remote value -- **When unchanged**: If the local and remote `schema_name` values are the same (including both being null/undefined), the field is omitted from the API request -- **When different**: If either the local or remote value is null/undefined while the other has a value, or if they have different values, the field is included in the API request - -**Important**: The Segment API fails when you send a `schema_name` that already exists in the warehouse configuration, even though it should be a no-operation. This behavior prevents those API failures while still allowing legitimate schema name changes. - ## Schema @@ -85,8 +75,6 @@ The `schema_name` attribute includes special handling to prevent API failures: - `enabled` (Boolean) Enable to allow this Warehouse to receive data. Defaults to true. - `name` (String) An optional human-readable name for this Warehouse. - `schema_name` (String) The custom schema name that Segment uses on the Warehouse side. The space slug value is default otherwise. - - **Note:** When updating a profiles warehouse, the `schema_name` is only sent to the Segment API if it differs from the current remote value. This prevents API failures that occur when sending a schema name that already exists in the warehouse configuration. ### Read-Only From 1f04964e58f25e582cab1c5dd26abc64c217a82d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yohann=20Br=C3=A9doux?= Date: Tue, 30 Sep 2025 23:15:42 +0200 Subject: [PATCH 6/8] fix test --- internal/provider/profiles_warehouse_resource.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/internal/provider/profiles_warehouse_resource.go b/internal/provider/profiles_warehouse_resource.go index 72ee3cd..012c5b4 100644 --- a/internal/provider/profiles_warehouse_resource.go +++ b/internal/provider/profiles_warehouse_resource.go @@ -367,7 +367,19 @@ func findProfileWarehouse(authContext context.Context, client *api.APIClient, id // determineSchemaNameForUpdate determines whether schemaName should be sent to the API // based on comparing the plan and state values. This prevents API failures when the // schema name already exists in the warehouse configuration. +// +// The function returns nil (not sent to API) when: +// - Plan value is unknown (should not send unknown values to API) +// - Plan and state values are equal (no change needed) +// +// The function returns a pointer to the plan value when: +// - Plan and state values are different (legitimate change) func determineSchemaNameForUpdate(planSchemaName, stateSchemaName types.String) *string { + // Don't send schemaName to API if plan value is unknown. + if planSchemaName.IsUnknown() { + return nil + } + // Only send schemaName to API if it differs from the remote state. if !planSchemaName.Equal(stateSchemaName) { return planSchemaName.ValueStringPointer() From c8d9e7c0c66978f2833c86a4d26985d2091598ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yohann=20Br=C3=A9doux?= Date: Tue, 30 Sep 2025 23:20:53 +0200 Subject: [PATCH 7/8] lint --- internal/provider/profiles_warehouse_resource.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/internal/provider/profiles_warehouse_resource.go b/internal/provider/profiles_warehouse_resource.go index 012c5b4..dffe212 100644 --- a/internal/provider/profiles_warehouse_resource.go +++ b/internal/provider/profiles_warehouse_resource.go @@ -231,12 +231,12 @@ func (r *profilesWarehouseResource) Update(ctx context.Context, req resource.Upd // Only send schemaName to API if it differs from the remote state. // This prevents API failures when the schema name already exists in the warehouse. - // The Segment API fails if we send a schemaName that matches the current configuration, + // The Segment API fails if we send a schemaName that matches the current configuration. // even though it should be a no-op. This handles all cases: - // 1. Both null/undefined: Equal() returns true, schemaName stays nil (not sent) - // 2. Both have same value: Equal() returns true, schemaName stays nil (not sent) - // 3. One null, other has value: Equal() returns false, schemaName gets the plan value (sent) - // 4. Both have different values: Equal() returns false, schemaName gets the plan value (sent) + // 1. Both null/undefined: Equal() returns true, schemaName stays nil (not sent). + // 2. Both have same value: Equal() returns true, schemaName stays nil (not sent). + // 3. One null, other has value: Equal() returns false, schemaName gets the plan value (sent). + // 4. Both have different values: Equal() returns false, schemaName gets the plan value (sent). schemaName := determineSchemaNameForUpdate(plan.SchemaName, state.SchemaName) out, body, err := r.client.ProfilesSyncAPI.UpdateProfilesWarehouseForSpaceWarehouse(r.authContext, state.SpaceID.ValueString(), state.ID.ValueString()).UpdateProfilesWarehouseForSpaceWarehouseAlphaInput(api.UpdateProfilesWarehouseForSpaceWarehouseAlphaInput{ @@ -369,11 +369,11 @@ func findProfileWarehouse(authContext context.Context, client *api.APIClient, id // schema name already exists in the warehouse configuration. // // The function returns nil (not sent to API) when: -// - Plan value is unknown (should not send unknown values to API) -// - Plan and state values are equal (no change needed) +// - Plan value is unknown (should not send unknown values to API). +// - Plan and state values are equal (no change needed). // // The function returns a pointer to the plan value when: -// - Plan and state values are different (legitimate change) +// - Plan and state values are different (legitimate change). func determineSchemaNameForUpdate(planSchemaName, stateSchemaName types.String) *string { // Don't send schemaName to API if plan value is unknown. if planSchemaName.IsUnknown() { From 82707800e7b8c4d4d0d9ac016f7a6269a0a8eff3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yohann=20Br=C3=A9doux?= Date: Tue, 30 Sep 2025 23:38:31 +0200 Subject: [PATCH 8/8] fix test --- .../profiles_warehouse_resource_test.go | 181 ++++++++++++++---- 1 file changed, 142 insertions(+), 39 deletions(-) diff --git a/internal/provider/profiles_warehouse_resource_test.go b/internal/provider/profiles_warehouse_resource_test.go index f2d476e..e06f7fe 100644 --- a/internal/provider/profiles_warehouse_resource_test.go +++ b/internal/provider/profiles_warehouse_resource_test.go @@ -277,42 +277,43 @@ func TestAccProfilesWarehouseResource_SchemaNameHandling(t *testing.T) { } else if req.URL.Path == "/spaces/my-space-id/profiles-warehouses/my-warehouse-id" && req.Method == http.MethodPatch { // Update response - schemaName should only be sent when it changes. updateCount++ - payload = ` - { - "data": { - "profilesWarehouse": { - "id": "my-warehouse-id", - "spaceId": "my-space-id", - "workspaceId": "my-workspace-id", - "enabled": false, - "metadata": { - "id": "my-metadata-id", - "slug": "snowflake", - "name": "Snowflake", - "description": "Data warehouse built for the cloud", - "logos": { - "default": "https://cdn.filepicker.io/api/file/JrQWOYvMRRCVvSHp4HL0", - "mark": "https://cdn.filepicker.io/api/file/OBhrGoCRKaSyvAhDX3fw", - "alt": "" + if updateCount == 1 { + // First update: name changes, schema_name stays the same + payload = ` + { + "data": { + "profilesWarehouse": { + "id": "my-warehouse-id", + "spaceId": "my-space-id", + "workspaceId": "my-workspace-id", + "enabled": false, + "metadata": { + "id": "my-metadata-id", + "slug": "snowflake", + "name": "Snowflake", + "description": "Data warehouse built for the cloud", + "logos": { + "default": "https://cdn.filepicker.io/api/file/JrQWOYvMRRCVvSHp4HL0", + "mark": "https://cdn.filepicker.io/api/file/OBhrGoCRKaSyvAhDX3fw", + "alt": "" + }, + "options": [] }, - "options": [] - }, - "settings": { - "name": "My new warehouse name", - "token": "my-other-token" - }, - "schemaName": "my-new-schema-name" + "settings": { + "name": "My updated warehouse name", + "token": "my-other-token" + }, + "schemaName": "my-schema-name" + } } } - } - ` - } else if req.URL.Path == "/spaces/my-space-id/profiles-warehouses" && req.Method == http.MethodGet { - // Read response. - payload = ` - { - "data": { - "profilesWarehouses": [ - { + ` + } else { + // Second update: schema_name changes + payload = ` + { + "data": { + "profilesWarehouse": { "id": "my-warehouse-id", "spaceId": "my-space-id", "workspaceId": "my-workspace-id", @@ -330,15 +331,117 @@ func TestAccProfilesWarehouseResource_SchemaNameHandling(t *testing.T) { "options": [] }, "settings": { - "name": "My warehouse name", - "token": "my-token" + "name": "My final warehouse name", + "token": "my-final-token" }, - "schemaName": "my-schema-name" + "schemaName": "my-new-schema-name" } - ] + } } - } - ` + ` + } + } else if req.URL.Path == "/spaces/my-space-id/profiles-warehouses" && req.Method == http.MethodGet { + // Read response - return current state based on update count. + if updateCount == 0 { + // Initial state + payload = ` + { + "data": { + "profilesWarehouses": [ + { + "id": "my-warehouse-id", + "spaceId": "my-space-id", + "workspaceId": "my-workspace-id", + "enabled": true, + "metadata": { + "id": "my-metadata-id", + "slug": "snowflake", + "name": "Snowflake", + "description": "Data warehouse built for the cloud", + "logos": { + "default": "https://cdn.filepicker.io/api/file/JrQWOYvMRRCVvSHp4HL0", + "mark": "https://cdn.filepicker.io/api/file/OBhrGoCRKaSyvAhDX3fw", + "alt": "" + }, + "options": [] + }, + "settings": { + "name": "My warehouse name", + "token": "my-token" + }, + "schemaName": "my-schema-name" + } + ] + } + } + ` + } else if updateCount == 1 { + // After first update: name changed, schema_name same + payload = ` + { + "data": { + "profilesWarehouses": [ + { + "id": "my-warehouse-id", + "spaceId": "my-space-id", + "workspaceId": "my-workspace-id", + "enabled": false, + "metadata": { + "id": "my-metadata-id", + "slug": "snowflake", + "name": "Snowflake", + "description": "Data warehouse built for the cloud", + "logos": { + "default": "https://cdn.filepicker.io/api/file/JrQWOYvMRRCVvSHp4HL0", + "mark": "https://cdn.filepicker.io/api/file/OBhrGoCRKaSyvAhDX3fw", + "alt": "" + }, + "options": [] + }, + "settings": { + "name": "My updated warehouse name", + "token": "my-other-token" + }, + "schemaName": "my-schema-name" + } + ] + } + } + ` + } else { + // After second update: schema_name changed + payload = ` + { + "data": { + "profilesWarehouses": [ + { + "id": "my-warehouse-id", + "spaceId": "my-space-id", + "workspaceId": "my-workspace-id", + "enabled": true, + "metadata": { + "id": "my-metadata-id", + "slug": "snowflake", + "name": "Snowflake", + "description": "Data warehouse built for the cloud", + "logos": { + "default": "https://cdn.filepicker.io/api/file/JrQWOYvMRRCVvSHp4HL0", + "mark": "https://cdn.filepicker.io/api/file/OBhrGoCRKaSyvAhDX3fw", + "alt": "" + }, + "options": [] + }, + "settings": { + "name": "My final warehouse name", + "token": "my-final-token" + }, + "schemaName": "my-new-schema-name" + } + ] + } + } + ` + } } _, _ = w.Write([]byte(payload))