feat: analytics #4068
feat: analytics #4068
Conversation
remove docs db schema for users
- Adds docs for analytics + errors - Rewrite of selects and others - Tranforming ch results - Other stuff im missing
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughImplements a new analytics SQL endpoint (POST /v2/analytics.getVerifications) with RBAC permission read_analytics, ClickHouse-backed query execution, a secure parser/rewriter, per-workspace connection management, result transformation, extensive tests, CLI tools for ClickHouse user setup and seeding, docs updates (overview, getting started, schema, security, errors), and config/generation adjustments. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API as API Route (v2/analytics.getVerifications)
participant RBAC as RBAC/Permissions
participant ACM as AnalyticsConnectionManager
participant CH as ClickHouse
participant Parser as Query Parser/Rewriter
participant Caches as Caches (API/Identity/Settings)
participant Xform as Result Transformer
Client->>API: POST query
API->>RBAC: Check read_analytics (+ per-API filter)
RBAC-->>API: Allowed/Denied
alt Denied
API-->>Client: 403 Forbidden
else Allowed
API->>ACM: Get workspace CH connection
ACM->>Caches: Load settings (SWR/cache)
ACM->>CH: Connect/Ping (with DSN)
alt Connection error
API-->>Client: 404/503 (not configured/connection failed)
else OK
API->>Parser: Parse + rewrite (workspace filter, limits, aliases)
Parser->>Caches: Resolve virtual columns (API/identity)
alt Invalid query/table/function/type
API-->>Client: 400 Bad Request
else OK
API->>CH: Execute rewritten SQL
alt Resource limits/quota hit
API-->>Client: 422/429 with specific code
else Success
API->>Xform: Map actual IDs -> virtual IDs
Xform->>Caches: Batch reverse-resolve (SWRMany)
Xform-->>API: Transformed rows
API-->>Client: 200 { verifications: [...] }
end
end
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~150 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
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. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
coderabbitai
bot
requested review from
MichaelUnkey,
imeyer,
mcstepp and
perkinsjr
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (27)
go/Makefile (1)
37-37: Pin the Buf module version.
go run github.com/bufbuild/buf/cmd/buf generatewill resolve to “latest” whenever the module isn’t already required, which makes codegen nondeterministic and can break as soon as a new Buf release lands (or simply when offline). Please pin the module explicitly (e.g.@v1.xx.x) so the build is repeatable.-generate: - go run github.com/bufbuild/buf/cmd/buf generate +generate: + go run github.com/bufbuild/buf/cmd/[email protected] generatego/go.mod (4)
306-306: Use oftooldirective for sqlc looks good; ensure reproducibility.Good move to pin tooling. Commit a minimal tools README or Makefile target showing how to invoke codegen via the
tooldirective to keep contributors aligned.
9-9: Confirm ClickHouse SQL parser choice
- AfterShip v0.4.13 is MIT-licensed but shows low maintenance; for long-term support consider hydrolix/clickhouse-sql-parser (Apache-2.0) or hungtcs/clickhouse-sql-parser.
- Verify the parser grammar aligns with your target ClickHouse dialect.
300-302: Update go-yit pin to a maintained release with YAML dependency bumps
Replace the 2019 commit pin with a go-yit tag/commit that includes the merged upgrades to go.yaml.in/yaml v3/v4 (see dprotaso/go-yit PRs) to receive upstream parser fixes. Also pin the underlying YAML module and monitor advisories (CVE-2019-11254, CVE-2021-4235, CVE-2022-3064, CVE-2022-28948).
205-205: Align parser version pin and document workaround
- go/go.mod lines 205 (require v0.0.0-20250930075046-70c7d5051c59) vs. 304 (replace → v0.0.0-20250806091815-327a22d5ebf8) are mismatched; the replace isn’t applied in the module graph
- Link to the upstream SQLC engine issue/PR that motivated this pin
- Open a tracking issue to remove or adjust the replace once SQLC is fixed
- Pin to the minimal commit that resolves the SQLC errors and re-test
go/apps/api/config.go (1)
56-58: LGTM! DSN template field added for analytics connections.The new field enables runtime provisioning of workspace-specific analytics connections. The comment clearly documents the expected format with placeholders for credentials.
Optional: Consider adding DSN template validation.
For production readiness, consider validating the DSN template format in the
Validate()method to catch configuration errors early rather than at runtime.Example validation:
func (c Config) Validate() error { + if c.ClickhouseAnalyticsDSN != "" { + if !strings.Contains(c.ClickhouseAnalyticsDSN, "%s") { + return fmt.Errorf("ClickhouseAnalyticsDSN must contain placeholders %%s for username and password") + } + } + // TLS configuration is validated when it's created from filesgo/pkg/clickhouse/query-parser/README.md (1)
145-221: Fix gin handler returns and client method names in examples
- In a gin handler, don’t
return c.JSON(...); callc.JSON(...)thenreturn. The current sample won’t compile.- Align the execution call with actual client API (e.g., use your Querier’s
QueryToMaps(ctx, safeQuery)instead ofclickhouse.Query(...)) to avoid confusion.Apply this minimal fix to the handler flow:
func HandleAnalyticsQuery(c *gin.Context) { @@ - if err := c.BindJSON(&req); err != nil { - return c.JSON(400, gin.H{"error": "invalid request"}) - } + if err := c.BindJSON(&req); err != nil { + c.JSON(400, gin.H{"error": "invalid request"}) + return + } @@ - safeQuery, err := parser.Parse(c.Request.Context(), req.Query) - if err != nil { - return c.JSON(400, gin.H{"error": err.Error()}) - } + safeQuery, err := parser.Parse(c.Request.Context(), req.Query) + if err != nil { + c.JSON(400, gin.H{"error": err.Error()}) + return + } @@ - rows, err := clickhouse.Query(c.Request.Context(), safeQuery) - if err != nil { - return c.JSON(500, gin.H{"error": "query failed"}) - } - - return c.JSON(200, gin.H{"data": rows}) + rows, err := ch.QueryToMaps(c.Request.Context(), safeQuery) + if err != nil { + c.JSON(500, gin.H{"error": "query failed"}) + return + } + c.JSON(200, gin.H{"data": rows}) }Confirm the public client you expose to handlers is named
ch(or adjust accordingly), and that it implementsQueryToMaps.go/pkg/db/api_find_key_auth_by_key_auth_ids.sql_generated.go (1)
42-54: Preallocate params slice for tiny perf/readability winCurrent code is correct. You can preallocate
queryParamscapacity.- var queryParams []interface{} - queryParams = append(queryParams, arg.WorkspaceID) + queryParams := make([]interface{}, 0, 1+len(arg.KeyAuthIds)) + queryParams = append(queryParams, arg.WorkspaceID)go/cmd/create-clickhouse-user/main.go (1)
101-118: Close connections (and optionally ping) to tidy resourcesNot critical for a short-lived CLI, but closing is cheap and avoids leaks in longer runs; ping can fail fast.
database, err := db.New(db.Config{ @@ if err != nil { return fmt.Errorf("failed to connect to database: %w", err) } + defer database.Close() @@ ch, err := clickhouse.New(clickhouse.Config{ @@ if err != nil { return fmt.Errorf("failed to connect to clickhouse: %w", err) } + defer ch.Close() + if err := ch.Ping(ctx); err != nil { + return fmt.Errorf("clickhouse ping failed: %w", err) + }Also applies to: 110-118
go/pkg/clickhouse/errors.go (1)
174-216: Solid error wrapping; consider preferring code-based checks before substring matchesCurrent flow checks substrings before code-based mapping for resource limits. For precision, you might invert the order (check
*ch.Exceptioncodes first), then fallback to substrings.No functional change required; suggestion is for precision and fewer false positives on generic words like “timeout” appearing in unrelated errors.
apps/docs/errors/user/bad_request/query_quota_exceeded.mdx (1)
3-3: Consider clarifying "current window" for users.The description mentions "query quota for the current window" but doesn't define what the window duration is (e.g., hourly, daily). Users may not understand the timeframe of rate limiting.
Consider updating the description to be more specific:
-description: "QueryQuotaExceeded indicates the workspace has exceeded their query quota for the current window." +description: "QueryQuotaExceeded indicates the workspace has exceeded their query quota for the current time window (e.g., hourly or daily limit)."go/pkg/clickhouse/query-parser/errors_test.go (3)
12-68: Expand test coverage to include all error scenarios.The test currently validates 4 error cases (invalid syntax, invalid table, invalid function, query not supported), but the PR introduces several additional error codes documented in other files:
query_memory_limit_exceededquery_rows_limit_exceededquery_result_rows_limit_exceededquery_execution_timeoutquery_quota_exceededConsider adding test cases for these additional error scenarios to ensure comprehensive coverage of the error handling surface:
{ name: "query memory limit exceeded", config: Config{ WorkspaceID: "ws_123", AllowedTables: []string{"default.keys_v2"}, MaxMemoryBytes: 1000, // Configure low limit }, query: "SELECT * FROM default.keys_v2 JOIN default.keys_v2", expectedCode: codes.User.BadRequest.QueryMemoryLimitExceeded.URN(), expectedError: "Query exceeded memory limit", }, // Add similar cases for other limits
82-84: Strengthen assertion for public error message validation.Using
require.Containsfor message validation only checks for substring presence, which can lead to false positives if the actual message contains the expected text but also includes additional unexpected content or if the substring appears in a different context.Consider using exact string matching or a more specific assertion pattern:
- // Check public message - publicMsg := fault.UserFacingMessage(err) - require.Contains(t, publicMsg, tt.expectedError) + // Check public message + publicMsg := fault.UserFacingMessage(err) + require.Equal(t, tt.expectedError, publicMsg, "Public message should match exactly")Alternatively, if exact matching is too strict, use a regex pattern to validate the message structure more precisely.
28-28: Use a more deterministic invalid-SQL test case.
Replace"SELECT * FROM @@@"with a query that will always trigger a syntax error—e.g."SELECT FROM table"or"SELECT * WHERE"—so the parser reliably returnsInvalid SQL syntax.apps/docs/errors/unkey/data/analytics_connection_failed.mdx (1)
1-7: Consider adding troubleshooting guidance for connection failures.This error indicates an internal infrastructure issue (analytics database connection failure). The documentation would be more helpful if it included guidance for users on:
- Whether this is a transient error that might resolve on retry
- Expected system behavior when this occurs
- Who to contact (support) if the issue persists
Consider expanding the documentation:
--- title: "analytics_connection_failed" description: "ConnectionFailed indicates the connection to the analytics database failed." --- <Danger>`err:unkey:data:analytics_connection_failed`</Danger> + +This is an internal infrastructure error indicating that the system could not establish a connection to the analytics database. + +**What to do:** +- Retry your request after a brief delay +- If the issue persists, please contact support + +This error typically indicates a temporary service disruption and should resolve automatically.apps/docs/errors/unkey/data/analytics_not_configured.mdx (1)
1-7: Add actionable guidance for configuring analytics.This error indicates that analytics is not configured for the workspace. Since users can potentially resolve this themselves, the documentation should include:
- How to enable/configure analytics for their workspace
- Link to analytics setup/getting started documentation
- Note that analytics may be in private beta (as mentioned in PR objectives)
Consider expanding the documentation:
--- title: "analytics_not_configured" description: "NotConfigured indicates analytics is not configured for the workspace." --- <Danger>`err:unkey:data:analytics_not_configured`</Danger> + +This error indicates that analytics has not been configured for your workspace. + +**To resolve:** +1. Visit your workspace settings +2. Navigate to the Analytics section +3. Follow the setup instructions to configure analytics + +**Note:** Analytics is currently in private beta. If you don't have access, please contact support to request access. + +For more information, see the [Analytics Getting Started Guide](/analytics/overview).apps/docs/errors/user/bad_request/query_execution_timeout.mdx (1)
3-3: Consider documenting the timeout value and optimization tips.The description mentions "maximum execution time limit" but doesn't specify what that limit is. Users would benefit from knowing:
- The specific timeout value (e.g., 30 seconds, 60 seconds)
- How to optimize queries to avoid timeouts
- Whether timeouts are configurable per plan/tier
Consider updating the description to be more informative:
-description: "QueryExecutionTimeout indicates the query exceeded the maximum execution time limit." +description: "QueryExecutionTimeout indicates the query exceeded the maximum execution time limit (e.g., 30 seconds). Consider optimizing your query or reducing the time range."apps/docs/errors/user/bad_request/query_result_rows_limit_exceeded.mdx (1)
1-7: Document the result rows limit and provide workarounds.The description mentions "maximum result rows limit" but doesn't specify:
- What the limit is (e.g., 10,000 rows)
- The difference between
query_result_rows_limit_exceededandquery_rows_limit_exceeded(mentioned in AI summary)- How users can work around this limit (pagination, filtering, aggregation)
Consider expanding the documentation:
--- title: "query_result_rows_limit_exceeded" description: "QueryResultRowsLimitExceeded indicates the query exceeded the maximum result rows limit." --- <Danger>`err:user:bad_request:query_result_rows_limit_exceeded`</Danger> + +Your query returned more rows than the maximum allowed result size (e.g., 10,000 rows). + +**To resolve:** +- Add more specific filters (e.g., date range, conditions) to reduce result size +- Use aggregation functions (COUNT, SUM, AVG) instead of returning raw rows +- Implement pagination if you need to retrieve large datasets +- Consider using LIMIT clause to cap the result size + +This limit protects both system resources and query performance.go/pkg/db/queries/api_find_key_auth_by_ids.sql (2)
4-4: Consider LEFT JOIN instead of INNER JOIN
Theapis.key_auth_idcolumn is nullable (noNOT NULLin schema), soJOIN key_auth as ka ON ka.id = a.key_auth_idwill exclude rows wherekey_auth_idis NULL. Switch toLEFT JOINif you need to include those APIs with no key_auth.
1-8: Add pagination and verify index coverage for large data sets
- Add a LIMIT or pagination to avoid unbounded result sets when
api_idsis large.- Confirm upstream caps on
api_idsto prevent excessively large IN clauses.apishasworkspace_id_idxbut consider adding composite indexes on(workspace_id, deleted_at_m)and onkey_auth_idfor the JOIN filter.apis.key_auth_idis nullable—inner JOIN excludes NULLs; switch to LEFT JOIN or enforce NOT NULL if you need to include all APIs.apps/dashboard/app/(app)/[workspaceSlug]/settings/root-keys/components/root-key/permissions.ts (1)
28-32: Clarify the description.The phrase "you will still need the seperate analytics permissions" is unclear. What "separate analytics permissions" are required beyond
api.*.read_analytics?Consider clarifying this to avoid user confusion, or remove the phrase if it's not necessary.
go/pkg/clickhouse/query-parser/limits_test.go (2)
27-40: Consider adding workspace ID verification for consistency.This test validates limit addition but doesn't verify workspace ID injection, unlike the other tests. For consistency, consider adding a workspace ID assertion.
Apply this diff to add the workspace ID check:
output, err := p.Parse(context.Background(), "SELECT * FROM default.keys_v2") require.NoError(t, err) require.Contains(t, strings.ToLower(output.Query), "limit 50") + require.Contains(t, strings.ToLower(output.Query), "ws_123")
42-55: Consider adding workspace ID verification for consistency.Similar to the previous test, this one doesn't verify workspace ID injection. For consistency across all limit tests, consider adding a workspace ID assertion.
Apply this diff to add the workspace ID check:
output, err := p.Parse(context.Background(), "SELECT * FROM default.keys_v2 LIMIT 10") require.NoError(t, err) require.Contains(t, strings.ToLower(output.Query), "limit 10") + require.Contains(t, strings.ToLower(output.Query), "ws_123")go/pkg/db/querier_bulk_generated.go (1)
14-14: Awkward plural form in generated method name.The method name
InsertClickhouseWorkspaceSettingsesuses an awkward double plural (Settingses). While this is generated code, consider reviewing the sqlc configuration or naming rules to produceInsertClickhouseWorkspaceSettingsinstead.go/pkg/clickhouse/query-parser/virtual_columns_test.go (1)
11-79: Consider testing the virtual column resolver.The tests configure a
Resolverfunction (lines 19-21 and 43-45) but never verify it's called or check the resolved values. Consider adding tests that verify:
- The resolver is invoked with the correct external IDs
- The resolved mappings are applied correctly to query results
go/pkg/clickhouse/query-parser/validation_test.go (1)
45-56: Consider testing additional non-SELECT statements.While this test covers INSERT, consider adding test cases for other write/mutation operations (UPDATE, DELETE, DROP, ALTER, TRUNCATE, CREATE) to ensure comprehensive enforcement of SELECT-only queries.
Expand the test to cover more statement types:
func TestParser_OnlySelectAllowed(t *testing.T) { p := NewParser(Config{ WorkspaceID: "ws_123", AllowedTables: []string{ "default.keys_v2", }, }) - _, err := p.Parse(context.Background(), "INSERT INTO default.keys_v2 VALUES (1)") - require.Error(t, err) - require.Contains(t, err.Error(), "SELECT") + nonSelectStmts := []string{ + "INSERT INTO default.keys_v2 VALUES (1)", + "UPDATE default.keys_v2 SET value = 1", + "DELETE FROM default.keys_v2", + "DROP TABLE default.keys_v2", + } + + for _, stmt := range nonSelectStmts { + _, err := p.Parse(context.Background(), stmt) + require.Error(t, err, "Statement should be blocked: %s", stmt) + require.Contains(t, err.Error(), "SELECT") + } }go/apps/api/openapi/spec/paths/v2/analytics/getVerifications/index.yaml (1)
1-57: LGTM with documentation suggestion!The endpoint specification is comprehensive with proper security controls (rootKey), thorough error handling (400/401/403/404/500), and clear descriptions. The documentation appropriately highlights workspace isolation and security controls.
Minor enhancement: Consider adding a direct link or path to the "comprehensive schema reference" mentioned in line 14, making it easier for API consumers to find the detailed documentation.
Example enhancement to the description:
For complete documentation including available tables, columns, data types, query examples, and best practices, see the comprehensive schema reference in the API documentation. + + Schema reference: /docs/api/analytics/schema
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
go/gen/proto/cache/v1/invalidation.pb.gois excluded by!**/*.pb.go,!**/gen/**go/go.sumis excluded by!**/*.sumpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (107)
apps/dashboard/app/(app)/[workspaceSlug]/settings/root-keys/[keyId]/permissions/permissions.ts(1 hunks)apps/dashboard/app/(app)/[workspaceSlug]/settings/root-keys/components/root-key/permissions.ts(2 hunks)apps/docs/analytics/getting-started.mdx(1 hunks)apps/docs/analytics/overview.mdx(1 hunks)apps/docs/analytics/query-examples.mdx(1 hunks)apps/docs/analytics/quickstarts.mdx(0 hunks)apps/docs/analytics/schema-reference.mdx(1 hunks)apps/docs/analytics/security.mdx(1 hunks)apps/docs/apis/features/analytics.mdx(3 hunks)apps/docs/docs.json(13 hunks)apps/docs/errors/unkey/application/precondition_failed.mdx(1 hunks)apps/docs/errors/unkey/data/analytics_connection_failed.mdx(1 hunks)apps/docs/errors/unkey/data/analytics_not_configured.mdx(1 hunks)apps/docs/errors/user/bad_request/invalid_analytics_query.mdx(1 hunks)apps/docs/errors/user/bad_request/invalid_function.mdx(1 hunks)apps/docs/errors/user/bad_request/invalid_table.mdx(1 hunks)apps/docs/errors/user/bad_request/query_execution_timeout.mdx(1 hunks)apps/docs/errors/user/bad_request/query_memory_limit_exceeded.mdx(1 hunks)apps/docs/errors/user/bad_request/query_not_supported.mdx(1 hunks)apps/docs/errors/user/bad_request/query_quota_exceeded.mdx(1 hunks)apps/docs/errors/user/bad_request/query_result_rows_limit_exceeded.mdx(1 hunks)apps/docs/errors/user/bad_request/query_rows_limit_exceeded.mdx(1 hunks)apps/docs/package.json(1 hunks)apps/docs/todo-golang-section.mint.json(0 hunks)apps/engineering/content/docs/architecture/services/analytics.mdx(1 hunks)apps/engineering/content/docs/architecture/services/meta.json(1 hunks)go/Makefile(1 hunks)go/apps/api/config.go(1 hunks)go/apps/api/openapi/gen.go(2 hunks)go/apps/api/openapi/generate.go(1 hunks)go/apps/api/openapi/generate_bundle.go(1 hunks)go/apps/api/openapi/openapi-generated.yaml(9 hunks)go/apps/api/openapi/openapi-split.yaml(4 hunks)go/apps/api/openapi/spec/error/ServiceUnavailableErrorResponse.yaml(1 hunks)go/apps/api/openapi/spec/paths/v2/analytics/getVerifications/V2AnalyticsGetVerificationsRequestBody.yaml(1 hunks)go/apps/api/openapi/spec/paths/v2/analytics/getVerifications/V2AnalyticsGetVerificationsResponseBody.yaml(1 hunks)go/apps/api/openapi/spec/paths/v2/analytics/getVerifications/V2AnalyticsGetVerificationsResponseData.yaml(1 hunks)go/apps/api/openapi/spec/paths/v2/analytics/getVerifications/index.yaml(1 hunks)go/apps/api/routes/register.go(2 hunks)go/apps/api/routes/services.go(2 hunks)go/apps/api/routes/v2_analytics_get_verifications/handler.go(1 hunks)go/apps/api/run.go(2 hunks)go/cmd/api/main.go(2 hunks)go/cmd/create-clickhouse-user/main.go(1 hunks)go/cmd/seed-analytics/main.go(1 hunks)go/go.mod(5 hunks)go/internal/services/analytics/connection_manager.go(1 hunks)go/internal/services/caches/caches.go(2 hunks)go/internal/services/caches/op.go(1 hunks)go/main.go(2 hunks)go/pkg/cache/cache.go(4 hunks)go/pkg/cache/clustering/cluster_cache.go(3 hunks)go/pkg/cache/interface.go(1 hunks)go/pkg/cache/many_test.go(1 hunks)go/pkg/cache/middleware/tracing.go(4 hunks)go/pkg/cache/noop.go(2 hunks)go/pkg/clickhouse/client.go(1 hunks)go/pkg/clickhouse/errors.go(1 hunks)go/pkg/clickhouse/interface.go(1 hunks)go/pkg/clickhouse/noop.go(1 hunks)go/pkg/clickhouse/query-parser/README.md(1 hunks)go/pkg/clickhouse/query-parser/errors_test.go(1 hunks)go/pkg/clickhouse/query-parser/limits.go(1 hunks)go/pkg/clickhouse/query-parser/limits_test.go(1 hunks)go/pkg/clickhouse/query-parser/parser.go(1 hunks)go/pkg/clickhouse/query-parser/security_test.go(1 hunks)go/pkg/clickhouse/query-parser/select_columns.go(1 hunks)go/pkg/clickhouse/query-parser/tables.go(1 hunks)go/pkg/clickhouse/query-parser/tables_test.go(1 hunks)go/pkg/clickhouse/query-parser/types.go(1 hunks)go/pkg/clickhouse/query-parser/validation.go(1 hunks)go/pkg/clickhouse/query-parser/validation_test.go(1 hunks)go/pkg/clickhouse/query-parser/virtual_columns.go(1 hunks)go/pkg/clickhouse/query-parser/virtual_columns_test.go(1 hunks)go/pkg/clickhouse/query-parser/workspace.go(1 hunks)go/pkg/clickhouse/query-parser/workspace_test.go(1 hunks)go/pkg/clickhouse/result-transformer/transformer.go(1 hunks)go/pkg/clickhouse/result-transformer/transformer_test.go(1 hunks)go/pkg/codes/constants_gen.go(5 hunks)go/pkg/codes/generate.go(7 hunks)go/pkg/codes/unkey_data.go(3 hunks)go/pkg/codes/user_request.go(2 hunks)go/pkg/db/api_find_key_auth_by_ids.sql_generated.go(1 hunks)go/pkg/db/api_find_key_auth_by_key_auth_ids.sql_generated.go(1 hunks)go/pkg/db/bulk_clickhouse_workspace_settings_insert.sql_generated.go(1 hunks)go/pkg/db/bulk_key_auth_insert.sql_generated.go(1 hunks)go/pkg/db/clickhouse_workspace_settings_find_by_workspace_id.sql_generated.go(1 hunks)go/pkg/db/clickhouse_workspace_settings_insert.sql_generated.go(1 hunks)go/pkg/db/clickhouse_workspace_settings_update_limits.sql_generated.go(1 hunks)go/pkg/db/generate.go(1 hunks)go/pkg/db/identity_find_identities_by_ids.sql_generated.go(1 hunks)go/pkg/db/identity_find_many.sql_generated.go(1 hunks)go/pkg/db/key_auth_get_by_id.sql_generated.go(1 hunks)go/pkg/db/key_auth_insert.sql_generated.go(1 hunks)go/pkg/db/models_generated.go(1 hunks)go/pkg/db/plugins/bulk-insert/bulk_insert.go.tmpl(1 hunks)go/pkg/db/querier_bulk_generated.go(1 hunks)go/pkg/db/querier_generated.go(7 hunks)go/pkg/db/queries/api_find_key_auth_by_ids.sql(1 hunks)go/pkg/db/queries/api_find_key_auth_by_key_auth_ids.sql(1 hunks)go/pkg/db/queries/clickhouse_workspace_settings_find_by_workspace_id.sql(1 hunks)go/pkg/db/queries/clickhouse_workspace_settings_insert.sql(1 hunks)go/pkg/db/queries/clickhouse_workspace_settings_update_limits.sql(1 hunks)go/pkg/db/queries/identity_find_many.sql(1 hunks)go/pkg/db/queries/key_auth_get_by_id.sql(1 hunks)go/pkg/db/queries/key_auth_insert.sql(1 hunks)go/pkg/db/schema.sql(1 hunks)
⛔ Files not processed due to max files limit (15)
- go/pkg/eventstream/producer.go
- go/pkg/hydra/store/generate.go
- go/pkg/rbac/permissions.go
- go/pkg/zen/middleware_errors.go
- go/tools.go
- internal/db/drizzle/0000_pale_dark_phoenix.sql
- internal/db/drizzle/0001_workable_wildside.sql
- internal/db/drizzle/meta/0000_snapshot.json
- internal/db/drizzle/meta/0001_snapshot.json
- internal/db/drizzle/meta/_journal.json
- internal/db/package.json
- internal/db/src/schema/clickhouse_workspace_settings.ts
- internal/db/src/schema/index.ts
- internal/db/src/schema/workspaces.ts
- packages/rbac/src/permissions.ts
💤 Files with no reviewable changes (2)
- apps/docs/analytics/quickstarts.mdx
- apps/docs/todo-golang-section.mint.json
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-01T02:33:43.791Z
Learnt from: imeyer
PR: unkeyed/unkey#3899
File: go/buf.gen.yaml:0-0
Timestamp: 2025-09-01T02:33:43.791Z
Learning: In the unkeyed/unkey repository, buf commands are executed from the `go/` directory where the `buf.yaml` file is located. This means the `out: gen` configuration in `go/buf.gen.yaml` generates files to `go/gen/` relative to the repository root, which aligns with the expected directory structure.
Applied to files:
go/Makefile
📚 Learning: 2025-09-01T01:57:42.227Z
Learnt from: imeyer
PR: unkeyed/unkey#3899
File: go/proto/metald/v1/metald.proto:5-9
Timestamp: 2025-09-01T01:57:42.227Z
Learning: In the unkeyed/unkey repository, buf is configured to properly resolve metald proto imports like "metald/v1/vm.proto" without needing the full "go/proto/" prefix. The buf lint command `buf lint --path proto/metald` passes successfully with these relative import paths.
Applied to files:
go/Makefilego/go.mod
📚 Learning: 2025-07-17T14:24:20.403Z
Learnt from: Flo4604
PR: unkeyed/unkey#3631
File: go/pkg/db/bulk_keyring_insert.sql.go:23-25
Timestamp: 2025-07-17T14:24:20.403Z
Learning: In go/pkg/db/bulk_keyring_insert.sql.go and similar bulk insert generated files, hardcoded zero values for fields like size_approx and size_last_updated_at are intentional and reflect the original SQL query structure, not missing parameters.
Applied to files:
go/pkg/db/bulk_key_auth_insert.sql_generated.go
📚 Learning: 2025-08-08T15:20:40.288Z
Learnt from: Flo4604
PR: unkeyed/unkey#3753
File: go/apps/api/openapi/config.yaml:9-10
Timestamp: 2025-08-08T15:20:40.288Z
Learning: Repo unkeyed/unkey: oapi-codegen v2.4+ (v2.5.0 in use) supports output-options.overlay in go/apps/api/openapi/config.yaml; the generator applies overlay.yaml at codegen time, so no separate pre-step is required if oapi-codegen is invoked with -config=config.yaml.
Applied to files:
go/apps/api/openapi/generate.go
🔇 Additional comments (77)
go/pkg/db/generate.go (1)
4-4: Approve reproducible SQLC code generationSwitching from a PATH-dependent
sqlcbinary togo run github.com/sqlc-dev/sqlc/cmd/sqlc generateensures all developers and CI use the pinned v1.29.0 version declared ingo/go.mod, eliminating version mismatch issues.go/apps/api/openapi/generate_bundle.go (1)
60-62: Confirm BuildV3Model signature matches single-error handling
go/go.mod pins github.com/pb33f/libopenapi v0.28.0; ensure BuildV3Model now returns (Model, error) instead of ([]error) so you don’t lose aggregated error details.go/go.mod (1)
36-36: Update verification script to locate and validate go/go.mod and sqlc installation#!/bin/bash set -e # Locate the Go module file go_mod=$(find . -type f -path './go/go.mod' | head -n1) if [[ -z "$go_mod" ]]; then echo "ERROR: go/go.mod not found" exit 1 fi echo "Inspecting $go_mod" # Verify tool directive for sqlc grep -nE '^tool\s+github.com/sqlc-dev/sqlc/cmd/sqlc' "$go_mod" # Verify pinned sqlc version grep -n 'github.com/sqlc-dev/sqlc v1\.29\.0' "$go_mod" # Report local tool versions echo "Go version: $(go version)" echo "sqlc version: $(sqlc version || echo 'sqlc not in PATH')"Run this in your CI/dev environment to confirm:
- Go toolchain ≥ the version required by the
tooldirectivesqlc v1.29.0is correctly installed or resolved viago toolgo/pkg/db/plugins/bulk-insert/bulk_insert.go.tmpl (1)
53-53: LGTM! Cosmetic indentation change only.This is a pure whitespace/indentation adjustment with no functional impact.
go/pkg/db/queries/api_find_key_auth_by_key_auth_ids.sql (1)
1-8: LGTM! Well-structured query with proper workspace isolation.The query correctly:
- Enforces workspace-level data isolation via
workspace_idfilter- Respects soft-delete semantics by checking
deleted_at_m IS NULLon both tables- Uses parameterized queries (sqlc.arg, sqlc.slice) to prevent SQL injection
- Employs clear aliasing for readability
go/apps/api/openapi/generate.go (1)
4-4: LGTM! Modernized code generation invocation.Switching from
go tool oapi-codegentogo run github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegenensures the exact versioned binary from go.mod is used, providing better reproducibility and consistency.Based on learnings.
go/main.go (1)
8-8: LGTM! CLI command registration follows established patterns.The new commands for ClickHouse user creation and analytics seeding are registered consistently with existing CLI commands.
Also applies to: 14-14, 33-34
apps/engineering/content/docs/architecture/services/meta.json (1)
7-7: LGTM! Documentation navigation updated for analytics.The new "analytics" page entry is properly positioned and aligns with the analytics documentation expansion throughout the PR.
apps/docs/package.json (2)
6-6: LGTM! Dev script now uses local Mintlify installation.Switching from
npx mintlify@latest devtomintlify dev --local-schemaprovides:
- Deterministic builds using the pinned version from devDependencies
- Local schema validation via the
--local-schemaflag
12-12: Clarify Mintlify npm package name: confirm whether the dependency in apps/docs/package.json is “mintlify” or “mint” so we can review the v4.2.31→v4.2.144 changelog for breaking changes.go/apps/api/openapi/gen.go (1)
504-530: Handler enforces documented SQL security controls
The handler’s use of chquery.NewParser with WorkspaceID, AllowedTables, TableAliases, VirtualColumns, and Limit ensures workspace_id injection, table alias mapping, SELECT-only queries, result-size limits, and dangerous-statement blocking. Subsequent auth.VerifyRootKey enforces both wildcard and per-API ReadAnalytics permissions.go/pkg/clickhouse/interface.go (1)
34-49: Verify implementers and mocks updated
- Adding QueryToMaps, Exec, GetBillableVerifications and GetBillableRatelimits to Querier is a breaking change; update all existing implementations and tests accordingly.
- If backward compatibility is needed, introduce a new extended interface (e.g., DynamicQuerier embedding Querier) and migrate clients progressively.
go/pkg/db/queries/key_auth_get_by_id.sql (1)
1-11: LGTM!The SQL query is well-structured with proper soft-delete filtering (
deleted_at_m IS NULL) and uses parameterized queries to prevent SQL injection. The:oneplan correctly matches the single-row retrieval pattern.apps/docs/errors/user/bad_request/query_memory_limit_exceeded.mdx (1)
1-7: LGTM!The error documentation is clear and follows the established pattern for user-facing error references. The description accurately conveys the error condition.
apps/docs/errors/user/bad_request/invalid_function.mdx (1)
1-7: LGTM!The error documentation clearly describes the invalid function error condition and follows the established documentation pattern.
apps/dashboard/app/(app)/[workspaceSlug]/settings/root-keys/[keyId]/permissions/permissions.ts (1)
205-210: LGTM!The new Analytics permission category properly extends the per-API permission surface, following the established naming and structure patterns. The permission scope (
api.${apiId}.read_analytics) correctly restricts access to the specific API.go/apps/api/routes/services.go (2)
4-4: LGTM!The analytics package import is correctly added to support the new analytics connection manager.
29-29: LGTM!The AnalyticsConnectionManager field extends the Services struct appropriately. The pointer type is correct for the manager pattern, enabling per-workspace connection pooling.
apps/docs/errors/user/bad_request/invalid_analytics_query.mdx (1)
1-7: LGTM!The error documentation accurately describes the invalid analytics query error condition and follows the established documentation pattern for user-facing error references.
go/apps/api/routes/register.go (2)
55-55: LGTM!The import for the new analytics route handler is correctly placed.
544-558: LGTM!The analytics route registration follows the established pattern for v2 endpoints, with appropriate middleware and all necessary service dependencies. The handler receives the AnalyticsConnectionManager needed for workspace-scoped analytics queries.
go/internal/services/caches/op.go (1)
10-18: Reorder is safe:db.IsNotFound(nil)returns false Reordering theIsNotFoundanderr == nilchecks preserves the original behavior.go/pkg/codes/unkey_data.go (2)
76-83: LGTM!The
dataAnalyticsstruct follows the established pattern for error categories in this file. The error codes are well-named and appropriately documented.
99-99: LGTM!The
Analyticsfield addition and initialization are consistent with the existing error category pattern in this file.Also applies to: 150-153
apps/docs/errors/user/bad_request/query_not_supported.mdx (1)
1-6: LGTM!The error documentation is clear and follows the standard format for error pages.
go/pkg/db/models_generated.go (1)
569-582: LGTM!The
ClickhouseWorkspaceSettingstruct is properly generated and follows the pattern of other models in this file. The field types and database tags are appropriate for the analytics workspace settings.go/pkg/db/queries/clickhouse_workspace_settings_insert.sql (1)
1-29: LGTM!The INSERT query is well-structured and follows sqlc conventions. All columns have corresponding parameter bindings.
apps/docs/apis/features/analytics.mdx (3)
8-14: LGTM!The warning banner clearly communicates the private beta status and provides a link to learn more.
34-51: LGTM!The Advanced Analytics section provides clear navigation to related documentation with appropriate icons and descriptions.
105-105: LGTM!The updated tags documentation appropriately references the new SQL-based analytics capability with a helpful link to examples.
apps/docs/analytics/getting-started.mdx (4)
1-37: LGTM!The Request Access and Authentication sections provide clear, step-by-step instructions with appropriate security warnings.
39-78: LGTM!The SQL query examples are practical and demonstrate common analytics patterns effectively.
80-122: LGTM!The sections on response format, time handling, and filtering provide clear guidance with practical examples.
124-164: LGTM!The resource limits table is comprehensive, and the error handling guidance is clear. The Next Steps section provides good navigation to related documentation.
apps/dashboard/app/(app)/[workspaceSlug]/settings/root-keys/components/root-key/permissions.ts (1)
210-215: LGTM!The API-level analytics permission is properly structured with a clear description and correct permission format.
go/pkg/clickhouse/query-parser/workspace.go (2)
19-30: LGTM!The WHERE clause injection logic correctly handles both cases:
- Creates a new WHERE clause when none exists
- ANDs the workspace filter with existing conditions, ensuring the workspace filter is always applied
This properly enforces workspace isolation in SQL queries.
7-17: Verify StringLiteral escaping for SQL injection
I couldn’t locate theStringLiteralimplementation in the repo—please confirm it properly escapes special characters inp.config.WorkspaceIDto prevent SQL injection.go/pkg/clickhouse/query-parser/limits_test.go (1)
11-25: LGTM!The test properly validates that the parser enforces the configured limit (100) when a query specifies a higher limit (1000), and also verifies that the workspace ID is injected into the query.
go/pkg/codes/user_request.go (1)
13-30: LGTM!The new analytics-related error codes are well-structured and consistently follow the existing pattern. The documentation clearly explains each error condition, and the naming is intuitive.
Also applies to: 45-57
go/cmd/api/main.go (2)
47-48: LGTM!The CLI flag definition is clear and follows the existing pattern for DSN configuration. The description properly documents the placeholder format, and making it optional is appropriate for a new feature.
134-135: LGTM!The config binding correctly wires the CLI flag value to the API configuration field. The placement next to
ClickhouseURLis logical and maintains good organization.go/apps/api/openapi/openapi-split.yaml (4)
85-85: Verify if localhost server URL should be in the OpenAPI spec.The addition of
http://127.0.0.1:7070as a server URL is unusual for a production OpenAPI specification. While this may be intentional for local development and testing during the WIP phase, it typically should not be included in the final production spec as it can lead to issues with generated clients and API documentation.Please confirm whether:
- This should remain in the spec for local development purposes
- This should be removed before the PR is merged
- This should be conditionally included based on environment
If this is meant for development only, consider using OpenAPI's server variables or maintaining separate spec files for development and production.
102-103: LGTM!The analytics tag is well-defined with a clear description and follows the existing pattern for other API tags.
122-124: LGTM!The analytics endpoint path follows the established V2 API naming convention and correctly references the external specification file. The placement and structure are consistent with other endpoints.
218-220: LGTM!The ServiceUnavailableErrorResponse schema addition follows the established pattern for error response schemas and correctly references the external specification file.
go/pkg/db/queries/key_auth_insert.sql (1)
1-16: Verify the hardcodedstore_encrypted_keysvalue.The
store_encrypted_keyscolumn is hardcoded tofalserather than being parameterized. This means all key_auth entries created through this query will have encrypted key storage disabled.Please confirm this is the intended behavior. If encrypted key storage should be configurable per key_auth entry, consider parameterizing this value:
-- name: InsertKeyAuth :exec INSERT INTO key_auth ( id, workspace_id, created_at_m, default_prefix, default_bytes, store_encrypted_keys ) VALUES ( ?, ?, ?, ?, ?, - false + ? );If
falseis always the correct value for this use case, consider adding a comment explaining why encrypted key storage is disabled for these entries.go/apps/api/openapi/spec/error/ServiceUnavailableErrorResponse.yaml (1)
1-17: LGTM!The ServiceUnavailableErrorResponse schema is well-defined with a comprehensive description that provides clear guidance for API consumers. The structure follows the established pattern for error responses, and the references to common schema components are appropriate.
go/pkg/clickhouse/query-parser/tables_test.go (3)
10-25: LGTM!The test properly validates that table aliases are correctly resolved and that the aliased table is used in the parsed query output. The test configuration correctly includes both the alias mapping and the allowed tables list.
27-35: LGTM!The test correctly validates that system tables are blocked, which is an important security feature. The error message validation ensures the rejection reason is clear to users.
37-53: LGTM!The test comprehensively validates the allowed tables enforcement by testing both positive (allowed table) and negative (disallowed table) scenarios. The dual assertions provide good coverage of this security feature.
go/pkg/db/queries/identity_find_many.sql (1)
1-6: Index coverage confirmed.
The primary key onidand the unique index on(workspace_id, external_id, deleted)satisfy the OR‐clause filters.go/pkg/db/querier_bulk_generated.go (1)
14-14: LGTM!The new bulk insert methods follow the existing pattern and integrate cleanly with the BulkQuerier interface.
Also applies to: 20-20
go/apps/api/openapi/spec/paths/v2/analytics/getVerifications/V2AnalyticsGetVerificationsResponseBody.yaml (1)
1-9: LGTM!The response schema follows OpenAPI best practices with clear required fields and proper references to Meta and response data schemas.
go/pkg/cache/noop.go (1)
14-21: LGTM!The batch operation implementations correctly follow the no-op pattern, returning empty maps and
Misscache hits as expected. The implementations are consistent with the existing single-key no-op methods.Also applies to: 25-25, 29-29, 50-57
apps/docs/analytics/query-examples.mdx (2)
1-834: Comprehensive documentation with clear examples.The query examples are well-organized, covering diverse analytics scenarios. The dual format (readable multi-line SQL and JSON for API requests) is helpful for users. The examples demonstrate proper use of time filters, aggregations, and ClickHouse-specific functions.
552-567: Alias usage is valid in ClickHouse
ClickHouse allows referencing column aliases within the same SELECT clause, so the existing example is correct.go/pkg/clickhouse/query-parser/virtual_columns_test.go (1)
11-33: LGTM!The tests thoroughly cover virtual column rewriting scenarios including GROUP BY clauses and various SELECT patterns with different alias handling. The test structure is clear and uses appropriate assertions.
Also applies to: 35-79
go/apps/api/openapi/spec/paths/v2/analytics/getVerifications/V2AnalyticsGetVerificationsRequestBody.yaml (1)
7-16: SELECT-only enforcement confirmed. The parser (parser.go) rejects all non-SELECT statements and existing tests cover this behavior.go/apps/api/run.go (1)
265-279: Nil handling for AnalyticsConnectionManager confirmed: v2_analytics_get_verifications guards on nil and returns a NotConfigured error; no other routes invoke it unguarded.go/pkg/db/queries/clickhouse_workspace_settings_update_limits.sql (1)
1-12: updated_at is correctly populated by caller. Verified thatUpdatedAtis set usingsql.NullInt64{Valid: true, Int64: now}in the caller; no further action needed.go/pkg/db/key_auth_get_by_id.sql_generated.go (1)
1-59: LGTM! Well-structured generated query.The sqlc-generated query correctly handles nullable fields and follows standard patterns for database queries.
go/pkg/db/bulk_key_auth_insert.sql_generated.go (1)
15-42: LGTM! Bulk insert implementation is correct.The hardcoded
falsevalue forstore_encrypted_keyson Line 24 is intentional and aligns with the SQL query structure. The 5 collected arguments match the 5 placeholders in the value clause.Based on learnings
apps/engineering/content/docs/architecture/services/analytics.mdx (1)
1-526: Excellent comprehensive security documentation.This document provides thorough coverage of the Analytics API security architecture, including query parsing, RBAC, ClickHouse-level security, and operational guidance. The examples and explanations are clear and actionable.
go/pkg/clickhouse/query-parser/tables.go (1)
1-79: LGTM! Robust table validation and rewriting.The implementation correctly:
- Validates FROM clause presence
- Resolves table aliases
- Blocks system and information_schema tables
- Handles both qualified (database.table) and unqualified table names
- Provides clear error messages with structured fault codes
apps/docs/analytics/schema-reference.mdx (1)
302-302: Verify result rows limit consistency.Line 302 states the result rows limit is "10 million", but Line 237 in the architecture doc and other parts of the documentation indicate the limit is "10,000 rows". Please verify which limit is correct and update for consistency.
go/pkg/db/identity_find_many.sql_generated.go (1)
34-54: LGTM! Correct handling of dual IN clauses.The seemingly duplicate code blocks (lines 39-46 and 47-54) are intentional. The SQL query on Line 18 contains two
INclauses:
external_id IN(/*SLICE:identities*/?)id IN (/*SLICE:identities*/?)Each requires independent slice expansion, so the parameters are correctly appended twice.
go/pkg/db/api_find_key_auth_by_ids.sql_generated.go (1)
1-74: LGTM! Standard sqlc-generated join query.The query correctly joins
apisandkey_authtables, handles slice expansion for theINclause, and properly manages database resources.go/pkg/db/bulk_clickhouse_workspace_settings_insert.sql_generated.go (1)
15-49: LGTM! Correct bulk insert implementation.The value clause on Line 24 has 12 placeholders that match the 12 arguments collected from Lines 32-43. All fields from
InsertClickhouseWorkspaceSettingsParamsare properly handled.go/pkg/clickhouse/query-parser/types.go (1)
1-41: LGTM!The type definitions are well-designed and form a cohesive API for the query parser. The VirtualColumnResolver function type appropriately uses context for cancellation/timeouts, the Config struct centralizes all parser configuration, and the Parser struct properly encapsulates internal state for alias resolution and column mapping.
go/pkg/clickhouse/query-parser/validation_test.go (2)
28-43: LGTM!The safe functions whitelist is appropriate for analytics queries, including standard aggregates (count, sum, avg, max, min) and date functions (now, toDate).
18-25: Function blocking is case-insensitive. validation.go lowercases function names before matching, so mixed-case variants (e.g. FILE(), File()) are already blocked.go/apps/api/openapi/spec/paths/v2/analytics/getVerifications/V2AnalyticsGetVerificationsResponseData.yaml (1)
1-18: LGTM!The use of
additionalProperties: truefor verification row objects is appropriate here, as the fields returned depend on the SQL SELECT clause. The example provides clear documentation of typical query results.go/pkg/clickhouse/query-parser/parser.go (2)
13-19: LGTM!The constructor is straightforward and properly initializes the Parser with the provided config and an empty columnMappings map.
54-78: Verify rewrite step ordering dependencies.The sequential execution of rewrite steps (rewriteSelectColumns, rewriteVirtualColumns, rewriteTables, injectWorkspaceFilter, enforceLimit, validateFunctions) suggests potential ordering dependencies. Ensure that:
- The order is intentional and documented (e.g., virtual columns must be rewritten before table rewriting)
- Steps don't have circular dependencies
- Each step can handle the output of previous steps
Consider adding a comment documenting the required step ordering and why it matters, e.g.:
// Rewrite steps are executed in a specific order: // 1. rewriteSelectColumns - expands virtual column references // 2. rewriteVirtualColumns - resolves virtual IDs to actual IDs // 3. rewriteTables - applies table aliases and validates access // 4. injectWorkspaceFilter - adds workspace isolation // 5. enforceLimit - prevents unbounded queries // 6. validateFunctions - blocks dangerous operationsgo/pkg/db/schema.sql (1)
310-325: LGTM with observation!The schema is well-designed with appropriate constraints and reasonable defaults. The
password_encryptedfield correctly usestexttype for encrypted credentials. Default values are suitable for analytics workloads (e.g., 1-hour quota windows, 1GB memory limits).Note: The
UNIQUE KEY (username)constraint will typically auto-create an index, but verify this for your MySQL/database version if username lookups are frequent.go/pkg/cache/clustering/cluster_cache.go (1)
106-172: LGTM!The multi-key operations are well-implemented with an efficient batch invalidation pattern. Key strengths:
- Efficient broadcasting: SetMany and SetNullMany collect all keys and broadcast a single invalidation event for the entire batch, reducing network overhead.
- Consistent pattern: All methods follow the same delegate-to-local-cache approach as their single-key counterparts.
- Proper integration: SWRMany correctly delegates batch stale-while-revalidate to the local cache implementation.
go/apps/api/routes/v2_analytics_get_verifications/handler.go (1)
61-66: Fix defer ordering to avoid nil panic
emitis deferred before checkingerr. IfGetRootKeyfails and returns a nilemit, the deferred call panics. Move thedefer emit()below the error check.- auth, emit, err := h.Keys.GetRootKey(ctx, s) - defer emit() - if err != nil { - return err - } + auth, emit, err := h.Keys.GetRootKey(ctx, s) + if err != nil { + return err + } + defer emit()⛔ Skipped due to learnings
Learnt from: Flo4604 PR: unkeyed/unkey#3841 File: go/apps/api/routes/v2_keys_migrate_keys/handler.go:60-65 Timestamp: 2025-08-27T14:08:31.731Z Learning: In the Unkey Go codebase, service methods like GetRootKey that return an emit function alongside potential errors are designed to always return a safe no-op emit function, even when the main operation fails. This makes it safe to defer emit() immediately after the call, before checking the error.Learnt from: Flo4604 PR: unkeyed/unkey#3841 File: go/apps/api/routes/v2_keys_migrate_keys/handler.go:60-65 Timestamp: 2025-08-27T14:08:31.731Z Learning: In the Unkey Go codebase, service methods like GetRootKey that return an emit function alongside potential errors are designed to always return a safe no-op emit function (emptyLog), even when the main operation fails. This makes it safe to defer emit() immediately after the call, before checking the error. This pattern is consistently used across all API route handlers.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@chronark I went over the docs and they should be clearer now But lets maybe go over this when you are back from the holidays with a proper review |
|
There was a problem hiding this comment.
It's worth to mention that queries supports CH functions and other utils of CH. I didn't see that so that might come in handy for users.
There was a problem hiding this comment.
analytics/schema-reference check that path
do you mean them? or where would you want to mention it instead?
| SELECT COUNT(*) as total | ||
| FROM key_verifications_v1 | ||
| WHERE time >= now() - INTERVAL 7 DAY |
There was a problem hiding this comment.
For longer queries converting SQL to single line might take some time. Maybe we can give them a link or some simple way to quickly turn SQL into single line. I'd love to have that DX as a customer
There was a problem hiding this comment.
wouldn't they use it programmatically and their language of choice can merge it into a single line?
the multiline thing is only really an issue with curl
There was a problem hiding this comment.
yeah and we have a curl below where it is only a single line so it should be fine?
| ### Root Key Best Practices | ||
|
|
||
| 1. **Use environment variables** - Never hardcode root keys | ||
| 2. **Rotate keys regularly** - Create new keys and revoke old ones |
There was a problem hiding this comment.
Why should they do this if the key is not exposed to the public? Maybe we should work on the wording?
There was a problem hiding this comment.
you assume that you know when a key is leaked
but what if you don't?
it's good to rotate them regularly
| TableAliases: map[string]string{ | ||
| "key_verifications_v1": "default.key_verifications_raw_v2", | ||
| "key_verifications_per_minute_v1": "default.key_verifications_per_minute_v2", | ||
| "key_verifications_per_hour_v1": "default.key_verifications_per_hour_v2", | ||
| "key_verifications_per_day_v1": "default.key_verifications_per_day_v2", | ||
| "key_verifications_per_month_v1": "default.key_verifications_per_month_v2", | ||
| }, | ||
| AllowedTables: []string{ | ||
| "default.key_verifications_raw_v2", | ||
| "default.key_verifications_per_minute_v2", | ||
| "default.key_verifications_per_hour_v2", | ||
| "default.key_verifications_per_day_v2", | ||
| "default.key_verifications_per_month_v2", | ||
| }, |
There was a problem hiding this comment.
nit: I think we can move them to top for a cleaner look.
| if len(keySpaceIds) > 0 { | ||
| keySpaces, keySpaceHits, err := h.Caches.KeyAuthToApiRow.SWRMany( | ||
| ctx, | ||
| array.Map(keySpaceIds, func(keySpaceId string) cache.ScopedKey { | ||
| return cache.ScopedKey{WorkspaceID: auth.AuthorizedWorkspaceID, Key: keySpaceId} | ||
| }), | ||
| func(ctx context.Context, keys []cache.ScopedKey) (map[cache.ScopedKey]db.FindKeyAuthsByKeyAuthIdsRow, error) { | ||
| keySpaces, err := db.Query.FindKeyAuthsByKeyAuthIds(ctx, h.DB.RO(), db.FindKeyAuthsByKeyAuthIdsParams{ | ||
| WorkspaceID: auth.AuthorizedWorkspaceID, | ||
| KeyAuthIds: array.Map(keys, func(keySpace cache.ScopedKey) string { | ||
| return keySpace.Key | ||
| }), | ||
| }) | ||
|
|
||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return array.Reduce( | ||
| keySpaces, | ||
| func(acc map[cache.ScopedKey]db.FindKeyAuthsByKeyAuthIdsRow, api db.FindKeyAuthsByKeyAuthIdsRow) map[cache.ScopedKey]db.FindKeyAuthsByKeyAuthIdsRow { | ||
| acc[cache.ScopedKey{WorkspaceID: auth.AuthorizedWorkspaceID, Key: api.KeyAuthID}] = api | ||
| return acc | ||
| }, | ||
| map[cache.ScopedKey]db.FindKeyAuthsByKeyAuthIdsRow{}, | ||
| ), nil | ||
| }, | ||
| caches.DefaultFindFirstOp, | ||
| ) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Nit but not nit 😄 . This block is quite hard to digest and read. I think we should definitely break that into smaller chunks I can't even tell where the block begins and ends. There is also one more above.
| clickhouse "github.com/AfterShip/clickhouse-sql-parser/parser" | ||
| ) | ||
|
|
||
| // buildCTERegistry scans the WITH clause and registers all CTE names |
There was a problem hiding this comment.
Can we add a comment why we need that method? It's hard to understand it without going through the code
| // SecurityFilter represents a row-level security constraint | ||
| type SecurityFilter struct { | ||
| Column string // Column name | ||
| AllowedValues []string // Values user is allowed to access |
There was a problem hiding this comment.
nit: I know we have them in the docs, but having another copy here might be better.
|
Thank you for following the naming conventions for pull request titles! 🙏 |
What does this PR do?
This does too many things, but mainly:
Extends our cache package to Many Operations (SWRMany GetMany SetMany) and so on, also adds a tests for this
Clickhouse user "management"
Seed analytics data but in go
2 New permissions for reading analytics api.*.read_analytics and api.<api_id>.read_analytics.
Error docs + analytics docs
New endpoint that lets the user query clickhouse with his user and with that
Adds a clickhouse query parser which does things
Regenerated the sql schema for our ts client
Regenerated our protobuf generation stuff
Fixes #3562
Type of change
How should this be tested?
(edited by @chronark)
make upThen,
pnpm localto trigger the build and generate new permissionsYou can use your favorite curl/httpie/gui and call the endpoint
So please go trough the docs and run the queries from there or write your owns and test if it works
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated