feat: support tenant.metadata

[alexluong]

No description provided.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
outpost-docs	Ready	Preview	Comment	Oct 28, 2025 4:21am
outpost-website	Ready	Preview	Comment	Oct 28, 2025 4:21am

[alexluong]

@claude review please

[claude]

Claude finished @alexluong's task —— View job

---

Code Review Progress

• Read and analyze changed files
• Review code quality and implementation
• Provide feedback

---

Review Summary

I've reviewed the metadata implementation for the Tenant API. The implementation is solid and comprehensive, with excellent test coverage. Below are my findings organized by category:

a. Code Quality & Best Practices

The implementation demonstrates good Go practices:

• Consistent patterns: The metadata handling follows the same patterns already established for destination.metadata and destination.delivery_metadata
• Clean separation: Business logic is properly separated between models, handlers, and storage layers
• Idiomatic Go: Uses proper nil handling for optional fields with omitempty JSON tags
• Factory pattern: The new TenantFactory in testutil/tenant.go follows the existing factory pattern nicely

b. Implementation Details

1. Redis Persistence (internal/models/entity.go:159-167)

The implementation correctly handles metadata storage:

• Sets metadata when present
• Explicitly deletes the field when nil (important for clearing metadata)
• Uses proper error handling

2. Deserialization (internal/models/tenant.go:33-39)

The parsing logic properly handles missing metadata:

• Only attempts deserialization if the field exists in Redis
• Returns descriptive errors on unmarshal failures

3. Handler Logic (internal/services/api/tenant_handlers.go:40-47)

The handler gracefully handles invalid JSON by continuing with nil metadata, which is reasonable for optional fields. However, there's a subtle issue here (see Potential Issues section).

c. Test Coverage

Excellent test coverage across multiple layers:

1. Unit tests (internal/models/tenant_test.go):

   • JSON marshaling/unmarshaling with and without metadata
   • Empty metadata preservation

2. Integration tests (internal/models/entitysuite_test.go:121-160):

   • Full CRUD lifecycle with metadata
   • Updating metadata
   • Clearing metadata (nil handling)

3. E2E tests (cmd/e2e/api_test.go:270-415):

   • Creating tenants with metadata
   • Retrieving metadata
   • Full replacement semantics (PUT replaces entire metadata object)
   • Clearing metadata
   • Proper omitempty behavior in JSON responses

The E2E tests are particularly thorough and clearly document the expected behavior with descriptive test names and comments.

d. Potential Issues

1. Silent Error Swallowing in Handler (internal/services/api/tenant_handlers.go:44-47)

if err := c.ShouldBindJSON(&input); err != nil {
    // If body is empty or invalid, that's OK - metadata is optional
    // Continue with nil metadata
}

This silently ignores all JSON binding errors, including:

• Malformed JSON (e.g., {"metadata": "not-an-object"})
• Type mismatches
• Syntax errors

Concern: If a client sends {"metadata": 123} or invalid JSON, they won't receive any error feedback. They might think their metadata was stored when it wasn't.

Recommendation: Distinguish between "no body" (acceptable) and "invalid body" (should error):

var input struct {
    Metadata models.Metadata `json:"metadata,omitempty"`
}
if c.Request.ContentLength > 0 {
    if err := c.ShouldBindJSON(&input); err != nil {
        AbortWithError(c, http.StatusBadRequest, NewErrInvalidJSON(err))
        return
    }
}

2. PUT Replacement Semantics Not Validated (internal/services/api/tenant_handlers.go:56-58)

The handler performs full replacement of metadata on PUT (line 58), which is correct for PUT semantics. However, there's no validation that clients understand this behavior.

Observation: The E2E tests correctly verify this behavior (lines 319-342), showing that PUT with different metadata replaces the entire object. This is good documentation but consider adding a note in the API documentation about this replacement behavior.

3. Missing Input Validation

There's no validation on:

• Metadata key/value lengths
• Number of metadata entries
• Reserved key names (if any)

Recommendation: Consider adding reasonable limits to prevent abuse:

if len(input.Metadata) > 50 {
    // error: too many metadata entries
}
for k, v := range input.Metadata {
    if len(k) > 255 || len(v) > 1024 {
        // error: key or value too long
    }
}

e. Security Considerations

No major security concerns, but worth noting:

1. Metadata is stored unencrypted in Redis (unlike credentials which are encrypted). This is appropriate for metadata, but ensure documentation clarifies that sensitive data shouldn't be stored here.

2. No sanitization of metadata keys/values. If metadata is displayed in UIs, consider XSS implications.

f. Performance Considerations

No performance concerns:

1. Metadata is stored alongside tenant data in the same Redis hash, so no extra round trips
2. Binary marshaling uses efficient msgpack encoding
3. Proper use of Redis pipelines maintained

g. Documentation

OpenAPI Spec (docs/apis/openapi.yaml):

• ✅ Properly documents metadata as nullable object with string key-value pairs
• ✅ Includes in TenantUpsert schema
• ✅ Shows omitempty behavior

Suggestion: Add a note about PUT replacement semantics in the operation description:

description: |
  Idempotently creates or updates a tenant. Required before associating destinations.
  Note: PUT replaces the entire metadata object if provided. To preserve existing 
  metadata, retrieve the tenant first and merge metadata client-side.

---

Recommendation

Overall: Approve with minor suggestions. The implementation is well-tested and follows established patterns. The main improvement would be to handle invalid JSON more explicitly in the handler to provide better feedback to API clients.

Priority:

• Medium: Fix silent error swallowing in tenant handler (suggestion d.1 above)
• Low: Add metadata validation limits
• Low: Document PUT replacement semantics in OpenAPI