feat: return errors as values for clickhouse #2665
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,7 +34,7 @@ export const VerificationTable = ({ verifications }: Props) => { | |
| </TableRow> | ||
| </TableHeader> | ||
| <TableBody className={"font-mono"}> | ||
| {verifications.val?.map((verification, i) => { | ||
| /** | ||
| * Instead of rounding every row individually, we want to round consecutive colored rows together. | ||
| * For example: | ||
|
|
@@ -54,10 +54,10 @@ export const VerificationTable = ({ verifications }: Props) => { | |
| */ | ||
| const isStartOfColoredBlock = | ||
| verification.outcome !== "VALID" && | ||
| (i === 0 || verifications.val[i - 1].outcome === "VALID"); | ||
| const isEndOfColoredBlock = | ||
| verification.outcome !== "VALID" && | ||
| (i === verifications.val.length - 1 || verifications.val[i + 1].outcome === "VALID"); | ||
|
|
||
| return ( | ||
| <TableRow | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -54,22 +54,22 @@ export default async function ApiPage(props: { | |||||||||
| .where(and(eq(schema.keys.keyAuthId, api.keyAuthId!), isNull(schema.keys.deletedAt))) | ||||||||||
| .execute() | ||||||||||
| .then((res) => res.at(0)?.count ?? 0), | ||||||||||
| getVerificationsPerInterval(query).then((res) => res.val!), | ||||||||||
| getActiveKeysPerInterval(query).then((res) => res.val!), | ||||||||||
|
Comment on lines
+57
to
+58
Contributor
There was a problem hiding this comment. 🛠️ Refactor suggestion Consider handling potential null values instead of using non-null assertions The non-null assertions ( Consider this safer approach: - getVerificationsPerInterval(query).then((res) => res.val!),
- getActiveKeysPerInterval(query).then((res) => res.val!),
+ getVerificationsPerInterval(query).then((res) => res.val ?? []),
+ getActiveKeysPerInterval(query).then((res) => res.val ?? []),📝 Committable suggestion
Suggested change
|
||||||||||
| clickhouse.activeKeys | ||||||||||
| .perMonth({ | ||||||||||
| workspaceId: api.workspaceId, | ||||||||||
| keySpaceId: api.keyAuthId!, | ||||||||||
| start: billingCycleStart, | ||||||||||
| end: billingCycleEnd, | ||||||||||
| }) | ||||||||||
| .then((res) => res.val!.at(0)), | ||||||||||
|
Contributor
There was a problem hiding this comment. 🛠️ Refactor suggestion Handle potential null values in the chain of operations The chain of operations Consider this safer approach: - .then((res) => res.val!.at(0)),
+ .then((res) => res.val?.at(0) ?? null),📝 Committable suggestion
Suggested change
|
||||||||||
| getVerificationsPerInterval({ | ||||||||||
| workspaceId: api.workspaceId, | ||||||||||
| keySpaceId: api.keyAuthId!, | ||||||||||
| start: billingCycleStart, | ||||||||||
| end: billingCycleEnd, | ||||||||||
| }).then((res) => res.val!), | ||||||||||
|
Contributor
There was a problem hiding this comment. 💡 Codebase verification 🛠️ Refactor suggestion Based on the gathered context, I can now generate the final response: Consider safer error handling for verification data The non-null assertion on verification data is used in billing calculations where - }).then((res) => res.val!),
+ }).then((res) => res.val ?? []),
🔗 Analysis chainEnsure safe handling of verification data for billing calculations The non-null assertion on verification data could affect billing calculations if the query fails or returns no data. Consider this safer approach: - }).then((res) => res.val!),
+ }).then((res) => res.val ?? []),Let's verify the usage of this data in calculations: Let me gather more context about the verification data structure and its usage. Let me fix the regex and try again. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for places where verification data is used in calculations
rg -A 5 "verificationsInBillingCycle" apps/dashboard/
Length of output: 1147 Script: #!/bin/bash
# Check the type definition and structure of verification data
ast-grep --pattern 'type $_ = { count: number $$$}'
# Look for error handling patterns around verification data
rg -B 3 -A 3 "\.val\!" apps/dashboard/
Length of output: 173 Script: #!/bin/bash
# Check the type definition and structure of verification data
ast-grep --pattern 'type $_ = { count: number }'
# Look for error handling patterns around verification data
rg -B 3 -A 3 '\.val!' apps/dashboard/
# Get more context around the verification data usage
rg -B 5 -A 5 'reduce\(.+sum \+ day\.count' apps/dashboard/
Length of output: 8560 |
||||||||||
| ]); | ||||||||||
|
|
||||||||||
| const successOverTime: { x: string; y: number }[] = []; | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -168,7 +168,7 @@ const LastUsed: React.FC<{ workspaceId: string; keySpaceId: string; keyId: strin | |
| keySpaceId: props.keySpaceId, | ||
| keyId: props.keyId, | ||
| }) | ||
| .then((res) => res.val?.at(0)?.time ?? null); | ||
|
|
||
| return ( | ||
| <TableCell> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -122,10 +122,7 @@ const AuditLogTable: React.FC<{ | |
|
|
||
| success: selected.success ?? undefined, | ||
| }; | ||
| const logs = await clickhouse.ratelimits.logs(query).then((res) => res.val!); | ||
|
|
||
| if (logs.length === 0) { | ||
| return ( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -92,7 +92,7 @@ const LastUsed: React.FC<{ | |
| identifier: [identifier], | ||
| }); | ||
|
|
||
| const unixMilli = lastUsed.val?.at(0)?.time; | ||
| if (unixMilli) { | ||
| return <span className="text-sm text-content-subtle">{ms(Date.now() - unixMilli)} ago</span>; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -68,17 +68,17 @@ export default async function RatelimitNamespacePage(props: { | |
| .from(schema.ratelimitOverrides) | ||
| .where(eq(schema.ratelimitOverrides.namespaceId, namespace.id)) | ||
| .execute() | ||
| .then((res) => res?.at(0)?.count ?? 0), | ||
| getRatelimitsPerInterval(query).then((res) => res.val!), | ||
|
Contributor
There was a problem hiding this comment. Avoid non-null assertions for Clickhouse results Using - getRatelimitsPerInterval(query).then((res) => res.val!),
+ getRatelimitsPerInterval(query).then((res) => res.val ?? { time: Date.now(), passed: 0, total: 0 }),This ensures we have a safe fallback when the Clickhouse query fails or returns no data. Also applies to: 78-78 |
||
| getRatelimitsPerInterval({ | ||
| workspaceId: namespace.workspaceId, | ||
| namespaceId: namespace.id, | ||
| start: billingCycleStart, | ||
| end: billingCycleEnd, | ||
| }).then((res) => res.val!), | ||
| clickhouse.ratelimits | ||
| .latest({ workspaceId: namespace.workspaceId, namespaceId: namespace.id }) | ||
| .then((res) => res.val?.at(0)?.time), | ||
| ]); | ||
|
|
||
| const passedOverTime: { x: string; y: number }[] = []; | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -17,15 +17,17 @@ export const RatelimitCard: React.FC<Props> = async ({ workspace, namespace }) = | |||||||||||||||||||||||||||||||||||||||||||||
| const intervalMs = 1000 * 60 * 60; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| const [history, lastUsed] = await Promise.all([ | ||||||||||||||||||||||||||||||||||||||||||||||
| clickhouse.ratelimits | ||||||||||||||||||||||||||||||||||||||||||||||
| .perMinute({ | ||||||||||||||||||||||||||||||||||||||||||||||
| workspaceId: workspace.id, | ||||||||||||||||||||||||||||||||||||||||||||||
| namespaceId: namespace.id, | ||||||||||||||||||||||||||||||||||||||||||||||
| start: end - intervalMs, | ||||||||||||||||||||||||||||||||||||||||||||||
| end, | ||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||
| .then((res) => res.val!), | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+20
to
+27
Contributor
There was a problem hiding this comment. Add proper error handling for the The non-null assertion operator ( Consider this safer approach: clickhouse.ratelimits
.perMinute({
workspaceId: workspace.id,
namespaceId: namespace.id,
start: end - intervalMs,
end,
})
- .then((res) => res.val!),
+ .then((res) => {
+ if (!res.val) {
+ // You might want to return a default value or throw a custom error
+ return [];
+ }
+ return res.val;
+ }),📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||
| clickhouse.ratelimits | ||||||||||||||||||||||||||||||||||||||||||||||
| .latest({ workspaceId: workspace.id, namespaceId: namespace.id }) | ||||||||||||||||||||||||||||||||||||||||||||||
| .then((res) => res.val?.at(0)?.time), | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+20
to
+30
Contributor
There was a problem hiding this comment. 💡 Codebase verification Inconsistent error handling patterns detected The codebase shows mixed usage of error handling patterns with Clickhouse responses:
The underlying Clickhouse client returns 🔗 Analysis chainVerify consistent error handling patterns Let's ensure this error handling pattern is consistently implemented across other Clickhouse query handlers in the codebase. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check for consistent error handling patterns in Clickhouse queries
# Look for direct non-null assertions on Clickhouse response values
rg -A 2 "clickhouse\." | rg "\.val!"
# Look for proper error handling patterns
rg -A 2 "clickhouse\." | rg "\.val\?"
Length of output: 784 Script: #!/bin/bash
# Let's check the implementation of these Clickhouse query handlers to understand the return types
# Search for clickhouse ratelimits type definitions and implementations
ast-grep --pattern 'interface $_ {
ratelimits: {
$$$
}
}'
# Also check the business methods
ast-grep --pattern 'interface $_ {
business: {
$$$
}
}'
Length of output: 133 Script: #!/bin/bash
# Let's try a different approach to find the Clickhouse client implementation
# Look for files that might contain the Clickhouse client definition
fd clickhouse
# Search for return type definitions in these files
rg -A 3 "type.*Result.*="
# Look for error handling patterns in the implementation
rg -A 3 "export (type|interface).*Click"
Length of output: 2353 Script: #!/bin/bash
# Let's examine the error handling implementation in the dashboard's clickhouse client
# Check the clickhouse client implementation
cat apps/dashboard/lib/clickhouse.ts
# Also check the internal clickhouse package
cat internal/clickhouse/src/index.ts
# Look for Result type usage in these files
rg -A 3 "Result<" apps/dashboard/lib/clickhouse.ts internal/clickhouse/src/index.ts
Length of output: 3018 Script: #!/bin/bash
# Let's check the implementation of ratelimits functions to understand the return types
# Check the ratelimits implementation file
cat internal/clickhouse/src/ratelimits.ts
# Also look for Result type usage in any implementation files
rg -A 3 "Result<" internal/clickhouse/src/
Length of output: 8841 |
||||||||||||||||||||||||||||||||||||||||||||||
| ]); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| const totalRequests = history.reduce((sum, d) => sum + d.total, 0); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,11 +39,13 @@ export default async function HistoryPage(props: { | |
| if (!key?.keyAuth?.api) { | ||
| return notFound(); | ||
| } | ||
| const history = await clickhouse.verifications | ||
| .logs({ | ||
| workspaceId: UNKEY_WORKSPACE_ID, | ||
| keySpaceId: key.keyAuthId, | ||
| keyId: key.id, | ||
| }) | ||
| .then((res) => res.val!); | ||
|
Comment on lines
+42
to
+48
Contributor
There was a problem hiding this comment. Remove unsafe non-null assertion and add proper error handling The current implementation assumes the clickhouse query will always succeed and forcefully unwraps the result with Consider handling potential errors and empty results explicitly: - const history = await clickhouse.verifications
- .logs({
- workspaceId: UNKEY_WORKSPACE_ID,
- keySpaceId: key.keyAuthId,
- keyId: key.id,
- })
- .then((res) => res.val!);
+ const result = await clickhouse.verifications
+ .logs({
+ workspaceId: UNKEY_WORKSPACE_ID,
+ keySpaceId: key.keyAuthId,
+ keyId: key.id,
+ });
+
+ if (!result.val) {
+ // Handle the error case appropriately
+ // This could be showing an error state or returning notFound()
+ return <div>Failed to load verification history</div>;
+ }
+
+ const history = result.val;
|
||
|
|
||
| return <AccessTable verifications={history} />; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -84,7 +84,7 @@ const LastUsed: React.FC<{ workspaceId: string; keySpaceId: string; keyId: strin | |
| }) => { | ||
| const lastUsed = await clickhouse.verifications | ||
| .latest({ workspaceId, keySpaceId, keyId }) | ||
| .then((res) => res.val?.at(0)?.time ?? 0); | ||
|
|
||
| return ( | ||
| <Metric label="Last Used" value={lastUsed ? `${ms(Date.now() - lastUsed)} ago` : "Never"} /> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -91,11 +91,13 @@ export default async function RootKeyPage(props: { | |
| if (!keyForHistory?.keyAuth?.api) { | ||
| return notFound(); | ||
| } | ||
| const history = await clickhouse.verifications | ||
| .latest({ | ||
| workspaceId: UNKEY_WORKSPACE_ID, | ||
| keySpaceId: key.keyAuthId, | ||
| keyId: key.id, | ||
| }) | ||
| .then((res) => res.val!); | ||
|
|
||
| const apis = workspace.apis.map((api) => { | ||
| const apiPermissionsStructure = apiPermissions(api.id); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,7 @@ import { | |
| SelectValue, | ||
| } from "@/components/ui/select"; | ||
| import type { Permission } from "@unkey/db"; | ||
| import { type PropsWithChildren, useState } from "react"; | ||
| import { PermissionToggle } from "./permission_toggle"; | ||
| import { apiPermissions } from "./permissions"; | ||
|
|
||
|
|
@@ -38,10 +38,7 @@ export function DialogAddPermissionsForAPI( | |
| }); | ||
|
|
||
| const [selectedApiId, setSelectedApiId] = useState<string>(""); | ||
| const selectedApi = props.apis.find((api) => api.id === selectedApiId); | ||
|
|
||
| const isSelectionDisabled = | ||
| selectedApi && !apisWithoutPermission.some((api) => api.id === selectedApi.id); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,7 +64,7 @@ export default async function SuccessPage() { | |
| }); | ||
| } | ||
|
|
||
| const activeWorkspaces = await clickhouse.business.activeWorkspaces().then((res) => res.val!); | ||
| const chartData = activeWorkspaces.map(({ time, workspaces }) => ({ | ||
| x: new Date(time).toLocaleDateString(), | ||
| y: workspaces, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ | |
| }, | ||
| "dependencies": { | ||
| "@clickhouse/client-web": "^1.6.0", | ||
| "@unkey/error": "workspace:^", | ||
| "zod": "^3.23.8" | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,10 +30,10 @@ export function getBillableRatelimits(ch: Querier) { | |
| }); | ||
|
|
||
| const res = await query(args); | ||
| if (res.err || res.val.length === 0) { | ||
| return 0; | ||
| } | ||
| return res.val.at(0)?.count ?? 0; | ||
| }; | ||
| } | ||
| // get the billable verifications for a workspace in a specific month. | ||
|
|
@@ -65,9 +65,9 @@ export function getBillableVerifications(ch: Querier) { | |
| }); | ||
|
|
||
| const res = await query(args); | ||
| if (res.err || res.val.length === 0) { | ||
| return 0; | ||
| } | ||
| return res.val.at(0)?.count ?? 0; | ||
| }; | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,7 +1,8 @@ | ||||||
| import { type ClickHouseClient, createClient } from "@clickhouse/client-web"; | ||||||
| import { Err, Ok, type Result } from "@unkey/error"; | ||||||
| import { z } from "zod"; | ||||||
| import { InsertError, QueryError } from "./error"; | ||||||
| import type { Inserter, Querier } from "./interface"; | ||||||
| export type Config = { | ||||||
| url: string; | ||||||
| }; | ||||||
|
|
@@ -12,6 +13,7 @@ export class Client implements Querier, Inserter { | |||||
| constructor(config: Config) { | ||||||
| this.client = createClient({ | ||||||
| url: config.url, | ||||||
|
|
||||||
| clickhouse_settings: { | ||||||
| async_insert: 1, | ||||||
| wait_for_async_insert: 1, | ||||||
|
|
@@ -32,11 +34,11 @@ export class Client implements Querier, Inserter { | |||||
| // The schema of the output of each row | ||||||
| // Example: z.object({ id: z.string() }) | ||||||
| schema: TOut; | ||||||
| }): (params: z.input<TIn>) => Promise<Result<z.output<TOut>[], QueryError>> { | ||||||
| return async (params: z.input<TIn>): Promise<Result<z.output<TOut>[], QueryError>> => { | ||||||
| const validParams = req.params?.safeParse(params); | ||||||
| if (validParams?.error) { | ||||||
| return Err(new QueryError(`Bad params: ${validParams.error.message}`, { query: "" })); | ||||||
| } | ||||||
| const res = await this.client | ||||||
| .query({ | ||||||
|
|
@@ -48,7 +50,11 @@ export class Client implements Querier, Inserter { | |||||
| throw new Error(`${err.message} ${req.query}, params: ${JSON.stringify(params)}`); | ||||||
| }); | ||||||
| const rows = await res.json(); | ||||||
| const parsed = z.array(req.schema).safeParse(rows); | ||||||
| if (parsed.error) { | ||||||
| return Err(new QueryError(`Malformed data: ${parsed.error.message}`, { query: req.query })); | ||||||
| } | ||||||
| return Ok(parsed.data); | ||||||
| }; | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -57,22 +63,40 @@ export class Client implements Querier, Inserter { | |||||
| schema: TSchema; | ||||||
| }): ( | ||||||
| events: z.input<TSchema> | z.input<TSchema>[], | ||||||
| ) => Promise<Result<{ executed: boolean; query_id: string }, InsertError>> { | ||||||
| return async (events: z.input<TSchema> | z.input<TSchema>[]) => { | ||||||
| let validatedEvents: z.output<TSchema> | z.output<TSchema>[] | undefined = undefined; | ||||||
| const v = Array.isArray(events) | ||||||
| ? req.schema.array().safeParse(events) | ||||||
| : req.schema.safeParse(events); | ||||||
| if (!v.success) { | ||||||
| return Err(new InsertError(v.error.message)); | ||||||
| } | ||||||
| validatedEvents = v.data; | ||||||
|
|
||||||
| return this.retry(() => | ||||||
| this.client | ||||||
| .insert({ | ||||||
| table: req.table, | ||||||
| format: "JSONEachRow", | ||||||
| values: Array.isArray(validatedEvents) ? validatedEvents : [validatedEvents], | ||||||
| }) | ||||||
| .then((res) => Ok(res)) | ||||||
| .catch((err) => Err(new InsertError(err.message))), | ||||||
| ); | ||||||
|
Comment on lines
+77
to
+86
Contributor
There was a problem hiding this comment. Adjust error handling in The Consider modifying the function to throw exceptions on failure so that the return this.retry(() =>
this.client
.insert({
table: req.table,
format: "JSONEachRow",
values: Array.isArray(validatedEvents) ? validatedEvents : [validatedEvents],
})
- .then((res) => Ok(res))
- .catch((err) => Err(new InsertError(err.message))),
+ .then((res) => res)
+ .catch((err) => {
+ throw new InsertError(err.message);
+ }),
);Then, handle the result after the + try {
+ const res = await this.retry(...);
+ return Ok(res);
+ } catch (err) {
+ return Err(err);
+ }
|
||||||
| }; | ||||||
| } | ||||||
|
|
||||||
| private async retry<T>(fn: (attempt: number) => Promise<T>): Promise<T> { | ||||||
| let err: Error | undefined = undefined; | ||||||
| for (let i = 1; i <= 3; i++) { | ||||||
| try { | ||||||
| return fn(i); | ||||||
|
Contributor
There was a problem hiding this comment. Add In the Apply this code change: - return fn(i);
+ return await fn(i);📝 Committable suggestion
Suggested change
|
||||||
| } catch (e) { | ||||||
| console.warn(e); | ||||||
| err = e as Error; | ||||||
| } | ||||||
| } | ||||||
| throw err; | ||||||
| } | ||||||
| } | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use UnkeyApiError for consistent error handling.
The current implementation throws a generic
Error, which is inconsistent with the rest of the file that usesUnkeyApiError. This could lead to inconsistent error responses in the API.Apply this diff to maintain consistency: