Skip to content

chore: fix db selector read/writes #685

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 16, 2025
Merged

chore: fix db selector read/writes #685

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e76b059
chore: fix db selector read/writes
adityachoudhari26 Oct 16, 2025
169fc17
cleanup
adityachoudhari26 Oct 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions apps/workspace-engine/pkg/db/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package db

import (
"encoding/json"

"workspace-engine/pkg/oapi"
)

func parseJSONToStruct(jsonData []byte) map[string]interface{} {
Expand All @@ -16,3 +18,40 @@ func parseJSONToStruct(jsonData []byte) map[string]interface{} {

return dataMap
}

// wrapSelectorFromDB wraps a db selector in oapi JsonSelector format
func wrapSelectorFromDB(rawMap map[string]interface{}) (*oapi.Selector, error) {
if rawMap == nil {
return nil, nil
}

// Wrap the raw map in JsonSelector format
selector := &oapi.Selector{}
err := selector.FromJsonSelector(oapi.JsonSelector{
Json: rawMap,
})
if err != nil {
return nil, err
}

return selector, nil
}

// unwrapSelectorForDB unwraps a selector for database storage (database stores unwrapped selector format)
// NOTE: CEL selectors are not currently supported - they will be written as NULL to the database.
func unwrapSelectorForDB(selector *oapi.Selector) (map[string]interface{}, error) {
if selector == nil {
return nil, nil
}

// Try as JsonSelector
jsonSelector, err := selector.AsJsonSelector()
if err == nil && jsonSelector.Json != nil {
// Return the unwrapped map directly - pgx can handle it
return jsonSelector.Json, nil
}

// CEL selectors are not supported - return nil to store NULL in database
// TODO: Add support for CEL selectors in the future
return nil, nil
}
Comment on lines +47 to +57
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Do not silently drop non-JSON selectors on write.

If a selector is backed by CEL (or any non-JSON variant), AsJsonSelector fails and this helper returns nil, causing writeEnvironment / writeDeployment / writeManySelectors to persist NULL. Any update will therefore delete existing CEL selectors from the database. Instead of discarding them, either preserve the raw bytes (e.g., marshal and store the existing representation) or fail fast so callers know the selector type is unsupported. The current behaviour causes immediate data loss for CEL selectors.

-	jsonSelector, err := selector.AsJsonSelector()
-	if err == nil && jsonSelector.Json != nil {
-		// Return the unwrapped map directly - pgx can handle it
-		return jsonSelector.Json, nil
-	}
-
-	// CEL selectors are not supported - return nil to store NULL in database
-	// TODO: Add support for CEL selectors in the future
-	return nil, nil
+	jsonSelector, err := selector.AsJsonSelector()
+	if err == nil && jsonSelector.Json != nil {
+		// Return the unwrapped map directly - pgx can handle it
+		return jsonSelector.Json, nil
+	}
+
+	// Fallback: preserve the raw union payload so non-JSON selectors are not lost.
+	rawBytes, err := selector.MarshalJSON()
+	if err != nil {
+		return nil, err
+	}
+	if len(rawBytes) == 0 || string(rawBytes) == "null" {
+		return nil, nil
+	}
+
+	var rawMap map[string]interface{}
+	if err := json.Unmarshal(rawBytes, &rawMap); err != nil {
+		return nil, err
+	}
+	return rawMap, nil

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In apps/workspace-engine/pkg/db/common.go around lines 57 to 67, the helper
currently swallows non-JSON selectors and returns nil which causes CEL (or other
non-JSON) selectors to be persisted as NULL and silently lost; instead, change
the function to not return nil for unsupported selectors: either (1) attempt to
preserve the selector by obtaining its raw representation (marshal or get raw
bytes of the selector) and return that for storage, or (2) if a raw
representation is not available, return a non-nil error so callers fail fast and
can handle unsupported selector types. Update the function to try Json first,
then try to extract raw bytes and return them if present, and only return an
error when no representation can be obtained; ensure callers of this helper
handle the returned error appropriately.

Copy link
Contributor Author

@adityachoudhari26 adityachoudhari26 Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is intentional

Copy link
Contributor

@coderabbitai coderabbitai bot Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adityachoudhari26, understood! I've noted that the current behavior of writing CEL selectors as NULL is intentional for this PR.

✏️ Learnings added 
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#685
File: apps/workspace-engine/pkg/db/common.go:47-57
Timestamp: 2025-10-16T20:59:56.422Z
Learning: In the ctrlplane workspace-engine db package, CEL selectors are intentionally not supported and are written as NULL to the database. This is documented with TODO comments for future implementation.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

19 changes: 17 additions & 2 deletions apps/workspace-engine/pkg/db/deployments.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ func getDeployments(ctx context.Context, workspaceID string) ([]*oapi.Deployment
deployments := make([]*oapi.Deployment, 0)
for rows.Next() {
var deployment oapi.Deployment
var rawSelector map[string]interface{}

err := rows.Scan(
&deployment.Id,
&deployment.Name,
Expand All @@ -47,11 +49,18 @@ func getDeployments(ctx context.Context, workspaceID string) ([]*oapi.Deployment
&deployment.SystemId,
&deployment.JobAgentId,
&deployment.JobAgentConfig,
&deployment.ResourceSelector,
&rawSelector,
)
if err != nil {
return nil, err
}

// Wrap selector from unwrapped database format to JsonSelector format
deployment.ResourceSelector, err = wrapSelectorFromDB(rawSelector)
if err != nil {
return nil, err
}

deployments = append(deployments, &deployment)
}
if err := rows.Err(); err != nil {
Expand All @@ -74,6 +83,12 @@ const DEPLOYMENT_UPSERT_QUERY = `
`

func writeDeployment(ctx context.Context, deployment *oapi.Deployment, tx pgx.Tx) error {
// Unwrap selector for database storage (database stores unwrapped ResourceCondition format)
selectorToStore, err := unwrapSelectorForDB(deployment.ResourceSelector)
if err != nil {
return err
}

if _, err := tx.Exec(
ctx,
DEPLOYMENT_UPSERT_QUERY,
Expand All @@ -84,7 +99,7 @@ func writeDeployment(ctx context.Context, deployment *oapi.Deployment, tx pgx.Tx
deployment.SystemId,
deployment.JobAgentId,
deployment.JobAgentConfig,
deployment.ResourceSelector,
selectorToStore,
); err != nil {
return err
}
Expand Down
110 changes: 18 additions & 92 deletions apps/workspace-engine/pkg/db/deployments_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,88 +464,8 @@ func TestDBDeployments_WithJsonResourceSelector(t *testing.T) {
}
}

func TestDBDeployments_WithCelResourceSelector(t *testing.T) {
workspaceID, conn := setupTestWithWorkspace(t)

tx, err := conn.Begin(t.Context())
if err != nil {
t.Fatalf("failed to begin tx: %v", err)
}
defer tx.Rollback(t.Context())

// Create a system first
systemID := uuid.New().String()
systemDescription := fmt.Sprintf("desc-%s", systemID[:8])
sys := &oapi.System{
Id: systemID,
WorkspaceId: workspaceID,
Name: fmt.Sprintf("test-system-%s", systemID[:8]),
Description: &systemDescription,
}
err = writeSystem(t.Context(), sys, tx)
if err != nil {
t.Fatalf("failed to create system: %v", err)
}

// Create deployment with CEL resource selector
deploymentID := uuid.New().String()
description := "test deployment with CEL selector"

// Create a CEL selector
resourceSelector := &oapi.Selector{}
celExpression := "resource.metadata.environment == 'production'"
err = resourceSelector.FromCelSelector(oapi.CelSelector{
Cel: celExpression,
})
if err != nil {
t.Fatalf("failed to create CEL selector: %v", err)
}

deployment := &oapi.Deployment{
Id: deploymentID,
Name: fmt.Sprintf("test-deployment-%s", deploymentID[:8]),
Slug: fmt.Sprintf("test-deployment-%s", deploymentID[:8]),
SystemId: systemID,
Description: &description,
JobAgentConfig: map[string]interface{}{},
ResourceSelector: resourceSelector,
}

err = writeDeployment(t.Context(), deployment, tx)
if err != nil {
t.Fatalf("expected no errors, got %v", err)
}

err = tx.Commit(t.Context())
if err != nil {
t.Fatalf("failed to commit: %v", err)
}

// Read back and validate
actualDeployments, err := getDeployments(t.Context(), workspaceID)
if err != nil {
t.Fatalf("expected no errors, got %v", err)
}

if len(actualDeployments) != 1 {
t.Fatalf("expected 1 deployment, got %d", len(actualDeployments))
}

actualDeployment := actualDeployments[0]
if actualDeployment.ResourceSelector == nil {
t.Fatalf("expected resource selector to be non-nil")
}

// Validate the selector content
celSelector, err := actualDeployment.ResourceSelector.AsCelSelector()
if err != nil {
t.Fatalf("expected CEL selector, got error: %v", err)
}

if celSelector.Cel != celExpression {
t.Fatalf("expected CEL expression %s, got %s", celExpression, celSelector.Cel)
}
}
// TODO: Add CEL selector tests when CEL support is implemented
// func TestDBDeployments_WithCelResourceSelector(t *testing.T) { ... }

func TestDBDeployments_UpdateResourceSelector(t *testing.T) {
workspaceID, conn := setupTestWithWorkspace(t)
Expand Down Expand Up @@ -605,20 +525,23 @@ func TestDBDeployments_UpdateResourceSelector(t *testing.T) {
t.Fatalf("failed to commit: %v", err)
}

// Update with CEL selector
// Update with a different JSON selector
tx, err = conn.Begin(t.Context())
if err != nil {
t.Fatalf("failed to begin tx: %v", err)
}
defer tx.Rollback(t.Context())

updatedSelector := &oapi.Selector{}
celExpression := "resource.kind == 'pod'"
err = updatedSelector.FromCelSelector(oapi.CelSelector{
Cel: celExpression,
err = updatedSelector.FromJsonSelector(oapi.JsonSelector{
Json: map[string]interface{}{
"type": "kind",
"value": "pod",
"operator": "equals",
},
})
if err != nil {
t.Fatalf("failed to create CEL selector: %v", err)
t.Fatalf("failed to create updated JSON selector: %v", err)
}

deployment.ResourceSelector = updatedSelector
Expand Down Expand Up @@ -648,13 +571,16 @@ func TestDBDeployments_UpdateResourceSelector(t *testing.T) {
t.Fatalf("expected resource selector to be non-nil")
}

// Validate it's now a CEL selector
celSelector, err := actualDeployment.ResourceSelector.AsCelSelector()
// Validate it's the updated JSON selector
jsonSelector, err := actualDeployment.ResourceSelector.AsJsonSelector()
if err != nil {
t.Fatalf("expected CEL selector, got error: %v", err)
t.Fatalf("expected JSON selector, got error: %v", err)
}

if celSelector.Cel != celExpression {
t.Fatalf("expected CEL expression %s, got %s", celExpression, celSelector.Cel)
if jsonSelector.Json["type"] != "kind" {
t.Fatalf("expected type 'kind', got %v", jsonSelector.Json["type"])
}
if jsonSelector.Json["value"] != "pod" {
t.Fatalf("expected value 'pod', got %v", jsonSelector.Json["value"])
}
}
19 changes: 17 additions & 2 deletions apps/workspace-engine/pkg/db/environments.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,18 +39,27 @@ func getEnvironments(ctx context.Context, workspaceID string) ([]*oapi.Environme
for rows.Next() {
var environment oapi.Environment
var createdAt time.Time
var rawSelector map[string]interface{}

err := rows.Scan(
&environment.Id,
&environment.Name,
&environment.SystemId,
&createdAt,
&environment.Description,
&environment.ResourceSelector,
&rawSelector,
)
if err != nil {
return nil, err
}
environment.CreatedAt = createdAt.Format(time.RFC3339)

// Wrap selector from unwrapped database format to JsonSelector format
environment.ResourceSelector, err = wrapSelectorFromDB(rawSelector)
if err != nil {
return nil, err
}

environments = append(environments, &environment)
}
if err := rows.Err(); err != nil {
Expand All @@ -70,14 +79,20 @@ const ENVIRONMENT_UPSERT_QUERY = `
`

func writeEnvironment(ctx context.Context, environment *oapi.Environment, tx pgx.Tx) error {
// Unwrap selector for database storage (database stores unwrapped ResourceCondition format)
selectorToStore, err := unwrapSelectorForDB(environment.ResourceSelector)
if err != nil {
return err
}

if _, err := tx.Exec(
ctx,
ENVIRONMENT_UPSERT_QUERY,
environment.Id,
environment.Name,
environment.SystemId,
environment.Description,
environment.ResourceSelector,
selectorToStore,
); err != nil {
return err
}
Expand Down
Loading
Loading