diff --git a/.nextchanges/bundles/schema-comment-nothing-to-update.md b/.nextchanges/bundles/schema-comment-nothing-to-update.md new file mode 100644 index 0000000000..d7e77b7043 --- /dev/null +++ b/.nextchanges/bundles/schema-comment-nothing-to-update.md @@ -0,0 +1 @@ +Fixed `bundle deploy` failing with `Nothing to update` on the direct engine when a schema's, catalog's or volume's comment was set outside the bundle. diff --git a/acceptance/bundle/resources/catalogs/drift/comment_out_of_band/databricks.yml.tmpl b/acceptance/bundle/resources/catalogs/drift/comment_out_of_band/databricks.yml.tmpl new file mode 100644 index 0000000000..b7fa6d6d95 --- /dev/null +++ b/acceptance/bundle/resources/catalogs/drift/comment_out_of_band/databricks.yml.tmpl @@ -0,0 +1,7 @@ +bundle: + name: test-bundle-$UNIQUE_NAME + +resources: + catalogs: + catalog1: + name: test-catalog-$UNIQUE_NAME diff --git a/acceptance/bundle/resources/catalogs/drift/comment_out_of_band/out.test.toml b/acceptance/bundle/resources/catalogs/drift/comment_out_of_band/out.test.toml new file mode 100644 index 0000000000..8c52d40aa2 --- /dev/null +++ b/acceptance/bundle/resources/catalogs/drift/comment_out_of_band/out.test.toml @@ -0,0 +1,3 @@ +Cloud = true +RequiresUnityCatalog = true +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] diff --git a/acceptance/bundle/resources/catalogs/drift/comment_out_of_band/output.txt b/acceptance/bundle/resources/catalogs/drift/comment_out_of_band/output.txt new file mode 100644 index 0000000000..6ff82f588e --- /dev/null +++ b/acceptance/bundle/resources/catalogs/drift/comment_out_of_band/output.txt @@ -0,0 +1,57 @@ + +=== Deploy a catalog that does not declare a comment +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/files... +Created catalogs.catalog1 +Files: 0 uploaded, 0 deleted +Resources: 1 created, 0 changed, 0 deleted, 0 unchanged + +=== Set the comment out of band, the way Catalog Explorer does +=== The remote comment is drift, so the plan updates the catalog +>>> [CLI] bundle plan --output json +{ + "action": "update", + "remote": "set outside the bundle" +} + +=== Redeploy clears the comment +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/files... +Updated catalogs.catalog1 +Files: 0 uploaded, 0 deleted +Resources: 0 created, 1 changed, 0 deleted, 0 unchanged + +=== The comment is gone +>>> [CLI] api get /api/2.1/unity-catalog/catalogs/test-catalog-[UNIQUE_NAME] +"" + +=== Redeploy is a no-op +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/files... +Files: 0 uploaded, 0 deleted +Resources: 0 created, 0 changed, 0 deleted, 1 unchanged + +>>> [CLI] bundle destroy --auto-approve +The following resources will be deleted: + delete resources.catalogs.catalog1 + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default + +Destroy: 1 deleted + +=== The UpdateCatalog payloads the deploys sent +>>> print_requests.py --method PATCH //unity-catalog/catalogs +{ + "method": "PATCH", + "path": "/api/2.1/unity-catalog/catalogs/test-catalog-[UNIQUE_NAME]", + "body": { + "comment": "set outside the bundle" + } +} +{ + "method": "PATCH", + "path": "/api/2.1/unity-catalog/catalogs/test-catalog-[UNIQUE_NAME]", + "body": { + "comment": "" + } +} diff --git a/acceptance/bundle/resources/catalogs/drift/comment_out_of_band/script b/acceptance/bundle/resources/catalogs/drift/comment_out_of_band/script new file mode 100644 index 0000000000..6f1a938177 --- /dev/null +++ b/acceptance/bundle/resources/catalogs/drift/comment_out_of_band/script @@ -0,0 +1,32 @@ +echo "*" > .gitignore +envsubst < databricks.yml.tmpl > databricks.yml + +CATALOG="test-catalog-${UNIQUE_NAME}" + +# print_requests.py consumes out.requests.txt, so it has to run after the last +# request the test makes, which is the destroy below. +cleanup() { + trace $CLI bundle destroy --auto-approve + + title "The UpdateCatalog payloads the deploys sent" + trace print_requests.py --method PATCH //unity-catalog/catalogs +} +trap cleanup EXIT + +title "Deploy a catalog that does not declare a comment" +trace $CLI bundle deploy + +title "Set the comment out of band, the way Catalog Explorer does" +MSYS_NO_PATHCONV=1 $CLI api patch "/api/2.1/unity-catalog/catalogs/$CATALOG" --json '{"comment":"set outside the bundle"}' > /dev/null + +title "The remote comment is drift, so the plan updates the catalog" +trace $CLI bundle plan --output json | jq '.plan[].changes.comment' + +title "Redeploy clears the comment" +trace $CLI bundle deploy + +title "The comment is gone" +MSYS_NO_PATHCONV=1 trace $CLI api get "/api/2.1/unity-catalog/catalogs/$CATALOG" | jq '.comment' + +title "Redeploy is a no-op" +trace $CLI bundle deploy diff --git a/acceptance/bundle/resources/catalogs/drift/comment_out_of_band/test.toml b/acceptance/bundle/resources/catalogs/drift/comment_out_of_band/test.toml new file mode 100644 index 0000000000..ae0b1a870d --- /dev/null +++ b/acceptance/bundle/resources/catalogs/drift/comment_out_of_band/test.toml @@ -0,0 +1,11 @@ +Cloud = true +RequiresUnityCatalog = true + +# UpdateCatalog payload construction is direct-engine-specific. +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] + +Ignore = [ + ".databricks", + ".gitignore", + "databricks.yml", +] diff --git a/acceptance/bundle/resources/schemas/drift/comment_out_of_band/databricks.yml.tmpl b/acceptance/bundle/resources/schemas/drift/comment_out_of_band/databricks.yml.tmpl new file mode 100644 index 0000000000..50720b6256 --- /dev/null +++ b/acceptance/bundle/resources/schemas/drift/comment_out_of_band/databricks.yml.tmpl @@ -0,0 +1,8 @@ +bundle: + name: test-bundle-$UNIQUE_NAME + +resources: + schemas: + schema1: + catalog_name: main + name: test-schema-$UNIQUE_NAME diff --git a/acceptance/bundle/resources/schemas/drift/comment_out_of_band/out.test.toml b/acceptance/bundle/resources/schemas/drift/comment_out_of_band/out.test.toml new file mode 100644 index 0000000000..8c52d40aa2 --- /dev/null +++ b/acceptance/bundle/resources/schemas/drift/comment_out_of_band/out.test.toml @@ -0,0 +1,3 @@ +Cloud = true +RequiresUnityCatalog = true +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] diff --git a/acceptance/bundle/resources/schemas/drift/comment_out_of_band/output.txt b/acceptance/bundle/resources/schemas/drift/comment_out_of_band/output.txt new file mode 100644 index 0000000000..d45016243e --- /dev/null +++ b/acceptance/bundle/resources/schemas/drift/comment_out_of_band/output.txt @@ -0,0 +1,60 @@ + +=== Deploy a schema that does not declare a comment +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/files... +Created schemas.schema1 +Files: 0 uploaded, 0 deleted +Resources: 1 created, 0 changed, 0 deleted, 0 unchanged + +=== Set the comment out of band, the way Catalog Explorer does +=== The remote comment is drift, so the plan updates the schema +>>> [CLI] bundle plan --output json +{ + "action": "update", + "remote": "set outside the bundle" +} + +=== Redeploy clears the comment +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/files... +Updated schemas.schema1 +Files: 0 uploaded, 0 deleted +Resources: 0 created, 1 changed, 0 deleted, 0 unchanged + +=== The comment is gone +>>> [CLI] api get /api/2.1/unity-catalog/schemas/main.test-schema-[UNIQUE_NAME] +"" + +=== Redeploy is a no-op +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/files... +Files: 0 uploaded, 0 deleted +Resources: 0 created, 0 changed, 0 deleted, 1 unchanged + +>>> [CLI] bundle destroy --auto-approve +The following resources will be deleted: + delete resources.schemas.schema1 + +This action will result in the deletion of the following UC schemas. Any underlying data may be lost: + delete resources.schemas.schema1 + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default + +Destroy: 1 deleted + +=== The UpdateSchema payloads the deploys sent +>>> print_requests.py --method PATCH //unity-catalog/schemas +{ + "method": "PATCH", + "path": "/api/2.1/unity-catalog/schemas/main.test-schema-[UNIQUE_NAME]", + "body": { + "comment": "set outside the bundle" + } +} +{ + "method": "PATCH", + "path": "/api/2.1/unity-catalog/schemas/main.test-schema-[UNIQUE_NAME]", + "body": { + "comment": "" + } +} diff --git a/acceptance/bundle/resources/schemas/drift/comment_out_of_band/script b/acceptance/bundle/resources/schemas/drift/comment_out_of_band/script new file mode 100644 index 0000000000..b5f23ec918 --- /dev/null +++ b/acceptance/bundle/resources/schemas/drift/comment_out_of_band/script @@ -0,0 +1,32 @@ +echo "*" > .gitignore +envsubst < databricks.yml.tmpl > databricks.yml + +SCHEMA="main.test-schema-${UNIQUE_NAME}" + +# print_requests.py consumes out.requests.txt, so it has to run after the last +# request the test makes, which is the destroy below. +cleanup() { + trace $CLI bundle destroy --auto-approve + + title "The UpdateSchema payloads the deploys sent" + trace print_requests.py --method PATCH //unity-catalog/schemas +} +trap cleanup EXIT + +title "Deploy a schema that does not declare a comment" +trace $CLI bundle deploy + +title "Set the comment out of band, the way Catalog Explorer does" +MSYS_NO_PATHCONV=1 $CLI api patch "/api/2.1/unity-catalog/schemas/$SCHEMA" --json '{"comment":"set outside the bundle"}' > /dev/null + +title "The remote comment is drift, so the plan updates the schema" +trace $CLI bundle plan --output json | jq '.plan[].changes.comment' + +title "Redeploy clears the comment" +trace $CLI bundle deploy + +title "The comment is gone" +MSYS_NO_PATHCONV=1 trace $CLI api get "/api/2.1/unity-catalog/schemas/$SCHEMA" | jq '.comment' + +title "Redeploy is a no-op" +trace $CLI bundle deploy diff --git a/acceptance/bundle/resources/schemas/drift/comment_out_of_band/test.toml b/acceptance/bundle/resources/schemas/drift/comment_out_of_band/test.toml new file mode 100644 index 0000000000..4f7503fc8e --- /dev/null +++ b/acceptance/bundle/resources/schemas/drift/comment_out_of_band/test.toml @@ -0,0 +1,11 @@ +Cloud = true +RequiresUnityCatalog = true + +# UpdateSchema payload construction is direct-engine-specific. +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] + +Ignore = [ + ".databricks", + ".gitignore", + "databricks.yml", +] diff --git a/acceptance/bundle/resources/volumes/drift/comment_out_of_band/databricks.yml.tmpl b/acceptance/bundle/resources/volumes/drift/comment_out_of_band/databricks.yml.tmpl new file mode 100644 index 0000000000..084b752f4a --- /dev/null +++ b/acceptance/bundle/resources/volumes/drift/comment_out_of_band/databricks.yml.tmpl @@ -0,0 +1,13 @@ +bundle: + name: test-bundle-$UNIQUE_NAME + +resources: + schemas: + schema1: + catalog_name: main + name: test-schema-$UNIQUE_NAME + volumes: + volume1: + catalog_name: main + schema_name: ${resources.schemas.schema1.name} + name: test-volume-$UNIQUE_NAME diff --git a/acceptance/bundle/resources/volumes/drift/comment_out_of_band/out.test.toml b/acceptance/bundle/resources/volumes/drift/comment_out_of_band/out.test.toml new file mode 100644 index 0000000000..8c52d40aa2 --- /dev/null +++ b/acceptance/bundle/resources/volumes/drift/comment_out_of_band/out.test.toml @@ -0,0 +1,3 @@ +Cloud = true +RequiresUnityCatalog = true +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] diff --git a/acceptance/bundle/resources/volumes/drift/comment_out_of_band/output.txt b/acceptance/bundle/resources/volumes/drift/comment_out_of_band/output.txt new file mode 100644 index 0000000000..c1e1f119fa --- /dev/null +++ b/acceptance/bundle/resources/volumes/drift/comment_out_of_band/output.txt @@ -0,0 +1,68 @@ + +=== Deploy a volume that does not declare a comment +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/files... +Created schemas.schema1 +Created volumes.volume1 +Files: 0 uploaded, 0 deleted +Resources: 2 created, 0 changed, 0 deleted, 0 unchanged + +=== Set the comment out of band, the way Catalog Explorer does +=== The remote comment is drift, so the plan updates the volume +>>> [CLI] bundle plan --output json +{ + "action": "update", + "remote": "set outside the bundle" +} + +=== Redeploy clears the comment +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/files... +Updated volumes.volume1 +Files: 0 uploaded, 0 deleted +Resources: 0 created, 1 changed, 0 deleted, 1 unchanged + +=== The comment is gone +>>> [CLI] api get /api/2.1/unity-catalog/volumes/main.test-schema-[UNIQUE_NAME].test-volume-[UNIQUE_NAME] +"" + +=== Redeploy is a no-op +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/files... +Files: 0 uploaded, 0 deleted +Resources: 0 created, 0 changed, 0 deleted, 2 unchanged + +>>> [CLI] bundle destroy --auto-approve +The following resources will be deleted: + delete resources.schemas.schema1 + delete resources.volumes.volume1 + +This action will result in the deletion of the following UC schemas. Any underlying data may be lost: + delete resources.schemas.schema1 + +This action will result in the deletion of the following volumes. +For managed volumes, the files stored in the volume are also deleted from your +cloud tenant within 30 days. For external volumes, the metadata about the volume +is removed from the catalog, but the underlying files are not deleted: + delete resources.volumes.volume1 + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default + +Destroy: 2 deleted + +=== The UpdateVolume payloads the deploys sent +>>> print_requests.py --method PATCH //unity-catalog/volumes +{ + "method": "PATCH", + "path": "/api/2.1/unity-catalog/volumes/main.test-schema-[UNIQUE_NAME].test-volume-[UNIQUE_NAME]", + "body": { + "comment": "set outside the bundle" + } +} +{ + "method": "PATCH", + "path": "/api/2.1/unity-catalog/volumes/main.test-schema-[UNIQUE_NAME].test-volume-[UNIQUE_NAME]", + "body": { + "comment": "" + } +} diff --git a/acceptance/bundle/resources/volumes/drift/comment_out_of_band/script b/acceptance/bundle/resources/volumes/drift/comment_out_of_band/script new file mode 100644 index 0000000000..c176403ba0 --- /dev/null +++ b/acceptance/bundle/resources/volumes/drift/comment_out_of_band/script @@ -0,0 +1,32 @@ +echo "*" > .gitignore +envsubst < databricks.yml.tmpl > databricks.yml + +VOLUME="main.test-schema-${UNIQUE_NAME}.test-volume-${UNIQUE_NAME}" + +# print_requests.py consumes out.requests.txt, so it has to run after the last +# request the test makes, which is the destroy below. +cleanup() { + trace $CLI bundle destroy --auto-approve + + title "The UpdateVolume payloads the deploys sent" + trace print_requests.py --method PATCH //unity-catalog/volumes +} +trap cleanup EXIT + +title "Deploy a volume that does not declare a comment" +trace $CLI bundle deploy + +title "Set the comment out of band, the way Catalog Explorer does" +MSYS_NO_PATHCONV=1 $CLI api patch "/api/2.1/unity-catalog/volumes/$VOLUME" --json '{"comment":"set outside the bundle"}' > /dev/null + +title "The remote comment is drift, so the plan updates the volume" +trace $CLI bundle plan --output json | jq '.plan["resources.volumes.volume1"].changes.comment' + +title "Redeploy clears the comment" +trace $CLI bundle deploy + +title "The comment is gone" +MSYS_NO_PATHCONV=1 trace $CLI api get "/api/2.1/unity-catalog/volumes/$VOLUME" | jq '.comment' + +title "Redeploy is a no-op" +trace $CLI bundle deploy diff --git a/acceptance/bundle/resources/volumes/drift/comment_out_of_band/test.toml b/acceptance/bundle/resources/volumes/drift/comment_out_of_band/test.toml new file mode 100644 index 0000000000..bec056028b --- /dev/null +++ b/acceptance/bundle/resources/volumes/drift/comment_out_of_band/test.toml @@ -0,0 +1,11 @@ +Cloud = true +RequiresUnityCatalog = true + +# UpdateVolume payload construction is direct-engine-specific. +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] + +Ignore = [ + ".databricks", + ".gitignore", + "databricks.yml", +] diff --git a/bundle/direct/dresources/catalog.go b/bundle/direct/dresources/catalog.go index 604100e92d..54afbb1fd3 100644 --- a/bundle/direct/dresources/catalog.go +++ b/bundle/direct/dresources/catalog.go @@ -62,7 +62,7 @@ func (r *ResourceCatalog) DoUpdate(ctx context.Context, id string, config *catal Options: config.Options, Owner: "", // Not supported by DABs Properties: config.Properties, - ForceSendFields: utils.FilterFields[catalog.UpdateCatalog](config.ForceSendFields, "EnablePredictiveOptimization", "IsolationMode", "Owner"), + ForceSendFields: forceSendComment(utils.FilterFields[catalog.UpdateCatalog](config.ForceSendFields, "EnablePredictiveOptimization", "IsolationMode", "Owner")), } response, err := r.client.Catalogs.Update(ctx, updateRequest) @@ -86,7 +86,7 @@ func (r *ResourceCatalog) DoUpdateWithID(ctx context.Context, id string, config Options: config.Options, Owner: "", // Not supported by DABs Properties: config.Properties, - ForceSendFields: utils.FilterFields[catalog.UpdateCatalog](config.ForceSendFields, "EnablePredictiveOptimization", "IsolationMode", "Owner"), + ForceSendFields: forceSendComment(utils.FilterFields[catalog.UpdateCatalog](config.ForceSendFields, "EnablePredictiveOptimization", "IsolationMode", "Owner")), } if config.Name != id { diff --git a/bundle/direct/dresources/schema.go b/bundle/direct/dresources/schema.go index 24c82ceb6a..ea406e2684 100644 --- a/bundle/direct/dresources/schema.go +++ b/bundle/direct/dresources/schema.go @@ -56,7 +56,7 @@ func (r *ResourceSchema) DoUpdate(ctx context.Context, id string, config *catalo NewName: "", // We recreate schemas on name change intentionally. Owner: "", // Not supported by DABs Properties: config.Properties, - ForceSendFields: utils.FilterFields[catalog.UpdateSchema](config.ForceSendFields, "EnablePredictiveOptimization", "NewName", "Owner"), + ForceSendFields: forceSendComment(utils.FilterFields[catalog.UpdateSchema](config.ForceSendFields, "EnablePredictiveOptimization", "NewName", "Owner")), } response, err := r.client.Schemas.Update(ctx, updateRequest) diff --git a/bundle/direct/dresources/util.go b/bundle/direct/dresources/util.go index fc68f3ea37..51db2a5dcd 100644 --- a/bundle/direct/dresources/util.go +++ b/bundle/direct/dresources/util.go @@ -65,3 +65,14 @@ func collectLeafUpdatePathsWithPrefix(changes Changes, prefix string) []string { slices.Sort(paths) return paths } + +// forceSendComment adds Comment to a ForceSendFields list, so a comment the config no +// longer sets serializes as "" instead of being dropped by omitempty. +// +// The UC update APIs answer a PATCH that carries no field with "Nothing to update" (400) +// rather than treating it as a no-op. A resource whose only updatable field is its comment +// would otherwise become undeployable the moment someone sets a description on it outside +// the bundle. Sending the empty value also makes clearing such a comment actually happen. +func forceSendComment(forceSendFields []string) []string { + return append(forceSendFields, "Comment") +} diff --git a/bundle/direct/dresources/volume.go b/bundle/direct/dresources/volume.go index 6c96e66ecc..ae56bb4f21 100644 --- a/bundle/direct/dresources/volume.go +++ b/bundle/direct/dresources/volume.go @@ -55,7 +55,7 @@ func (r *ResourceVolume) DoUpdate(ctx context.Context, id string, config *catalo NewName: "", // Not supported by Update(). Needs DoUpdateWithID() Owner: "", // Not supported by DABs - ForceSendFields: utils.FilterFields[catalog.UpdateVolumeRequestContent](config.ForceSendFields, "NewName", "Owner"), + ForceSendFields: forceSendComment(utils.FilterFields[catalog.UpdateVolumeRequestContent](config.ForceSendFields, "NewName", "Owner")), } nameFromID, err := getNameFromID(id) @@ -87,7 +87,7 @@ func (r *ResourceVolume) DoUpdateWithID(ctx context.Context, id string, config * NewName: "", // Initialized below if needed Owner: "", // Not supported by DABs - ForceSendFields: utils.FilterFields[catalog.UpdateVolumeRequestContent](config.ForceSendFields, "Owner"), + ForceSendFields: forceSendComment(utils.FilterFields[catalog.UpdateVolumeRequestContent](config.ForceSendFields, "Owner")), } items := strings.Split(id, ".") diff --git a/libs/testserver/catalogs.go b/libs/testserver/catalogs.go index 1d0bc06568..4c1c864b1a 100644 --- a/libs/testserver/catalogs.go +++ b/libs/testserver/catalogs.go @@ -88,6 +88,11 @@ func (s *FakeWorkspace) CatalogsUpdate(req Request, name string) Response { } } + fields, errResponse := parseUCUpdate(req.Body, "UpdateCatalog") + if errResponse != nil { + return *errResponse + } + var updateRequest catalog.UpdateCatalog if err := json.Unmarshal(req.Body, &updateRequest); err != nil { return Response{ @@ -96,9 +101,13 @@ func (s *FakeWorkspace) CatalogsUpdate(req Request, name string) Response { } } - // Update only the fields that can be updated - if updateRequest.Comment != "" { + // Update only the fields that can be updated. Comment is keyed off presence rather + // than a non-empty value so an explicit empty comment clears it, the way UC does. + if _, ok := fields["comment"]; ok { existing.Comment = updateRequest.Comment + // UC echoes a comment once it has been set, including when it was set to "", + // so keep it in the response rather than letting omitempty drop it. + existing.ForceSendFields = append(existing.ForceSendFields, "Comment") } if updateRequest.CustomMaxRetentionHours != 0 { existing.CustomMaxRetentionHours = updateRequest.CustomMaxRetentionHours diff --git a/libs/testserver/fake_workspace.go b/libs/testserver/fake_workspace.go index 1caec84d1e..d5af448c31 100644 --- a/libs/testserver/fake_workspace.go +++ b/libs/testserver/fake_workspace.go @@ -5,6 +5,7 @@ import ( "encoding/binary" "encoding/json" "fmt" + "net/http" "os" "path" "path/filepath" @@ -254,6 +255,37 @@ func (s *FakeWorkspace) LockUnlock() func() { return func() { s.mu.Unlock() } } +// parseUCUpdate decodes a Unity Catalog update payload into its raw fields. It returns a +// rejection response when the body carries no field to act on: UC answers such a PATCH with +// " Nothing to update." (400) rather than treating it as a no-op. A key set to +// null does not count. Verified against a real workspace for schemas, volumes and catalogs: +// {} and {"comment": null} are rejected, while {"comment": ""} and +// {"custom_max_retention_hours": 0} are accepted. +func parseUCUpdate(body []byte, operation string) (map[string]json.RawMessage, *Response) { + var fields map[string]json.RawMessage + + if err := json.Unmarshal(body, &fields); err != nil { + return nil, &Response{ + Body: fmt.Sprintf("internal error: %s", err), + StatusCode: http.StatusInternalServerError, + } + } + + for _, value := range fields { + if string(value) != "null" { + return fields, nil + } + } + + return nil, &Response{ + StatusCode: http.StatusBadRequest, + Body: map[string]string{ + "error_code": "INVALID_PARAMETER_VALUE", + "message": operation + " Nothing to update.", + }, + } +} + // Generic functions to handle map operations func MapGet[T any](w *FakeWorkspace, collection map[string]T, key string) Response { defer w.LockUnlock()() diff --git a/libs/testserver/schemas.go b/libs/testserver/schemas.go index 66d23a3011..72fd456427 100644 --- a/libs/testserver/schemas.go +++ b/libs/testserver/schemas.go @@ -80,6 +80,11 @@ func (s *FakeWorkspace) SchemasUpdate(req Request, name string) Response { } } + fields, errResponse := parseUCUpdate(req.Body, "UpdateSchema") + if errResponse != nil { + return *errResponse + } + var schemaUpdate catalog.SchemaInfo if err := json.Unmarshal(req.Body, &schemaUpdate); err != nil { @@ -97,6 +102,13 @@ func (s *FakeWorkspace) SchemasUpdate(req Request, name string) Response { } } + // mergo skips zero values, so an explicit empty comment would leave the stored one in + // place. UC clears it instead. Comment is the only field UpdateSchema can clear at + // all: clearing properties needs an empty or null map, which the check above rejects. + if _, ok := fields["comment"]; ok { + existing.Comment = schemaUpdate.Comment + } + existing.UpdatedAt = nowMilli() existing.UpdatedBy = s.CurrentUser().UserName diff --git a/libs/testserver/volumes.go b/libs/testserver/volumes.go index 88eae7ac02..5a6923b0e8 100644 --- a/libs/testserver/volumes.go +++ b/libs/testserver/volumes.go @@ -76,6 +76,11 @@ func (s *FakeWorkspace) VolumesUpdate(req Request, fullname string) Response { } } + fields, errResponse := parseUCUpdate(req.Body, "UpdateVolume") + if errResponse != nil { + return *errResponse + } + var request catalog.UpdateVolumeRequestContent if err := json.Unmarshal(req.Body, &request); err != nil { @@ -85,8 +90,13 @@ func (s *FakeWorkspace) VolumesUpdate(req Request, fullname string) Response { } } - if request.Comment != "" { + // Keyed off presence rather than a non-empty value so an explicit empty comment + // clears the stored one, the way UC does. + if _, ok := fields["comment"]; ok { existing.Comment = request.Comment + // UC echoes a comment once it has been set, including when it was set to "", + // so keep it in the response rather than letting omitempty drop it. + existing.ForceSendFields = append(existing.ForceSendFields, "Comment") } if request.Owner != "" {