Skip to content

Bug fix: Go SDK clients + support same process testing#1

Open
coxley wants to merge 5 commits into
charlesgreen:mainfrom
coxley:go-sdk-fixes
Open

Bug fix: Go SDK clients + support same process testing#1
coxley wants to merge 5 commits into
charlesgreen:mainfrom
coxley:go-sdk-fixes

Conversation

@coxley

@coxley coxley commented Oct 15, 2025

Copy link
Copy Markdown

Summary

Firstly, thank you very much for creating this. :) Following the documented examples didn't quite work for me, and these changes address that:

  • While Google's API reference shows camelCase field names, the SDK types use snake_case in their field tags. Upstream handles both, so this adds a normalization step when decoding payloads.
  • POST /v1/projects/*/secrets actually expects secretId in query parameters instead of the body. SDKs in other languages might do it this way, and since Google is doing gRPC transcoding both work, but the Go client doesn't have a way to configure that so we now support both.
  • secretmanager.NewClient uses gRPC by default. I've updated the documentation and examples for secretmanager.RESTClient
  • storage.PersistentStorage had a few deadlocks due to interleaved mutexes.

Outside of those fixes, it would be handy for Go users to use the emulator without fork/exec or Docker. I've created the gsmtest package as a parallel to bttest, pstest, and others provided by Google.

The new package is self-tested with the Go client to make sure that all the fixes above remain working.

Thanks again for the package, and any time you take to review.

Test Plan

> go test ./... -count=1
?       github.com/charlesgreen/gsm/cmd/server  [no test files]
ok      github.com/charlesgreen/gsm/gsmtest     0.680s
?       github.com/charlesgreen/gsm/internal/api/handlers       [no test files]
?       github.com/charlesgreen/gsm/internal/api/middleware     [no test files]
?       github.com/charlesgreen/gsm/internal/api/routes [no test files]
?       github.com/charlesgreen/gsm/internal/models     [no test files]
?       github.com/charlesgreen/gsm/internal/storage    [no test files]
?       github.com/charlesgreen/gsm/pkg/client  [no test files]
ok      github.com/charlesgreen/gsm/tests/integration   0.246s
ok      github.com/charlesgreen/gsm/tests/unit  0.523s

Summary by CodeRabbit

  • New Features

    • Local Secret Manager emulator with in-memory and persistent modes; emulator clients use REST-based connections when configured.
  • Documentation

    • README updated with a "Testing Configuration" section and examples for emulator usage.
  • Tests

    • Added tests covering TCP, in-memory, and persistent emulator modes.
  • Refactor

    • Unified JSON decoding with automatic snake_case → camelCase normalization for incoming requests.

@coderabbitai

coderabbitai Bot commented Oct 15, 2025

Copy link
Copy Markdown

Walkthrough

Adds a configurable GSM emulator test harness (TCP, in-memory via memconn, persistent), updates example clients to use secretmanager.NewRESTClient for emulator hosts, introduces decodeJSON to normalize snake_case→camelCase, adjusts persistence locking and file permissions, and adds tests and README testing examples.

Changes

Cohort / File(s) Summary of Changes
Documentation & Examples
README.md, pkg/client/example.go
README extended with a "Testing Configuration" section and examples; example client code now uses secretmanager.NewRESTClient when SECRET_MANAGER_EMULATOR_HOST is set.
Module Dependencies
go.mod
Added dependency: github.com/pires/memconn v0.1.0.
GSM Test Harness & Tests
gsmtest/test.go, gsmtest/server_test.go
New gsmtest package providing SecretManager emulator with options (InMemory, Listener, Addr, StorageFile, ShutdownTimeout), Start/Addr/Endpoint/Client methods, memconn-backed in-memory REST client support; tests TestTCP, TestMem, TestPersistent.
Request Decoding Utilities
internal/api/handlers/json.go
Added generic decodeJSON[T any] that decodes into map[string]any, normalizes snake_case keys to camelCase recursively (skipping labels/annotations/versionAliases), then unmarshals into target type.
API Handlers
internal/api/handlers/secrets.go, internal/api/handlers/versions.go
Replaced direct JSON decoding with decodeJSON; set defaults for missing fields (using cmp.Or), small formatting and TODO updates.
Persistence/Storage
internal/storage/persistence.go
Simplified locking in Load/Save to rely on memory storage mutexing, adjusted error-path locking to MemoryStorage.mu, changed file mode to 0o644, and minor whitespace/formatting edits.
Tests for JSON Decoding
internal/api/handlers/json_test.go
Added tests validating decodeJSON behavior and key normalization for nested structures.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Test as Test Harness
  participant GSM as SecretManager (emulator)
  participant HTTP as HTTP Server
  participant Handlers as API Handlers
  participant Store as Storage

  rect rgba(214,234,248,0.35)
    note over Test,GSM: Setup & client creation
    Test->>GSM: New(...options)
    Test->>GSM: Start(ctx)
    GSM->>HTTP: Serve(listener)
    Test->>GSM: Client(ctx)
    alt in-memory transport
      GSM-->>Test: memconn-backed REST client
    else TCP transport
      GSM-->>Test: REST client (endpoint URL)
    end
  end

  rect rgba(235,245,224,0.35)
    note over HTTP,Handlers: Create secret flow
    Test->>Handlers: POST /v1/.../secrets (body)
    Handlers->>Handlers: decodeJSON(body) → normalize keys
    Handlers->>Store: CreateSecret(data)
    Store-->>Handlers: Secret
    Handlers-->>Test: 200 OK
  end

  rect rgba(252,243,207,0.35)
    note over HTTP,Handlers: Add version & access
    Test->>Handlers: POST /:addVersion
    Handlers->>Store: AddSecretVersion(payload)
    Store-->>Handlers: Version
    Test->>Handlers: GET /:access
    Handlers->>Store: AccessSecretVersion
    Store-->>Handlers: Payload
    Handlers-->>Test: 200 OK (payload)
  end

  rect rgba(242,215,213,0.25)
    note over Test,GSM: Shutdown
    Test-->>GSM: cancel ctx
    GSM->>HTTP: graceful shutdown (with timeout)
  end
Loading
sequenceDiagram
  autonumber
  participant Client as Example Client
  participant Env as Environment
  participant GSM as Emulator

  Client->>Env: Read SECRET_MANAGER_EMULATOR_HOST
  alt present
    Client->>GSM: secretmanager.NewRESTClient(ctx, WithEndpoint, WithNoAuth)
    GSM-->>Client: *secretmanager.Client (REST)
  else absent
    Client->>GCP: secretmanager.NewClient(ctx)
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

I thump the keys from snake to camel,
spin a memconn tunnel — soft as a flannel.
Secrets dance in file or RAM,
I hop, I test, I clap my yam.
Carrots for CI, hops for the PR — hooray! 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "Bug fix: Go SDK clients + support same process testing" directly aligns with the actual changes in this pull request. The PR addresses two main themes: first, it implements bug fixes and improvements related to Go SDK client compatibility (REST client usage in pkg/client/example.go, secretId query parameter support in internal/api/handlers/secrets.go, snake_case to camelCase normalization, and deadlock fixes in persistent storage), and second, it introduces a comprehensive new gsmtest package to enable same-process testing without fork/exec or Docker. The title is sufficiently specific and clear that a teammate scanning the commit history would understand the PR involves Go SDK client enhancements and in-process testing support. While the title doesn't enumerate all specific fixes (as expected per the guidelines), it accurately captures the primary objectives.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
internal/storage/persistence.go (2)

36-57: Load should lock MemoryStorage when swapping p.secrets

Swap the underlying map under MemoryStorage’s write lock to avoid races with readers/writers.

 func (p *PersistentStorage) Load() error {
-	p.mu.Lock()
-	defer p.mu.Unlock()
+	// If MemoryStorage has a mutex, prefer it for mutating p.secrets.
+	// p.mu is not necessary to protect the in-memory map.
 
 	if _, err := os.Stat(p.filePath); os.IsNotExist(err) {
 		return nil
 	}
@@
 	var storageData Data
 	if err := json.Unmarshal(data, &storageData); err != nil {
 		return fmt.Errorf("failed to parse storage file: %w", err)
 	}
 
-	p.secrets = storageData.Secrets
+	p.MemoryStorage.mu.Lock()
+	p.secrets = storageData.Secrets
+	p.MemoryStorage.mu.Unlock()
 	return nil
 }

104-114: Rollback after failed Save should use MemoryStorage lock

Use the same lock MemoryStorage uses for p.secrets to maintain consistency and avoid races.

-	if err := p.Save(); err != nil {
-		p.mu.Lock()
+	if err := p.Save(); err != nil {
+		p.MemoryStorage.mu.Lock()
 		key := fmt.Sprintf("%s/%s", projectID, secretID)
 		if secret, exists := p.secrets[key]; exists {
 			delete(secret.Versions, version.GetVersionID())
 			secret.VersionCount--
 		}
-		p.mu.Unlock()
+		p.MemoryStorage.mu.Unlock()
 		return nil, err
 	}
🧹 Nitpick comments (5)
internal/api/handlers/secrets.go (2)

40-40: Use URL query directly for secretId

Prefer r.URL.Query().Get to avoid ParseMultipartForm side effects from FormValue.

-req.SecretID = cmp.Or(req.SecretID, r.FormValue("secretId"))
+req.SecretID = cmp.Or(req.SecretID, r.URL.Query().Get("secretId"))

124-126: TODO: Support ?etag on DeleteSecret

If you want, I can implement conditional delete (If-Match via etag query) consistent with API semantics.

gsmtest/server_test.go (1)

50-55: Log server start errors to aid debugging

Capture and log Start(ctx) errors to avoid silent failures in CI.

-	go gsm.Start(ctx)
+	go func() {
+		if err := gsm.Start(ctx); err != nil {
+			t.Logf("server exited with error: %v", err)
+		}
+	}()
gsmtest/test.go (2)

155-168: Mem transport client: set Endpoint and disable auth

Add endpoint and no-auth options for consistency and to avoid relying on default Google endpoint.

 func (s *SecretManager) memClient(ctx context.Context) (*secretmanager.Client, error) {
@@
-	return secretmanager.NewRESTClient(ctx, option.WithHTTPClient(client))
+	return secretmanager.NewRESTClient(
+		ctx,
+		option.WithHTTPClient(client),
+		option.WithoutAuthentication(),
+		option.WithEndpoint(s.Endpoint()),
+	)
 }

103-131: Flush storage on shutdown

Ensure persistent backends save at shutdown.

 func (s *SecretManager) Start(ctx context.Context) error {
@@
-		if err := s.srv.Shutdown(timeCtx); err != nil {
+		if err := s.srv.Shutdown(timeCtx); err != nil {
 			s.tb.Logf("error shutting down: %v", err)
 		}
+		if err := s.store.Close(); err != nil {
+			s.tb.Logf("error closing storage: %v", err)
+		}
 	}()
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 58eaaca and 6f428ad.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (9)
  • README.md (3 hunks)
  • go.mod (1 hunks)
  • gsmtest/server_test.go (1 hunks)
  • gsmtest/test.go (1 hunks)
  • internal/api/handlers/json.go (1 hunks)
  • internal/api/handlers/secrets.go (6 hunks)
  • internal/api/handlers/versions.go (2 hunks)
  • internal/storage/persistence.go (4 hunks)
  • pkg/client/example.go (4 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
internal/api/handlers/secrets.go (2)
internal/models/secret.go (1)
  • Secret (9-17)
internal/models/response.go (1)
  • CreateSecretData (35-38)
gsmtest/server_test.go (4)
gsmtest/test.go (4)
  • New (68-92)
  • InMemory (52-56)
  • StorageFile (27-31)
  • SecretManager (94-100)
internal/models/response.go (2)
  • CreateSecretRequest (29-32)
  • AddSecretVersionRequest (41-43)
internal/models/secret.go (1)
  • Secret (9-17)
internal/models/version.go (1)
  • SecretPayload (43-46)
gsmtest/test.go (4)
internal/api/routes/router.go (1)
  • SetupRoutes (15-83)
internal/storage/interface.go (1)
  • Storage (20-34)
internal/storage/memory.go (1)
  • NewMemoryStorage (21-25)
internal/storage/persistence.go (2)
  • PersistentStorage (15-19)
  • NewPersistentStorage (29-34)
🪛 markdownlint-cli2 (0.18.1)
README.md

247-247: Hard tabs
Column: 1

(MD010, no-hard-tabs)


248-248: Hard tabs
Column: 1

(MD010, no-hard-tabs)


249-249: Hard tabs
Column: 1

(MD010, no-hard-tabs)


250-250: Hard tabs
Column: 1

(MD010, no-hard-tabs)


252-252: Hard tabs
Column: 1

(MD010, no-hard-tabs)


253-253: Hard tabs
Column: 1

(MD010, no-hard-tabs)


254-254: Hard tabs
Column: 1

(MD010, no-hard-tabs)


256-256: Hard tabs
Column: 1

(MD010, no-hard-tabs)


257-257: Hard tabs
Column: 1

(MD010, no-hard-tabs)


258-258: Hard tabs
Column: 1

(MD010, no-hard-tabs)


259-259: Hard tabs
Column: 1

(MD010, no-hard-tabs)


260-260: Hard tabs
Column: 1

(MD010, no-hard-tabs)


267-267: Hard tabs
Column: 1

(MD010, no-hard-tabs)


268-268: Hard tabs
Column: 1

(MD010, no-hard-tabs)


269-269: Hard tabs
Column: 1

(MD010, no-hard-tabs)


270-270: Hard tabs
Column: 1

(MD010, no-hard-tabs)

🔇 Additional comments (3)
internal/storage/persistence.go (2)

75-76: LGTM: explicit 0o644 permissions

Using 0o644 is clear and idiomatic.


60-78: Potential data race: Save reads p.secrets without MemoryStorage lock

If MemoryStorage protects p.secrets with its own mutex, Save should take that lock (RLock) while marshalling to avoid concurrent map read/write. Consider this refactor (serialize file writes without interleaving and avoid interlocking with p.mu):

Proposed change:

-func (p *PersistentStorage) Save() error {
-	p.mu.Lock()
-	defer p.mu.Unlock()
-
-	storageData := Data{
-		Secrets:   p.secrets,
-		Timestamp: time.Now().UTC(),
-		Version:   "1.0.0",
-	}
-
-	data, err := json.MarshalIndent(storageData, "", "  ")
+func (p *PersistentStorage) Save() error {
+	// Snapshot under MemoryStorage read lock to avoid races while marshalling.
+	// Assumes MemoryStorage has a mutex named mu (same package).
+	p.MemoryStorage.mu.RLock()
+	storageData := Data{
+		Secrets:   p.secrets,
+		Timestamp: time.Now().UTC(),
+		Version:   "1.0.0",
+	}
+	data, err := json.MarshalIndent(storageData, "", "  ")
+	p.MemoryStorage.mu.RUnlock()
 	if err != nil {
 		return fmt.Errorf("failed to marshal storage data: %w", err)
 	}
 
 	if err := os.WriteFile(p.filePath, data, 0o644); err != nil {
 		return fmt.Errorf("failed to write storage file: %w", err)
 	}
 
 	return nil
 }
internal/api/handlers/secrets.go (1)

4-4: cmp.Or requires a recent Go toolchain

Ensure go.mod specifies a Go version that supports cmp.Or (Go 1.22+).

Comment thread internal/api/handlers/json.go
Comment thread internal/api/handlers/secrets.go Outdated
Comment thread README.md

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (1)
internal/api/handlers/json.go (1)

29-50: Critical issue from past review remains unaddressed.

This function still normalizes user-provided map keys (labels, annotations, versionAliases), corrupting caller data. For example, labels: {"created_by": "ci"} becomes {"createdBy": "ci"}, breaking API parity.

Please apply the fix suggested in the previous review comment to skip normalization for known user-defined fields.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6f428ad and 23ab9fe.

📒 Files selected for processing (1)
  • internal/api/handlers/json.go (1 hunks)
🔇 Additional comments (1)
internal/api/handlers/json.go (1)

15-27: LGTM with a note on performance.

The decode-normalize-reencode flow is correct. The round-trip encoding is inefficient but acceptable for an emulator context.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
internal/storage/persistence.go (1)

59-68: Race condition: Inconsistent mutex usage.

Save() reads p.secrets at line 64 while holding p.mu.Lock(), but other methods (Load() at line 52, AddSecretVersion() error path at line 105) mutate p.secrets under p.MemoryStorage.mu.Lock(). Since p.secrets belongs to the embedded MemoryStorage struct, all accesses should use p.MemoryStorage.mu for synchronization. The current code uses two separate mutexes to protect the same data, creating a data race.

Apply this diff to use the correct mutex:

 func (p *PersistentStorage) Save() error {
-	p.mu.Lock()
-	defer p.mu.Unlock()
+	p.MemoryStorage.mu.RLock()
+	defer p.MemoryStorage.mu.RUnlock()
 
 	storageData := Data{
 		Secrets:   p.secrets,
 		Timestamp: time.Now().UTC(),
 		Version:   "1.0.0",
 	}
+
+	p.MemoryStorage.mu.RUnlock()
 
 	data, err := json.MarshalIndent(storageData, "", "  ")
 	if err != nil {
 		return fmt.Errorf("failed to marshal storage data: %w", err)
 	}
 
 	if err := os.WriteFile(p.filePath, data, 0o644); err != nil {
 		return fmt.Errorf("failed to write storage file: %w", err)
 	}
 
 	return nil
 }

Note: Using RLock() since Save() only reads p.secrets. Release the lock before marshaling to avoid holding it during I/O.

♻️ Duplicate comments (2)
internal/api/handlers/json.go (2)

35-59: Add continue to make the skip list effective.

The skip list check at lines 38-41 is ineffective because execution falls through to the recursive normalization logic below. When a key matches the skip list (e.g., "labels"), the code assigns v at line 40 but then continues to lines 45-56 where:

  1. If v is a map[string]any, line 47 recursively normalizes it anyway, corrupting user data like {"created_by": "ci"} into {"createdBy": "ci"}.
  2. Line 56 overwrites dst[camelCase] with the now-normalized value.

This breaks API parity and corrupts caller data in fields like labels, annotations, and versionAliases.

Apply this diff to skip normalization for listed keys:

 	for k, v := range src {
 		camelCase := toCamelCase(k)
 		if _, ok := skipNormalizationFor[camelCase]; ok {
 			dst[camelCase] = v
+			continue
 		}
 
 		// Decide whether we need to run recursively for other objects or arrays of
 		// objects
 		switch vv := v.(type) {
 		case map[string]any:
 			v = normalizeKeys(vv)
 		case []any:
 			for i, inner := range vv {
 				if m, ok := inner.(map[string]any); ok {
 					vv[i] = normalizeKeys(m)
 				}
 			}
 			v = vv
 		}
 		dst[camelCase] = v
 	}

61-75: Leading underscore edge case remains.

The guard check at lines 69-71 correctly prevents panics on consecutive or trailing underscores. However, a leading underscore produces incorrect output: "_foo"["", "foo"]camelCase = "" at line 67, then appends "Foo", yielding "Foo" instead of "_foo" or "foo".

While leading underscores are uncommon in JSON keys, consider this edge case if strict API compatibility is required:

 func toCamelCase(s string) string {
 	parts := strings.Split(s, "_")
 	if len(parts) < 2 {
 		return s
 	}
 
-	camelCase := parts[0]
+	// Skip leading empty parts from leading underscores
+	var camelCase string
+	firstIdx := 0
+	for i, p := range parts {
+		if len(p) > 0 {
+			camelCase = p
+			firstIdx = i + 1
+			break
+		}
+	}
+	if camelCase == "" {
+		return s
+	}
+	
-	for _, p := range parts[1:] {
+	for _, p := range parts[firstIdx:] {
 		if len(p) == 0 {
 			continue
 		}
 		camelCase += strings.ToUpper(p[:1]) + p[1:]
 	}
 	return camelCase
 }
🧹 Nitpick comments (2)
README.md (2)

232-241: Env-based client: minor doc tweak.

Add the os import in the snippet so it compiles when copy/pasted.

Apply:

+// import "os"
 func newSecretManagerClient(ctx context.Context) (*secretmanager.Client, error) {
     if emulatorHost := os.Getenv("SECRET_MANAGER_EMULATOR_HOST"); emulatorHost != "" {
         return secretmanager.NewRESTClient(ctx,
             option.WithEndpoint("http://"+emulatorHost),
             option.WithoutAuthentication(),
         )
     }

243-275: Fix markdownlint MD010 (hard tabs) in Testing Configuration block.

Either replace tabs with spaces or disable MD010 around the fenced code to keep Go’s tab formatting.

Apply:

-### Testing Configuration
-
-```go
+### Testing Configuration
+
+<!-- markdownlint-disable MD010 no-hard-tabs -->
+```go
 func TestTCP(t *testing.T) {
 	gsm, err := gsmtest.New(t)
 	if err != nil {
 		t.Fatal(err)
 	}
 
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 	go gsm.Start(ctx)
 
 	client, err := gsm.Client(ctx)
 	if err != nil {
 		t.Fatal(err)
 	}
 	defer client.Close()
     // Use normally
 }
 
 func TestMem(t *testing.T) {
     // Uses local buffer instead of network sockets. Enables use with new packages like
     // testing/synctest
 	gsm, err := gsmtest.New(t, gsmtest.InMemory())
 	if err != nil {
 		t.Fatal(err)
 	}
     // Same as other test
 }
 

+


</blockquote></details>

</blockquote></details>

<details>
<summary>📜 Review details</summary>

**Configuration used**: CodeRabbit UI

**Review profile**: CHILL

**Plan**: Pro

<details>
<summary>📥 Commits</summary>

Reviewing files that changed from the base of the PR and between 23ab9fe47e23b4e33704403fa9e46ea6ae19a987 and 87b896ac8befb8762a9e2049b72388e3f8865d9f.

</details>

<details>
<summary>📒 Files selected for processing (4)</summary>

* `README.md` (3 hunks)
* `internal/api/handlers/json.go` (1 hunks)
* `internal/api/handlers/secrets.go` (6 hunks)
* `internal/storage/persistence.go` (4 hunks)

</details>

<details>
<summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary>

* internal/api/handlers/secrets.go

</details>

<details>
<summary>🧰 Additional context used</summary>

<details>
<summary>🧬 Code graph analysis (1)</summary>

<details>
<summary>internal/storage/persistence.go (1)</summary><blockquote>

<details>
<summary>internal/storage/memory.go (1)</summary>

* `MemoryStorage` (15-18)

</details>

</blockquote></details>

</details><details>
<summary>🪛 markdownlint-cli2 (0.18.1)</summary>

<details>
<summary>README.md</summary>

247-247: Hard tabs
Column: 1

(MD010, no-hard-tabs)

---

248-248: Hard tabs
Column: 1

(MD010, no-hard-tabs)

---

249-249: Hard tabs
Column: 1

(MD010, no-hard-tabs)

---

250-250: Hard tabs
Column: 1

(MD010, no-hard-tabs)

---

252-252: Hard tabs
Column: 1

(MD010, no-hard-tabs)

---

253-253: Hard tabs
Column: 1

(MD010, no-hard-tabs)

---

254-254: Hard tabs
Column: 1

(MD010, no-hard-tabs)

---

256-256: Hard tabs
Column: 1

(MD010, no-hard-tabs)

---

257-257: Hard tabs
Column: 1

(MD010, no-hard-tabs)

---

258-258: Hard tabs
Column: 1

(MD010, no-hard-tabs)

---

259-259: Hard tabs
Column: 1

(MD010, no-hard-tabs)

---

260-260: Hard tabs
Column: 1

(MD010, no-hard-tabs)

---

267-267: Hard tabs
Column: 1

(MD010, no-hard-tabs)

---

268-268: Hard tabs
Column: 1

(MD010, no-hard-tabs)

---

269-269: Hard tabs
Column: 1

(MD010, no-hard-tabs)

---

270-270: Hard tabs
Column: 1

(MD010, no-hard-tabs)

</details>

</details>

</details>

<details>
<summary>🔇 Additional comments (5)</summary><blockquote>

<details>
<summary>README.md (1)</summary><blockquote>

`213-216`: **Good switch to NewRESTClient for emulator usage.**

This avoids gRPC defaults and matches the emulator’s HTTP endpoint.

</blockquote></details>
<details>
<summary>internal/api/handlers/json.go (1)</summary><blockquote>

`10-33`: **LGTM: Function structure is correct.**

The `decodeJSON` function correctly implements a decode-normalize-remarshal flow to handle snake_case to camelCase conversion. The error handling properly distinguishes between decoding failures and normalization encoding failures. The skip list at lines 10-14 appropriately identifies user-defined maps that should not be normalized.

</blockquote></details>
<details>
<summary>internal/storage/persistence.go (3)</summary><blockquote>

`52-54`: **LGTM: Correct mutex usage.**

Using `p.MemoryStorage.mu.Lock()` to protect `p.secrets` assignment is correct since `secrets` belongs to the embedded `MemoryStorage` struct. This aligns with the locking pattern used by other `MemoryStorage` methods.

---

`74-74`: **LGTM: Explicit octal literal.**

Using `0o644` instead of `0644` makes the octal notation explicit and aligns with modern Go style guidelines.

---

`98-116`: **LGTM: Correct mutex in error path.**

The error rollback at lines 105-111 correctly uses `p.MemoryStorage.mu.Lock()` to protect mutations to `p.secrets`, maintaining consistency with the locking pattern in `Load()` and ensuring proper synchronization.

</blockquote></details>

</blockquote></details>

</details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (1)
internal/api/handlers/json.go (1)

21-33: Consider the performance trade-off of the decode-marshal-unmarshal round-trip.

The three-step approach (decode to map[string]any → normalize → marshal → unmarshal to T) is clear but involves extra allocations and serialization overhead. For an emulator this may be acceptable, but if performance becomes a concern you could explore direct field mapping during the initial decode phase.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 87b896a and 56db653.

📒 Files selected for processing (2)
  • README.md (3 hunks)
  • internal/api/handlers/json.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • README.md
🔇 Additional comments (1)
internal/api/handlers/json.go (1)

10-14: Good approach to protect user-provided maps.

The skip list correctly identifies fields containing user data that should not have their keys normalized. This addresses the concern from the previous review about mangling labels.

Comment thread internal/api/handlers/json.go
Comment thread internal/api/handlers/json.go

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (3)
internal/api/handlers/json_test.go (2)

20-45: Good coverage of the happy path.

The test validates snake_case → camelCase normalization, skip list behavior, and nested structures. The JSON payload appropriately exercises both user-defined map keys (keep_me) and schema field keys (version_aliases, user_managed).

Consider adding test cases for edge conditions:

func TestDecodeEdgeCases(t *testing.T) {
    tests := []struct {
        name    string
        input   string
        wantErr bool
    }{
        {"empty object", `{}`, false},
        {"malformed JSON", `{invalid}`, true},
        {"null values", `{"secret_id": null}`, false},
        {"empty arrays", `{"secret": {"replication": {"user_managed": {"replicas": []}}}}`, false},
    }
    // ... test each case
}

47-72: Thorough validation of normalization behavior.

The assertions correctly verify:

  • Skip-listed map keys are preserved (keep_me in labels, annotations, versionAliases)
  • Non-skipped fields are normalized (secret_idsecretId)
  • Nested structures are handled (replicas[0].location)

For more comprehensive validation, consider also asserting that nested field names were normalized (e.g., verify user_managed became accessible via UserManaged):

if req.Secret.Replication.UserManaged == nil {
    t.Fatal("expected UserManaged to be populated from user_managed")
}
internal/api/handlers/json.go (1)

16-33: Sound implementation with clear documentation.

The decode-normalize-remarshal approach correctly handles the snake_case/camelCase incompatibility between Go SDK struct tags and the public API schema. Error handling properly distinguishes normalization failures from decode failures.

For high-throughput scenarios, consider a more efficient approach that normalizes during the initial decode pass rather than the decode → marshal → unmarshal cycle. However, the current implementation prioritizes correctness and maintainability, which is appropriate for an emulator.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 56db653 and 7957e1c.

📒 Files selected for processing (2)
  • internal/api/handlers/json.go (1 hunks)
  • internal/api/handlers/json_test.go (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/api/handlers/json_test.go (1)
internal/models/secret.go (2)
  • Secret (9-17)
  • Replication (20-23)
🔇 Additional comments (4)
internal/api/handlers/json_test.go (1)

10-18: LGTM! Clear test structure.

The example struct effectively covers all three skip-list fields plus nested structures. Using map[string]int instead of map[string]string is fine for test purposes and doesn't affect validation of the normalization logic.

internal/api/handlers/json.go (3)

10-14: Skip list correctly addresses user data preservation.

The skip list includes all user-defined map fields that should not have their keys normalized. Using camelCase entries is correct since the check occurs after key conversion (line 38-39).


35-60: Normalization logic is correct.

The skip list check on line 39 correctly prevents recursion into user-defined maps. The recursive handling of nested maps (line 48) and array elements (lines 50-54) ensures deep normalization for schema-defined fields while preserving user data in protected fields.


62-82: Critical bug fixed: toCamelCase now correctly preserves first segment.

The function now iterates over all parts (line 69), correctly producing:

  • "foo_bar""fooBar"
  • "version_aliases""versionAliases"
  • "secret_id""secretId"

This fixes the critical issue flagged in past reviews where parts[1:] was dropping the first segment. The skip list on lines 10-14 now functions as intended.

@charlesgreen

Copy link
Copy Markdown
Owner

@coxley thank you for the pull request and the feedback. It is very helpful.
Sorry for the very long delay. I didn't see this until just now. I'll review and update.

@charlesgreen charlesgreen self-assigned this Apr 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants