Skip to content

Commit 21090c8

Browse files
rauchyrauchy
authored
[Fix] Allow Updating Share Objects With shared_as Defined (#5287)
This PR fixes a bug where UPDATE operations on share objects would send an empty string for the `shared_as` field, causing API validation errors. The fix allows users to update share object properties (e.g., comments, history settings) without encountering `INVALID_PARAMETER_VALUE` errors. ## Background In [PR #2307](#2307), the provider was modified to suppress sending `shared_as` for UPDATE calls by setting it to an empty string. This was done to address [issue #2240](#2240). However, the Databricks API has since been updated to support `shared_as` in UPDATE operations. The empty string suppression now causes API validation errors when users attempt to update any property of a share object that has `shared_as` defined. ## Problem When updating any field on a share object (e.g., comment, `history_data_sharing_status`), the provider would: 1. Detect the change and generate an UPDATE action 2. Set `shared_as = ""` (empty string) for the UPDATE 3. Send the UPDATE request to the API 4. API rejects with: `INVALID_PARAMETER_VALUE: name is not a valid name. Valid names must contain only alphanumeric characters and underscores, and cannot contain spaces, periods, forward slashes, or control characters.` This made share resources with `shared_as` effectively immutable after creation. ## Solution Remove the line that sets `shared_as = ""` in UPDATE operations, allowing the actual `shared_as` value to be preserved and sent to the API. ## Test Coverage Added ### Unit Tests - `TestDiffUpdateObjectComment` - Verifies updating a single object's comment preserves `shared_as` - `TestDiffAddAndUpdate` - Verifies combining ADD and UPDATE operations preserves `shared_as` on updates - `TestDiffAddUpdateRemove` - Verifies all three operations (ADD, UPDATE, REMOVE) together preserve `shared_as` on updates ### Acceptance Tests - `TestUcAccUpdateShareCommentWithSharedAs` - End-to-end test updating a table comment with `shared_as` defined - `TestUcAccUpdateShareMultipleObjectsWithSharedAs` - End-to-end test updating multiple objects with `shared_as` simultaneously All tests verify that `shared_as` is correctly preserved during UPDATE operations and that updates complete successfully without API validation errors. --------- Co-authored-by: Omer Lachish <rauchy@users.noreply.github.com>
1 parent e189a3f commit 21090c8

File tree

4 files changed

+229
-2
lines changed

4 files changed

+229
-2
lines changed

‎NEXT_CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
### New Features and Improvements
88

99
### Bug Fixes
10+
* [Fix] Allow Updating Share Objects With shared_as Defined ([#5287](https://github.com/databricks/terraform-provider-databricks/pull/5287))
1011

1112
### Documentation
1213

‎internal/providers/pluginfw/products/sharing/resource_share.go‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -108,8 +108,6 @@ func diff(beforeSi sharing.ShareInfo, afterSi sharing.ShareInfo) []sharing.Share
108108
for _, afterSdo := range afterSi.Objects {
109109
if beforeSdo, ok := beforeMap[afterSdo.Name]; ok {
110110
if !equal(beforeSdo, afterSdo) {
111-
// do not send SharedAs
112-
afterSdo.SharedAs = ""
113111
changes = append(changes, sharing.SharedDataObjectUpdate{
114112
Action: sharing.SharedDataObjectUpdateActionUpdate,
115113
DataObject: &afterSdo,

‎internal/providers/pluginfw/products/sharing/resource_share_acc_test.go‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -812,3 +812,79 @@ func TestUcAccShareVolume(t *testing.T) {
812812
Template: shareVolumeTemplate,
813813
})
814814
}
815+
816+
// TestUcAccUpdateShareCommentWithSharedAs tests updating a comment on an object with shared_as
817+
func TestUcAccUpdateShareCommentWithSharedAs(t *testing.T) {
818+
acceptance.UnityWorkspaceLevel(t, acceptance.Step{
819+
Template: preTestTemplate + preTestTemplateUpdate +
820+
`resource "databricks_share" "myshare" {
821+
name = "{var.STICKY_RANDOM}-terraform-delta-share"
822+
owner = "account users"
823+
object {
824+
name = databricks_sql_table.mytable.id
825+
comment = "Original comment"
826+
data_object_type = "TABLE"
827+
shared_as = "things.bar"
828+
history_data_sharing_status = "ENABLED"
829+
}
830+
}`,
831+
}, acceptance.Step{
832+
Template: preTestTemplate + preTestTemplateUpdate +
833+
`resource "databricks_share" "myshare" {
834+
name = "{var.STICKY_RANDOM}-terraform-delta-share"
835+
owner = "account users"
836+
object {
837+
name = databricks_sql_table.mytable.id
838+
comment = "Updated comment"
839+
data_object_type = "TABLE"
840+
shared_as = "things.bar"
841+
history_data_sharing_status = "ENABLED"
842+
}
843+
}`,
844+
})
845+
}
846+
847+
// TestUcAccUpdateShareMultipleObjectsWithSharedAs tests updating multiple objects with shared_as
848+
func TestUcAccUpdateShareMultipleObjectsWithSharedAs(t *testing.T) {
849+
acceptance.UnityWorkspaceLevel(t, acceptance.Step{
850+
Template: preTestTemplate + preTestTemplateUpdate +
851+
`resource "databricks_share" "myshare" {
852+
name = "{var.STICKY_RANDOM}-terraform-delta-share"
853+
owner = "account users"
854+
object {
855+
name = databricks_sql_table.mytable.id
856+
comment = "Table 1 original"
857+
data_object_type = "TABLE"
858+
shared_as = "things.bar"
859+
history_data_sharing_status = "ENABLED"
860+
}
861+
object {
862+
name = databricks_sql_table.mytable_2.id
863+
comment = "Table 2 original"
864+
data_object_type = "TABLE"
865+
shared_as = "things.bar_2"
866+
history_data_sharing_status = "ENABLED"
867+
}
868+
}`,
869+
}, acceptance.Step{
870+
Template: preTestTemplate + preTestTemplateUpdate +
871+
`resource "databricks_share" "myshare" {
872+
name = "{var.STICKY_RANDOM}-terraform-delta-share"
873+
owner = "account users"
874+
object {
875+
name = databricks_sql_table.mytable.id
876+
comment = "Table 1 updated"
877+
data_object_type = "TABLE"
878+
shared_as = "things.bar"
879+
history_data_sharing_status = "ENABLED"
880+
}
881+
object {
882+
name = databricks_sql_table.mytable_2.id
883+
comment = "Table 2 updated"
884+
data_object_type = "TABLE"
885+
shared_as = "things.bar_2"
886+
history_data_sharing_status = "ENABLED"
887+
}
888+
}`,
889+
})
890+
}

‎internal/providers/pluginfw/products/sharing/resource_share_test.go‎

Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -405,3 +405,155 @@ func TestShareChanges(t *testing.T) {
405405
assert.Equal(t, sharing.SharedDataObjectUpdateActionRemove, update.Action)
406406
}
407407
}
408+
409+
// TestDiffUpdateObjectComment tests updating an object's comment field
410+
func TestDiffUpdateObjectComment(t *testing.T) {
411+
before := sharing.ShareInfo{
412+
Name: "test-share",
413+
Objects: []sharing.SharedDataObject{
414+
{
415+
Name: "catalog.schema.table",
416+
DataObjectType: "TABLE",
417+
SharedAs: "schema.table",
418+
Comment: "original comment",
419+
},
420+
},
421+
}
422+
423+
after := sharing.ShareInfo{
424+
Name: "test-share",
425+
Objects: []sharing.SharedDataObject{
426+
{
427+
Name: "catalog.schema.table",
428+
DataObjectType: "TABLE",
429+
SharedAs: "schema.table",
430+
Comment: "updated comment",
431+
},
432+
},
433+
}
434+
435+
changes := diff(before, after)
436+
assert.Len(t, changes, 1, "Should generate UPDATE for comment change")
437+
assert.Equal(t, sharing.SharedDataObjectUpdateActionUpdate, changes[0].Action)
438+
assert.Equal(t, "updated comment", changes[0].DataObject.Comment)
439+
440+
// SharedAs must be preserved in UPDATE operations
441+
assert.Equal(t, "schema.table", changes[0].DataObject.SharedAs,
442+
"SharedAs must be preserved in UPDATE operations")
443+
}
444+
445+
// TestDiffAddAndUpdate tests a combination of ADD and UPDATE operations
446+
func TestDiffAddAndUpdate(t *testing.T) {
447+
before := sharing.ShareInfo{
448+
Name: "test-share",
449+
Objects: []sharing.SharedDataObject{
450+
{
451+
Name: "catalog.schema.table1",
452+
DataObjectType: "TABLE",
453+
SharedAs: "schema.table1",
454+
Comment: "comment1",
455+
},
456+
},
457+
}
458+
459+
after := sharing.ShareInfo{
460+
Name: "test-share",
461+
Objects: []sharing.SharedDataObject{
462+
{
463+
Name: "catalog.schema.table1",
464+
DataObjectType: "TABLE",
465+
SharedAs: "schema.table1",
466+
Comment: "updated comment1",
467+
},
468+
{
469+
Name: "catalog.schema.table2",
470+
DataObjectType: "TABLE",
471+
SharedAs: "schema.table2",
472+
Comment: "comment2",
473+
},
474+
},
475+
}
476+
477+
changes := diff(before, after)
478+
assert.Len(t, changes, 2, "Should generate both UPDATE and ADD")
479+
480+
var hasUpdate, hasAdd bool
481+
for _, change := range changes {
482+
if change.Action == sharing.SharedDataObjectUpdateActionUpdate {
483+
hasUpdate = true
484+
assert.Equal(t, "catalog.schema.table1", change.DataObject.Name)
485+
// SharedAs must be preserved in UPDATE operations
486+
assert.Equal(t, "schema.table1", change.DataObject.SharedAs,
487+
"SharedAs must be preserved in UPDATE operations")
488+
} else if change.Action == sharing.SharedDataObjectUpdateActionAdd {
489+
hasAdd = true
490+
assert.Equal(t, "catalog.schema.table2", change.DataObject.Name)
491+
assert.Equal(t, "schema.table2", change.DataObject.SharedAs)
492+
}
493+
}
494+
assert.True(t, hasUpdate, "Should have UPDATE action")
495+
assert.True(t, hasAdd, "Should have ADD action")
496+
}
497+
498+
// TestDiffAddUpdateRemove tests all three operations together
499+
func TestDiffAddUpdateRemove(t *testing.T) {
500+
before := sharing.ShareInfo{
501+
Name: "test-share",
502+
Objects: []sharing.SharedDataObject{
503+
{
504+
Name: "catalog.schema.table1",
505+
DataObjectType: "TABLE",
506+
SharedAs: "schema.table1",
507+
Comment: "comment1",
508+
},
509+
{
510+
Name: "catalog.schema.table2",
511+
DataObjectType: "TABLE",
512+
SharedAs: "schema.table2",
513+
Comment: "comment2",
514+
},
515+
},
516+
}
517+
518+
after := sharing.ShareInfo{
519+
Name: "test-share",
520+
Objects: []sharing.SharedDataObject{
521+
{
522+
Name: "catalog.schema.table1",
523+
DataObjectType: "TABLE",
524+
SharedAs: "schema.table1",
525+
Comment: "updated comment1",
526+
},
527+
{
528+
Name: "catalog.schema.table3",
529+
DataObjectType: "TABLE",
530+
SharedAs: "schema.table3",
531+
Comment: "comment3",
532+
},
533+
},
534+
}
535+
536+
changes := diff(before, after)
537+
assert.Len(t, changes, 3, "Should generate REMOVE, UPDATE, and ADD")
538+
539+
var hasUpdate, hasAdd, hasRemove bool
540+
for _, change := range changes {
541+
switch change.Action {
542+
case sharing.SharedDataObjectUpdateActionUpdate:
543+
hasUpdate = true
544+
assert.Equal(t, "catalog.schema.table1", change.DataObject.Name)
545+
// SharedAs must be preserved in UPDATE operations
546+
assert.Equal(t, "schema.table1", change.DataObject.SharedAs,
547+
"SharedAs must be preserved in UPDATE operations")
548+
case sharing.SharedDataObjectUpdateActionAdd:
549+
hasAdd = true
550+
assert.Equal(t, "catalog.schema.table3", change.DataObject.Name)
551+
case sharing.SharedDataObjectUpdateActionRemove:
552+
hasRemove = true
553+
assert.Equal(t, "catalog.schema.table2", change.DataObject.Name)
554+
}
555+
}
556+
assert.True(t, hasUpdate, "Should have UPDATE action")
557+
assert.True(t, hasAdd, "Should have ADD action")
558+
assert.True(t, hasRemove, "Should have REMOVE action")
559+
}

0 commit comments

Comments
 (0)