bundle: address genie_space PR review comments

janniklasrose · janniklasrose · commit 2b186cb4d582 · 2026-06-04T08:21:18.000+02:00
Apply the unresolved self-review comments on PR #5282: - Permissions use object type "genie" (endpoint /permissions/genie/{id}) rather than "genie/spaces" (permissions.go, all_test.go, and the testserver object-type map). - parent_path is a normal, updatable field now that the Genie GET API returns it (without the /Workspace prefix) and the update API accepts it: * DoRead re-adds the /Workspace prefix via the shared ensureWorkspacePrefix (mirrors ResourceDashboard) instead of clearing the field. * DoUpdate sends parent_path so the backend can move the space. * Removed parent_path from recreate_on_changes and ignore_remote_changes. * The testserver strips the /Workspace prefix on write to match the real API, and `generate genie-space` re-adds it so the generated config keeps the conventional form. * Renamed the parent_path_recreate acceptance test to parent_path_update: changing parent_path now plans an update, not a recreate. - serialized_space uses reason: etag_based (matching dashboards) since drift is detected via etag. Also switches isMissingGenieParentPathError to errors.AsType[T] (forbidigo rule from the SDK bump on main) and lets testifylint rewrite the genie assertions in state_load_test.go to assert.Empty. Co-authored-by: Isaac
diff --git a/acceptance/bundle/resources/genie_spaces/inline/out.plan.json b/acceptance/bundle/resources/genie_spaces/inline/out.plan.json
@@ -9,6 +9,7 @@
       "remote_state": {
         "description": "Inline serialized_space test",
         "etag": "1",
+        "parent_path": "/Workspace/Users/[USERNAME]",
         "serialized_space": "{\"config\":{\"sample_questions\":[{\"id\":\"sq-001\",\"question\":[\"What is the total revenue?\"]}]},\"data_sources\":{\"tables\":[{\"column_configs\":[{\"column_name\":\"amount\",\"get_example_values\":true}],\"identifier\":\"main.sales.orders\"}]},\"version\":1}",
         "space_id": "[GENIE_SPACE_ID]",
         "title": "Sales Analytics Inline Genie",
@@ -20,12 +21,6 @@
           "reason": "custom",
           "old": "1",
           "remote": "1"
-        },
-        "parent_path": {
-          "action": "skip",
-          "reason": "input_only",
-          "old": "/Workspace/Users/[USERNAME]",
-          "new": "/Workspace/Users/[USERNAME]"
         }
       }
     }
diff --git a/acceptance/bundle/resources/genie_spaces/parent_path_recreate/output.txt b/acceptance/bundle/resources/genie_spaces/parent_path_recreate/output.txt
diff --git a/acceptance/bundle/resources/genie_spaces/parent_path_update/databricks.yml.tmpl b/acceptance/bundle/resources/genie_spaces/parent_path_update/databricks.yml.tmpl
@@ -1,10 +1,10 @@
 bundle:
-  name: recreate-genie-space-$UNIQUE_NAME
+  name: update-genie-space-$UNIQUE_NAME
 
 resources:
   genie_spaces:
-    recreate_target:
-      title: "Recreate Target"
+    update_target:
+      title: "Update Target"
       warehouse_id: "test-warehouse-id"
       parent_path: PARENT_PATH_PLACEHOLDER
       serialized_space: "{}"
diff --git a/acceptance/bundle/resources/genie_spaces/parent_path_update/out.test.toml b/acceptance/bundle/resources/genie_spaces/parent_path_update/out.test.toml
diff --git a/acceptance/bundle/resources/genie_spaces/parent_path_update/output.txt b/acceptance/bundle/resources/genie_spaces/parent_path_update/output.txt
@@ -0,0 +1,21 @@
+
+>>> [CLI] bundle deploy
+Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/update-genie-space-[UNIQUE_NAME]/default/files...
+Deploying resources...
+Updating deployment state...
+Deployment complete!
+
+=== Plan after changing parent_path should show update
+>>> [CLI] bundle plan
+update genie_spaces.update_target
+
+Plan: 0 to add, 1 to change, 0 to delete, 0 unchanged
+
+>>> [CLI] bundle destroy --auto-approve
+The following resources will be deleted:
+  delete resources.genie_spaces.update_target
+
+All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/update-genie-space-[UNIQUE_NAME]/default
+
+Deleting files...
+Destroy complete!
diff --git a/acceptance/bundle/resources/genie_spaces/parent_path_update/script b/acceptance/bundle/resources/genie_spaces/parent_path_update/script
@@ -7,8 +7,8 @@ trap cleanup EXIT
 envsubst < databricks.yml.tmpl | sed "s|PARENT_PATH_PLACEHOLDER|/Users/$CURRENT_USER_NAME/genie-old|" > databricks.yml
 trace $CLI bundle deploy
 
-# Change parent_path. parent_path is recreate_on_changes in resources.yml,
-# so the plan should show a recreate (delete + create) rather than an update.
+# Change parent_path. The Genie update API moves the space to the new parent,
+# so the plan should show an update rather than a recreate (delete + create).
 envsubst < databricks.yml.tmpl | sed "s|PARENT_PATH_PLACEHOLDER|/Users/$CURRENT_USER_NAME/genie-new|" > databricks.yml
-title "Plan after changing parent_path should show recreate"
+title "Plan after changing parent_path should show update"
 trace $CLI bundle plan
diff --git a/acceptance/bundle/resources/genie_spaces/parent_path_update/test.toml b/acceptance/bundle/resources/genie_spaces/parent_path_update/test.toml
diff --git a/acceptance/bundle/resources/genie_spaces/simple/out.plan.json b/acceptance/bundle/resources/genie_spaces/simple/out.plan.json
@@ -9,6 +9,7 @@
       "remote_state": {
         "description": "AI assistant for sales data analysis",
         "etag": "1",
+        "parent_path": "/Workspace/Users/[USERNAME]",
         "serialized_space": "{\n  \"benchmarks\": {\n    \"questions\": [\n      {\n        \"answer\": [\n          {\n            \"content\": [\n              \"SELECT\\n\",\n              \"  name,\\n\",\n              \"  country\\n\",\n              \"FROM main.default.countries\\n\",\n              \"ORDER BY name\"\n            ],\n            \"format\": \"SQL\"\n          }\n        ],\n        \"id\": \"[NUMID]\",\n        \"question\": [\n          \"Show all names and countries\"\n        ]\n      }\n    ]\n  },\n  \"config\": {\n    \"sample_questions\": [\n      {\n        \"id\": \"[NUMID]\",\n        \"question\": [\n          \"List the names and countries\"\n        ]\n      },\n      {\n        \"id\": \"[NUMID]\",\n        \"question\": [\n          \"Which names are in Canada?\"\n        ]\n      }\n    ]\n  },\n  \"data_sources\": {\n    \"tables\": [\n      {\n        \"column_configs\": [\n          {\n            \"column_name\": \"country\",\n            \"enable_entity_matching\": true,\n            \"enable_format_assistance\": true\n          },\n          {\n            \"column_name\": \"name\",\n            \"enable_entity_matching\": true,\n            \"enable_format_assistance\": true\n          }\n        ],\n        \"identifier\": \"main.default.countries\"\n      }\n    ]\n  },\n  \"instructions\": {\n    \"example_question_sqls\": [\n      {\n        \"id\": \"[NUMID]\",\n        \"question\": [\n          \"List the names and countries\"\n        ],\n        \"sql\": [\n          \"SELECT\\n\",\n          \"  name,\\n\",\n          \"  country\\n\",\n          \"FROM main.default.countries\\n\",\n          \"ORDER BY name\"\n        ]\n      }\n    ],\n    \"text_instructions\": [\n      {\n        \"content\": [\n          \"This genie space answers simple questions about people and their countries.\\n\",\n          \"Use only the main.default.countries table.\\n\",\n          \"Prefer returning the name and country columns directly.\"\n        ],\n        \"id\": \"[NUMID]\"\n      }\n    ]\n  },\n  \"version\": 2\n}\n",
         "space_id": "[GENIE_SPACE_ID]",
         "title": "Sales Analytics Genie",
@@ -20,12 +21,6 @@
           "reason": "custom",
           "old": "1",
           "remote": "1"
-        },
-        "parent_path": {
-          "action": "skip",
-          "reason": "input_only",
-          "old": "/Workspace/Users/[USERNAME]",
-          "new": "/Workspace/Users/[USERNAME]"
         }
       }
     }
diff --git a/acceptance/bundle/resources/permissions/genie_spaces/current_can_manage/out.plan.direct.json b/acceptance/bundle/resources/permissions/genie_spaces/current_can_manage/out.plan.direct.json
@@ -44,7 +44,7 @@
           ]
         },
         "vars": {
-          "object_id": "/genie/spaces/${resources.genie_spaces.foo.id}"
+          "object_id": "/genie/${resources.genie_spaces.foo.id}"
         }
       }
     }
diff --git a/acceptance/bundle/resources/permissions/genie_spaces/current_can_manage/out.requests.deploy.direct.json b/acceptance/bundle/resources/permissions/genie_spaces/current_can_manage/out.requests.deploy.direct.json
@@ -1,6 +1,6 @@
 {
   "method": "PUT",
-  "path": "/api/2.0/permissions/genie/spaces/[FOO_ID]",
+  "path": "/api/2.0/permissions/genie/[FOO_ID]",
   "body": {
     "access_control_list": [
       {
diff --git a/bundle/direct/dresources/all_test.go b/bundle/direct/dresources/all_test.go
@@ -515,7 +515,7 @@ var testDeps = map[string]prepareWorkspace{
 		}
 
 		return &PermissionsState{
-			ObjectID: "/genie/spaces/" + resp.SpaceId,
+			ObjectID: "/genie/" + resp.SpaceId,
 			EmbeddedSlice: []StatePermission{{
 				Level:    "CAN_MANAGE",
 				UserName: "user@example.com",
diff --git a/bundle/direct/dresources/genie_space.go b/bundle/direct/dresources/genie_space.go
@@ -25,9 +25,6 @@ var pathSerializedSpace = structpath.MustParsePath("serialized_space")
 //     PrepareState returns the config directly.
 //   - RemapState filters fewer fields: Genie has no LifecycleState / CreateTime /
 //     Path / UpdateTime output-only fields to scrub.
-//   - DoRead clears ParentPath: the GET API does not reliably return parent_path,
-//     so we drop it from ForceSendFields and zero the value rather than re-adding
-//     a "/Workspace" prefix the way dashboard.go does in ensureWorkspacePrefix.
 //   - DoUpdate omits serialized_space when unchanged: serialized_space is in
 //     ignore_remote_changes (see resources.yml), so a UI edit produces no plan
 //     entry. Sending the local body anyway would clobber the UI edit on every
@@ -97,18 +94,14 @@ func prepareGenieSpaceRequest(config *resources.GenieSpaceConfig) (string, error
 }
 
 func responseToGenieSpaceConfig(space *dashboards.GenieSpace, serializedSpace string) *resources.GenieSpaceConfig {
-	// Drop ParentPath from ForceSendFields. We always clear ParentPath
-	// below because the GET Genie space API does not reliably return it,
-	// and keeping it in ForceSendFields would force-emit parent_path: ""
-	// in state output even though the field is logically unset.
-	forceSendFields := utils.FilterFields[resources.GenieSpaceConfig](space.ForceSendFields, "ParentPath")
+	forceSendFields := utils.FilterFields[resources.GenieSpaceConfig](space.ForceSendFields)
 
 	return &resources.GenieSpaceConfig{
 		Description:     space.Description,
 		Etag:            space.Etag,
 		Title:           space.Title,
 		WarehouseId:     space.WarehouseId,
-		ParentPath:      "",
+		ParentPath:      ensureWorkspacePrefix(space.ParentPath),
 		SerializedSpace: serializedSpace,
 
 		// Output only fields
@@ -141,8 +134,8 @@ func isMissingGenieParentPathError(err error) bool {
 		return true
 	}
 
-	var apiErr *apierr.APIError
-	if !errors.As(err, &apiErr) {
+	apiErr, ok := errors.AsType[*apierr.APIError](err)
+	if !ok {
 		return false
 	}
 
@@ -218,6 +211,7 @@ func (r *ResourceGenieSpace) DoUpdate(ctx context.Context, id string, config *re
 		Description:     config.Description,
 		Title:           config.Title,
 		WarehouseId:     config.WarehouseId,
+		ParentPath:      config.ParentPath,
 		SerializedSpace: serializedSpace,
 		// Send the etag we last observed. The backend uses it as an If-Match
 		// guard against concurrent writes, and OverrideChangeDesc uses the
diff --git a/bundle/direct/dresources/permissions.go b/bundle/direct/dresources/permissions.go
@@ -18,7 +18,7 @@ var permissionResourceToObjectType = map[string]string{
 	"apps":                    "/apps/",
 	"clusters":                "/clusters/",
 	"dashboards":              "/dashboards/",
-	"genie_spaces":            "/genie/spaces/",
+	"genie_spaces":            "/genie/",
 	"database_instances":      "/database-instances/",
 	"postgres_projects":       "/database-projects/",
 	"jobs":                    "/jobs/",
diff --git a/bundle/direct/dresources/resources.yml b/bundle/direct/dresources/resources.yml
@@ -360,17 +360,11 @@ resources:
         reason: input_only
 
   genie_spaces:
-    recreate_on_changes:
-      - field: parent_path
-        reason: immutable
     ignore_remote_changes:
       # serialized_space locally (structured YAML) and remotely (JSON string) will differ
       # textually, so we cannot meaningfully compare them for drift.
       - field: serialized_space
-        reason: input_only
-      # parent_path is not reliably returned by the GET Genie space API.
-      - field: parent_path
-        reason: input_only
+        reason: etag_based
 
   apps:
     recreate_on_changes:
diff --git a/bundle/generate/genie_space.go b/bundle/generate/genie_space.go
@@ -1,6 +1,9 @@
 package generate
 
 import (
+	"path"
+	"strings"
+
 	"github.com/databricks/cli/libs/dyn"
 	"github.com/databricks/databricks-sdk-go/service/dashboards"
 )
@@ -19,8 +22,19 @@ func ConvertGenieSpaceToValue(genieSpace *dashboards.GenieSpace, filePath string
 	}
 
 	if genieSpace.ParentPath != "" {
-		dv["parent_path"] = dyn.NewValue(genieSpace.ParentPath, []dyn.Location{{Line: 5}})
+		dv["parent_path"] = dyn.NewValue(ensureWorkspacePrefix(genieSpace.ParentPath), []dyn.Location{{Line: 5}})
 	}
 
 	return dyn.V(dv), nil
 }
+
+// ensureWorkspacePrefix re-adds the /Workspace prefix that the Genie GET API
+// strips from parent_path, so the generated config matches the convention used
+// in hand-written bundles and in deployment state (mirrors the equivalent
+// helper in bundle/direct/dresources/dashboard.go).
+func ensureWorkspacePrefix(parentPath string) string {
+	if parentPath == "/Workspace" || strings.HasPrefix(parentPath, "/Workspace/") {
+		return parentPath
+	}
+	return path.Join("/Workspace", parentPath)
+}
diff --git a/bundle/statemgmt/state_load_test.go b/bundle/statemgmt/state_load_test.go
@@ -906,10 +906,10 @@ func TestStateToBundleModifiedResources(t *testing.T) {
 	assert.Equal(t, resources.ModifiedStatusCreated, config.Resources.Dashboards["test_dashboard_new"].ModifiedStatus)
 
 	assert.Equal(t, "1", config.Resources.GenieSpaces["test_genie_space"].ID)
-	assert.Equal(t, "", config.Resources.GenieSpaces["test_genie_space"].ModifiedStatus)
+	assert.Empty(t, config.Resources.GenieSpaces["test_genie_space"].ModifiedStatus)
 	assert.Equal(t, "2", config.Resources.GenieSpaces["test_genie_space_old"].ID)
 	assert.Equal(t, resources.ModifiedStatusDeleted, config.Resources.GenieSpaces["test_genie_space_old"].ModifiedStatus)
-	assert.Equal(t, "", config.Resources.GenieSpaces["test_genie_space_new"].ID)
+	assert.Empty(t, config.Resources.GenieSpaces["test_genie_space_new"].ID)
 	assert.Equal(t, resources.ModifiedStatusCreated, config.Resources.GenieSpaces["test_genie_space_new"].ModifiedStatus)
 
 	assert.Equal(t, "test_app", config.Resources.Apps["test_app"].Name)
diff --git a/libs/testserver/genie_spaces.go b/libs/testserver/genie_spaces.go
@@ -45,6 +45,13 @@ func (s *FakeWorkspace) GenieSpaceCreate(req Request) Response {
 		}
 	}
 
+	// Strip the /Workspace prefix from parent_path before storing. This matches
+	// the remote behavior: the GET API returns parent_path without the prefix,
+	// mirroring DashboardCreate.
+	if strings.HasPrefix(createReq.ParentPath, "/Workspace/") {
+		createReq.ParentPath = strings.TrimPrefix(createReq.ParentPath, "/Workspace")
+	}
+
 	genieSpace := dashboards.GenieSpace{
 		SpaceId:         spaceId,
 		Title:           createReq.Title,
@@ -146,6 +153,13 @@ func (s *FakeWorkspace) GenieSpaceUpdate(req Request) Response {
 	if updateReq.WarehouseId != "" {
 		genieSpace.WarehouseId = updateReq.WarehouseId
 	}
+	if updateReq.ParentPath != "" {
+		parentPath := updateReq.ParentPath
+		if strings.HasPrefix(parentPath, "/Workspace/") {
+			parentPath = strings.TrimPrefix(parentPath, "/Workspace")
+		}
+		genieSpace.ParentPath = parentPath
+	}
 	if updateReq.SerializedSpace != "" {
 		genieSpace.SerializedSpace = updateReq.SerializedSpace
 	}
@@ -157,6 +171,7 @@ func (s *FakeWorkspace) GenieSpaceUpdate(req Request) Response {
 	if prev.Title != genieSpace.Title ||
 		prev.Description != genieSpace.Description ||
 		prev.WarehouseId != genieSpace.WarehouseId ||
+		prev.ParentPath != genieSpace.ParentPath ||
 		prev.SerializedSpace != genieSpace.SerializedSpace {
 		prevEtag, err := strconv.Atoi(genieSpace.Etag)
 		if err != nil {
diff --git a/libs/testserver/permissions.go b/libs/testserver/permissions.go
@@ -25,7 +25,7 @@ var requestObjectTypeToObjectType = map[string]string{
 	"sql/alerts":              "alert",
 	"sql/queries":             "query",
 	"dashboards":              "dashboard",
-	"genie/spaces":            "genie-space",
+	"genie":                   "genie-space",
 	"experiments":             "mlflowExperiment",
 	"registered-models":       "registered-model",
 	"serving-endpoints":       "serving-endpoint",

Original file line number	Diff line number	Diff line change
`@@ -44,7 +44,7 @@`
`44`	`44`	`]`
`45`	`45`	`},`
`46`	`46`	`"vars": {`
`47`		`- "object_id": "/genie/spaces/${resources.genie_spaces.foo.id}"`
	`47`	`+ "object_id": "/genie/${resources.genie_spaces.foo.id}"`
`48`	`48`	`}`
`49`	`49`	`}`
`50`	`50`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1,6 +1,6 @@`
`1`	`1`	`{`
`2`	`2`	`"method": "PUT",`
`3`		`- "path": "/api/2.0/permissions/genie/spaces/[FOO_ID]",`
	`3`	`+ "path": "/api/2.0/permissions/genie/[FOO_ID]",`
`4`	`4`	`"body": {`
`5`	`5`	`"access_control_list": [`
`6`	`6`	`{`