fix: remove ineffective client-side dedup logic from identity schema resource (#155)

* fix: remove ineffective client-side dedup logic from identity schema resource

Remove the findExistingSchemaByContent()-based client-side deduplication
from the identity schema Create path. This logic was intended to prevent
duplicate schemas when re-applying after a destroy, but it only checked
Layer A (project config) while duplicates occur in Layer B (identity_schemas
table in the backend).

Changes:
- Remove client-side dedup logic that skipped PatchProject when content
  already existed (this was ineffective at preventing backend duplicates)
- Simplify post-patch ID resolution to use resolveSchemaID() which does
  content-based matching solely for finding the canonical hash ID
- Add content-based fallback to Read for cases where the API has
  transformed both the schema ID and URL
- Remove TestAccIdentitySchemaResource_deduplication test and
  dedup.tf.tmpl template
- Remove unused errors import

Closes #152

* fix: address review feedback on error handling and test coverage

- resolveSchemaID: track marshal failures and return a descriptive error
  when no content match is found and schemas couldn't be compared, instead
  of silently skipping
- WaitForCondition: restore proper error handling — abort on context
  cancellation, surface non-transient API errors as warnings, and filter
  out user-provided schemaID from ID-diff detection to only accept
  canonical hash IDs
- Add TestAccIdentitySchemaResource_hashIDResolution test that verifies
  Create resolves the canonical hash ID and Read survives the API
  transformation of both ID and URL

Author: KT-Doan

internal/resources/identityschema/resource.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"encoding/base64"
 	"encoding/json"
-	"errors"
 	"fmt"
 	"time"
 
@@ -219,66 +218,48 @@ func (r *IdentitySchemaResource) findSchemaByURL(schemas []map[string]interface{
 	return -1
 }
 
-// findExistingSchemaByContent checks all project schemas for one with
-// identical JSON content. Returns the schema's canonical (hash-based) ID if
-// found, or "". Used to detect and reuse existing schemas, preventing
-// duplicates when re-applying after a destroy.
-//
-// It uses ListIdentitySchemas (Kratos API) when a project client is available
-// because the Kratos API returns actual schema content with stable hash IDs.
-// The console API path (ListIdentitySchemasViaProject) reads the project config,
-// which may store non-base64 URLs after API transformation — making content
-// comparison impossible for those schemas.
-// Falls back to ListIdentitySchemasViaProject when only a console client is
-// available (workspace-only configuration).
-func (r *IdentitySchemaResource) findExistingSchemaByContent(ctx context.Context, projectID, schemaJSON string) (string, error) {
+// resolveSchemaID resolves the canonical (hash-based) schema ID after a
+// PatchProject call. The Ory API may transform the user-provided schema_id
+// into a content-hash-based ID. This function uses the Kratos API
+// (ListIdentitySchemas) to match by content and find the canonical ID.
+// Falls back to ListIdentitySchemasViaProject when only a console client
+// is available.
+func (r *IdentitySchemaResource) resolveSchemaID(ctx context.Context, projectID, schemaJSON string) (string, error) {
 	var target interface{}
 	if err := json.Unmarshal([]byte(schemaJSON), &target); err != nil {
-		return "", fmt.Errorf("failed to parse schema JSON for content-based lookup: %w", err)
+		return "", fmt.Errorf("failed to parse schema JSON: %w", err)
 	}
 	targetNormalized, err := json.Marshal(target)
 	if err != nil {
-		return "", fmt.Errorf("failed to normalize schema JSON for content-based lookup: %w", err)
+		return "", fmt.Errorf("failed to normalize schema JSON: %w", err)
 	}
 
 	var schemas []ory.IdentitySchemaContainer
-	for attempt := 0; attempt < helpers.ReadRetryMaxAttempts; attempt++ {
-		// Prefer the Kratos API (ListIdentitySchemas) because it returns actual
-		// schema content with canonical hash-based IDs. The console API path
-		// (ListIdentitySchemasViaProject) reads the project config, which may
-		// store non-base64 URLs after the API transforms them — making content
-		// comparison impossible.
-		if r.client.HasProjectClient() {
-			schemas, err = r.client.ListIdentitySchemas(ctx)
-		} else if r.client.HasConsoleClient() {
-			schemas, err = r.client.ListIdentitySchemasViaProject(ctx, projectID)
-		} else {
-			return "", fmt.Errorf("no API client configured for listing identity schemas: set project_api_key or workspace_api_key")
-		}
-		if err == nil {
-			break
-		}
-		if attempt < helpers.ReadRetryMaxAttempts-1 {
-			select {
-			case <-ctx.Done():
-				return "", ctx.Err()
-			case <-time.After(helpers.EventualConsistencyDelay):
-			}
-		}
+	if r.client.HasProjectClient() {
+		schemas, err = r.client.ListIdentitySchemas(ctx)
+	} else if r.client.HasConsoleClient() {
+		schemas, err = r.client.ListIdentitySchemasViaProject(ctx, projectID)
+	} else {
+		return "", fmt.Errorf("no API client configured for listing identity schemas: set project_api_key or workspace_api_key")
 	}
 	if err != nil {
-		return "", fmt.Errorf("failed to list identity schemas for content-based lookup: %w", err)
+		return "", fmt.Errorf("failed to list identity schemas: %w", err)
 	}
 
+	var marshalFailures int
 	for _, s := range schemas {
-		storedNormalized, err := json.Marshal(s.GetSchema())
-		if err != nil {
-			return "", fmt.Errorf("failed to normalize stored schema %s for content-based lookup: %w", s.GetId(), err)
+		storedNormalized, marshalErr := json.Marshal(s.GetSchema())
+		if marshalErr != nil {
+			marshalFailures++
+			continue
 		}
 		if string(targetNormalized) == string(storedNormalized) {
 			return s.GetId(), nil
 		}
 	}
+	if marshalFailures > 0 {
+		return "", fmt.Errorf("no content match found; %d of %d schemas could not be compared due to marshal errors", marshalFailures, len(schemas))
+	}
 	return "", nil
 }
 
@@ -303,14 +284,13 @@ func (r *IdentitySchemaResource) Create(ctx context.Context, req resource.Create
 		return
 	}
 
-	// Get existing schemas and their IDs before the patch
+	// Get existing schemas before the patch
 	existingSchemas, err := r.getSchemas(ctx, projectID)
 	if err != nil {
 		resp.Diagnostics.AddError("Error Getting Schemas", err.Error())
 		return
 	}
 
-	// Record pre-patch IDs for the console-only fallback in WaitForCondition.
 	existingIDs := make(map[string]bool, len(existingSchemas))
 	for _, s := range existingSchemas {
 		if id, ok := s["id"].(string); ok {
@@ -322,68 +302,6 @@ func (r *IdentitySchemaResource) Create(ctx context.Context, req resource.Create
 
 	existingIndex := r.findSchemaIndex(existingSchemas, schemaID)
 
-	// Only perform content-based deduplication when the schema ID does not
-	// already exist in the project configuration. This avoids an extra
-	// ListIdentitySchemas call when we're simply replacing an existing schema.
-	if existingIndex < 0 {
-		existingID, err := r.findExistingSchemaByContent(ctx, projectID, schemaJSON)
-		if err != nil {
-			resp.Diagnostics.AddError("Error Checking for Duplicate Schemas", err.Error())
-			return
-		}
-		if existingID != "" {
-			// Schema with identical content already exists — adopt it.
-			// We intentionally do NOT add a new schema entry to the project
-			// config here because that would create a duplicate (same content,
-			// different schema_id). The existing schema is already registered
-			// in the project config under its original ID, and Read will find
-			// it by the hash-based ID stored in state.
-			if plan.SetDefault.ValueBool() {
-				var defaultPatches []ory.JsonPatch
-
-				// Ensure the schema is in the project's schema list before
-				// setting it as default. For a new project, workspace-level
-				// schemas exist but aren't in the project config yet.
-				if r.findSchemaIndex(existingSchemas, existingID) < 0 {
-					if len(existingSchemas) == 0 {
-						defaultPatches = append(defaultPatches, ory.JsonPatch{
-							Op:   "add",
-							Path: "/services/identity/config/identity/schemas",
-							Value: []map[string]string{{
-								"id":  existingID,
-								"url": schemaURL,
-							}},
-						})
-					} else {
-						defaultPatches = append(defaultPatches, ory.JsonPatch{
-							Op:   "add",
-							Path: "/services/identity/config/identity/schemas/-",
-							Value: map[string]string{
-								"id":  existingID,
-								"url": schemaURL,
-							},
-						})
-					}
-				}
-
-				defaultPatches = append(defaultPatches, ory.JsonPatch{
-					Op:    "add",
-					Path:  "/services/identity/config/identity/default_schema_id",
-					Value: existingID,
-				})
-				_, patchErr := r.client.PatchProject(ctx, projectID, defaultPatches)
-				if patchErr != nil {
-					resp.Diagnostics.AddError("Error Setting Default Schema", patchErr.Error())
-					return
-				}
-			}
-
-			plan.ID = types.StringValue(existingID)
-			plan.ProjectID = types.StringValue(projectID)
-			resp.Diagnostics.Append(resp.State.Set(ctx, plan)...)
-			return
-		}
-	}
 	if existingIndex >= 0 {
 		// Replace existing
 		patches = append(patches, ory.JsonPatch{
@@ -436,40 +354,27 @@ func (r *IdentitySchemaResource) Create(ctx context.Context, req resource.Create
 		return
 	}
 
-	// Resolve the canonical (hash-based) schema ID by polling after the patch.
-	// The Ory API initially stores the schema with the user-provided schema_id,
-	// then asynchronously transforms it to a hash-based ID.
-	//
-	// Two strategies are used depending on the configured client:
-	//
-	//  1. Project client available: content-based matching via ListIdentitySchemas
-	//     (Kratos API). Deterministic and safe for concurrent Terraform applies.
-	//     Non-retryable errors (e.g., auth failures) abort immediately.
-	//
-	//  2. Console client only: the project config stores HTTPS URLs after
-	//     transformation, making schema body decoding impossible. Fall back to
-	//     ID-diff matching (any new schema ID not seen before the patch).
+	// Resolve the canonical (hash-based) schema ID after the patch.
+	// The Ory API transforms the user-provided schema_id into a content-hash ID.
+	// We use multiple strategies to find it:
+	//  1. Content-based matching via ListIdentitySchemas (most reliable)
+	//  2. ID-diff detection from the project config (new hash ID not seen before)
 	var actualID string
 	waitErr := helpers.WaitForCondition(ctx, func() (bool, error) {
-		if r.client.HasProjectClient() {
-			foundID, findErr := r.findExistingSchemaByContent(ctx, projectID, schemaJSON)
-			if findErr != nil {
-				if client.IsTransientError(findErr) {
-					return false, nil // transient (5xx/429) — keep retrying
-				}
-				return false, findErr // non-retryable (auth/permission) — fail fast
+		// Strategy 1: content-based matching via Kratos API
+		if r.client.HasProjectClient() || r.client.HasConsoleClient() {
+			foundID, findErr := r.resolveSchemaID(ctx, projectID, schemaJSON)
+			if findErr != nil && !client.IsTransientError(findErr) {
+				return false, findErr // non-retryable — fail fast
 			}
-			// Only accept once the API has assigned a different (canonical hash) ID.
-			if foundID != "" && foundID != schemaID {
+			if foundID != "" {
 				actualID = foundID
 				return true, nil
 			}
-			return false, nil
 		}
 
-		// Console-only fallback: content matching is unreliable because the project
-		// config stores HTTPS URLs after transformation. Use ID-diff detection instead:
-		// any schema ID that wasn't present before the patch is the hash-based ID.
+		// Strategy 2: check project config for a new hash ID not seen before the patch.
+		// Exclude the user-provided schemaID to ensure we only accept a canonical hash.
 		freshSchemas, gsErr := r.getSchemas(ctx, projectID)
 		if gsErr != nil {
 			return false, nil //nolint:nilerr
@@ -480,21 +385,19 @@ func (r *IdentitySchemaResource) Create(ctx context.Context, req resource.Create
 				return true, nil
 			}
 		}
+
 		return false, nil
 	})
-	if errors.Is(waitErr, context.Canceled) || errors.Is(waitErr, context.DeadlineExceeded) {
-		resp.Diagnostics.AddError("Error Creating Identity Schema",
-			fmt.Sprintf("context canceled while waiting for schema ID transformation: %v", waitErr))
-		return
-	}
 	if waitErr != nil {
-		// Non-retryable API error (e.g., auth/permission failure). The schema was
-		// already created by PatchProject; surface the error so the operator can
-		// investigate. Fall back to the user-provided ID so Terraform can write
-		// state — the canonical ID will be corrected on the next refresh/plan.
+		if ctx.Err() != nil {
+			resp.Diagnostics.AddError("Error Creating Identity Schema",
+				fmt.Sprintf("context canceled while waiting for schema ID transformation: %v", waitErr))
+			return
+		}
+		// Non-retryable API error. The schema was already created by PatchProject;
+		// fall back to user-provided ID so Terraform can write state.
 		resp.Diagnostics.AddWarning("Schema ID Not Resolved",
-			fmt.Sprintf("Could not resolve canonical schema ID due to a non-retryable error; "+
-				"using user-provided ID %q as fallback. "+
+			fmt.Sprintf("Could not resolve canonical schema ID; using %q as fallback. "+
 				"The state will be corrected on the next plan/refresh. Reason: %v",
 				schemaID, waitErr))
 	}
@@ -539,14 +442,7 @@ func (r *IdentitySchemaResource) Read(ctx context.Context, req resource.ReadRequ
 	var schemas []map[string]interface{}
 	var index = -1
 
-	// Note: We intentionally do NOT use the PatchProject cache here.
-	// The cache holds the PatchProject response, which may contain the
-	// user-provided schema_id before the API transforms it to a hash-based ID.
-	// Using stale cache data causes Read to store the wrong (user-provided) ID,
-	// which then fails to match on subsequent refresh cycles once the API has
-	// applied its hash transformation.
-	//
-	// Always call GetProject fresh to get the canonical (post-transformation) IDs.
+	// Try fresh GetProject with retries for eventual consistency.
 	{
 		var err error
 		for attempt := 0; attempt < helpers.ReadRetryMaxAttempts; attempt++ {
@@ -584,6 +480,19 @@ func (r *IdentitySchemaResource) Read(ctx context.Context, req resource.ReadRequ
 		}
 	}
 
+	// If not found by ID or URL in the project config, try content-based matching
+	// via ListIdentitySchemas as a last resort. The project config transforms URLs
+	// from base64:// to https://, making URL matching impossible after transformation.
+	if index < 0 && !state.Schema.IsNull() && !state.Schema.IsUnknown() {
+		if resolvedID, err := r.resolveSchemaID(ctx, projectID, state.Schema.ValueString()); err == nil && resolvedID != "" {
+			// Found by content — verify it exists in the project config
+			index = r.findSchemaIndex(schemas, resolvedID)
+			if index >= 0 {
+				state.ID = types.StringValue(resolvedID)
+			}
+		}
+	}
+
 	if index < 0 {
 		// List available schemas to help with debugging import issues
 		var availableIDs []string

internal/resources/identityschema/resource_test.go

@@ -62,15 +62,13 @@ func TestAccIdentitySchemaResource_basic(t *testing.T) {
 	})
 }
 
-func TestAccIdentitySchemaResource_deduplication(t *testing.T) {
+// TestAccIdentitySchemaResource_hashIDResolution verifies that Create resolves the
+// canonical hash-based ID (not the user-provided schema_id) and that Read can find
+// the schema after the API transforms both the ID and URL. This exercises the
+// content-based fallback in Read that was added to handle the API transformation.
+func TestAccIdentitySchemaResource_hashIDResolution(t *testing.T) {
 	suffix := time.Now().UnixNano()
-	// ContentID stays the same across steps so the schema JSON is identical.
-	// SchemaID changes to prove dedup is based on content, not schema_id.
-	contentID := fmt.Sprintf("tf-test-dedup-%d", suffix)
-	schemaID1 := fmt.Sprintf("tf-test-dedup1-%d", suffix)
-	schemaID2 := fmt.Sprintf("tf-test-dedup2-%d", suffix)
-
-	var firstID string
+	schemaID := fmt.Sprintf("tf-test-hash-%d", suffix)
 
 	resource.Test(t, resource.TestCase{
 		PreCheck: func() {
@@ -79,30 +77,17 @@ func TestAccIdentitySchemaResource_deduplication(t *testing.T) {
 		},
 		ProtoV6ProviderFactories: acctest.TestAccProtoV6ProviderFactories(),
 		Steps: []resource.TestStep{
-			// Step 1: Create a schema and capture its API-assigned ID
-			{
-				Config: acctest.LoadTestConfig(t, "testdata/dedup.tf.tmpl", map[string]string{"SchemaID": schemaID1, "ContentID": contentID, "AppURL": testutil.ExampleAppURL}),
-				Check: resource.ComposeAggregateTestCheckFunc(
-					resource.TestCheckResourceAttrSet("ory_identity_schema.dedup", "id"),
-					resource.TestCheckResourceAttrWith("ory_identity_schema.dedup", "id", func(value string) error {
-						firstID = value
-						return nil
-					}),
-				),
-			},
-			// Step 2: Destroy the resource (removes from state, schema stays in Ory)
-			{
-				Config:  " ", // empty config triggers destroy of previous resources
-				Destroy: false,
-			},
-			// Step 3: Re-create with a DIFFERENT schema_id but SAME content — should reuse the same schema
 			{
-				Config: acctest.LoadTestConfig(t, "testdata/dedup.tf.tmpl", map[string]string{"SchemaID": schemaID2, "ContentID": contentID, "AppURL": testutil.ExampleAppURL}),
+				Config: acctest.LoadTestConfig(t, "testdata/basic.tf.tmpl", map[string]string{"SchemaID": schemaID, "AppURL": testutil.ExampleAppURL}),
 				Check: resource.ComposeAggregateTestCheckFunc(
-					resource.TestCheckResourceAttrSet("ory_identity_schema.dedup", "id"),
-					resource.TestCheckResourceAttrWith("ory_identity_schema.dedup", "id", func(value string) error {
-						if value != firstID {
-							return fmt.Errorf("expected deduplication to reuse ID %q, but got new ID %q", firstID, value)
+					resource.TestCheckResourceAttrSet("ory_identity_schema.test", "id"),
+					resource.TestCheckResourceAttr("ory_identity_schema.test", "schema_id", schemaID),
+					// Verify the stored ID is the canonical hash, not the user-provided schema_id.
+					// This proves Create resolved the hash and Read can find the schema after
+					// the API transforms the ID and URL.
+					resource.TestCheckResourceAttrWith("ory_identity_schema.test", "id", func(value string) error {
+						if value == schemaID {
+							return fmt.Errorf("expected canonical hash ID, but got user-provided schema_id %q", value)
 						}
 						return nil
 					}),