Copilot commented on code in PR #1197: URL: https://github.com/apache/skywalking-banyandb/pull/1197#discussion_r3489722153
########## canopy/scripts/seed-m3-review.mjs: ########## @@ -0,0 +1,162 @@ +/* + * Licensed to Apache Software Foundation (ASF) under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Apache Software Foundation (ASF) licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/* + * Seed BanyanDB (via the BFF) with M3-reviewable data: + * - Measure group `sw_metric` with 3 lifecycle stages (Hot/Warm/Cold) + * - Stream group `sw_logs` + * - Measure `cpu_usage` with entity tags host, service + * - Stream `app_logs` with entity tags host, service + * - IndexRule `by_host` (TREE) + * - IndexRuleBinding `metric-bind` (subject=cpu_usage, rule=by_host) + * + * Idempotent: if a resource already exists (409), it's skipped silently. + * + * Usage: BFF=http://127.0.0.1:4000 node scripts/seed-m3-review.mjs + */ + +const BFF = process.env.BFF ?? 'http://127.0.0.1:4000'; + +async function login() { + const res = await fetch(`${BFF}/auth/login`, { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ username: 'admin', password: 'admin', endpoint: 'http://127.0.0.1:17913' }), Review Comment: `/auth/login` no longer accepts an `endpoint` field (the BFF target is configured via BANYANDB_TARGET), so this seed script will fail with 400 unless the payload is updated. ########## canopy/e2e/m3-metadata.spec.ts: ########## @@ -0,0 +1,128 @@ +/* + * Licensed to Apache Software Foundation (ASF) under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Apache Software Foundation (ASF) licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { test, expect } from '@playwright/test'; +import { join } from 'node:path'; +import { mkdirSync } from 'node:fs'; + +const screenshotsDir = join(process.cwd(), 'e2e', 'screenshots'); + +test.beforeAll(() => { + mkdirSync(screenshotsDir, { recursive: true }); +}); + +async function loginAsAdmin(page: import('@playwright/test').Page) { + const user = process.env.CANOPY_TEST_USER ?? 'admin'; + const pass = process.env.CANOPY_TEST_PASS ?? 'admin'; + const endpoint = process.env.BANYANDB_TARGET ?? 'http://127.0.0.1:17913'; + + const res = await page.request.post('/auth/login', { + data: { username: user, password: pass, endpoint }, + }); Review Comment: The BFF login endpoint no longer accepts an `endpoint` field, so this Playwright helper currently posts an invalid payload and will fail with 400. Drop the endpoint from the request body (the upstream is configured server-side). ########## canopy/server/package.json: ########## @@ -20,6 +20,7 @@ "@fastify/cookie": "^11.0.2", "undici": "^7.10.0", "bcryptjs": "^3.0.2", + "argon2": "^0.43.0", "js-yaml": "^4.1.0", Review Comment: `argon2` is added as a runtime dependency, but there is no usage in `canopy/server/src` (auth still uses bcryptjs). This adds a native module to installs/builds without effect; either implement argon2 hash verification or drop the dependency until it’s needed. ########## canopy/web/src/data/api.ts: ########## @@ -17,30 +17,294 @@ * under the License. */ -import type { GroupListResponse, QueryRequest, QueryResponse } from 'canopy-shared'; +import type { + GroupListResponse, Group, + CreateGroupRequest, UpdateGroupRequest, + CreateStreamRequest, UpdateStreamRequest, StreamSchema, + CreateMeasureRequest, UpdateMeasureRequest, MeasureSchema, + CreateTraceRequest, UpdateTraceRequest, TraceSchema, + CreateIndexRuleRequest, UpdateIndexRuleRequest, IndexRuleSchema, + CreateIndexRuleBindingRequest, UpdateIndexRuleBindingRequest, IndexRuleBindingSchema, + PropertySchema, + QueryRequest, QueryResponse, +} from 'canopy-shared'; import type { DataSource } from './DataSource.js'; -interface BanyanGroupListResponse { - groups?: GroupListResponse['groups']; +async function apiFetch<T>(url: string, init?: RequestInit): Promise<T> { + const res = await fetch(url, init); + if (!res.ok) { + let msg = `${res.status} ${res.statusText}`; + try { + const body = await res.json() as { message?: string }; + if (body.message) msg = body.message; + } catch { /* ignore */ } + throw new Error(msg); + } + if (res.status === 204) return undefined as unknown as T; + return res.json() as Promise<T>; +} + +const JSON_HEADERS = { 'content-type': 'application/json' }; + +// BanyanDB liaison speaks grpc-gateway, which serializes `google.protobuf.Timestamp` +// as RFC3339 strings (`2026-01-01T00:00:00Z`). Internally the web app works with +// epoch ms (smaller, comparable, easy to feed into `<input type="datetime-local">`), +// so we convert at the boundary instead of carrying strings through the UI. +type RawBinding = Omit<IndexRuleBindingSchema, 'beginAt' | 'expireAt'> & { + readonly beginAt?: string | number; + readonly expireAt?: string | number; +}; + +function decodeBinding(raw: RawBinding): IndexRuleBindingSchema { + return { + ...raw, + beginAt: toMs(raw.beginAt), + expireAt: toMs(raw.expireAt), + }; } +function toMs(v: string | number | undefined | null): number { + if (v == null) return 0; + if (typeof v === 'number') return v; + const t = Date.parse(v); + return Number.isNaN(t) ? 0 : t; +} + +function encodeBinding( + b: IndexRuleBindingSchema, +): Omit<RawBinding, 'beginAt' | 'expireAt'> & { beginAt: string; expireAt: string } { + // BanyanDB liaison rejects epoch-ms for `google.protobuf.Timestamp` (returns + // 400 "type mismatch"). Convert to RFC3339 strings here. Guard against a + // missing or malformed payload so the failure surfaces as a readable error + // instead of the cryptic "undefined is not an object (evaluating 'e.beginAt')" + // we'd otherwise hit inside `new Date(...)`. + if (b == null) { + throw new Error('encodeBinding: payload is missing (got ' + String(b) + ')'); + } + if (typeof b.beginAt !== 'number' || !Number.isFinite(b.beginAt)) { + throw new Error('encodeBinding: beginAt must be a finite number (got ' + String(b.beginAt) + ')'); + } + if (typeof b.expireAt !== 'number' || !Number.isFinite(b.expireAt)) { + throw new Error('encodeBinding: expireAt must be a finite number (got ' + String(b.expireAt) + ')'); + } + return { + ...b, + beginAt: new Date(b.beginAt).toISOString(), + expireAt: new Date(b.expireAt).toISOString(), + }; +} + +// BanyanDB REST API uses singular resource type names in paths; the app routes use plural. +const TYPE_SINGULAR: Record<string, string> = { + measures: 'measure', + streams: 'stream', + traces: 'trace', + properties: 'property', +}; + export class ApiDataSource implements DataSource { + // ── Groups ────────────────────────────────────────────────────────────── + async listGroups(): Promise<GroupListResponse> { - const res = await fetch('/api/v1/group/schema/lists'); - if (!res.ok) { - throw new Error(`Failed to load groups: ${res.status} ${res.statusText}`); + type RawGroup = Omit<Group, 'name'> & { metadata: { name: string } }; + const data = await apiFetch<{ group?: RawGroup[] }>('/api/v1/group/schema/lists'); + // BanyanDB liaison encodes the proto `repeated Group` field as a map keyed + // by index ("1": {...}, "2": {...}), so the resulting JS object's + // iteration order is whatever the server emitted — non-deterministic + // across requests. Sort by name on the client for a stable list order. + // + // BanyanDB also exposes internal objects prefixed with `_` (e.g. + // `_deletion_task`, backing the property schema registry). Strip them at + // the data layer so no consumer sees them as user data. + const groups = (data.group ?? []) + .filter((g) => !g.metadata.name.startsWith('_')) + .map((g) => ({ ...g, name: g.metadata.name })) + .sort((a, b) => a.name.localeCompare(b.name)); + return { groups }; + } + + async createGroup(req: CreateGroupRequest): Promise<Group> { + const data = await apiFetch<{ group: Group }>('/api/v1/group/schema', { + method: 'POST', headers: JSON_HEADERS, body: JSON.stringify(req), + }); + return data.group; + } Review Comment: `createGroup()` returns the raw `/api/v1/group/schema` response, but BanyanDB group payloads use `metadata.name` (as handled in `listGroups()`). If the create response also uses `metadata.name`, `created.name` will be undefined and the post-create navigation (`/metadata/${type}/${created.name}`) will break. ########## canopy/e2e/m3-indexrule.spec.ts: ########## @@ -0,0 +1,289 @@ +/* + * Licensed to Apache Software Foundation (ASF) under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Apache Software Foundation (ASF) licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { test, expect } from '@playwright/test'; +import { join } from 'node:path'; +import { mkdirSync } from 'node:fs'; + +const screenshotsDir = join(process.cwd(), 'e2e', 'screenshots'); + +const TS = `idx-${Date.now()}`; + +test.beforeAll(() => { + mkdirSync(screenshotsDir, { recursive: true }); +}); + +type ApiCtx = import('@playwright/test').APIRequestContext; + +async function apiLogin(ctx: ApiCtx) { + const user = process.env.CANOPY_TEST_USER ?? 'admin'; + const pass = process.env.CANOPY_TEST_PASS ?? 'admin'; + const endpoint = process.env.BANYANDB_TARGET ?? 'http://127.0.0.1:17913'; + const res = await ctx.post('/auth/login', { data: { username: user, password: pass, endpoint } }); + expect(res.ok()).toBeTruthy(); Review Comment: The BFF login endpoint no longer accepts an `endpoint` field, so this APIRequestContext login helper posts an invalid payload and will fail with 400. Remove `endpoint` from the request body. ########## canopy/web/src/pages/LoginPage.tsx: ########## @@ -81,7 +81,7 @@ function Field({ label, required, value, onChange, type = 'text', placeholder, m onKeyDown={(e) => { if (e.key === 'Enter' && onSubmit) onSubmit(); }} /> {eye && ( - <button type="button" className="f-eye" onClick={onEye} aria-label="Toggle visibility"> + <button type="button" className="f-eye" tabIndex={-1} onClick={onEye} aria-label="Toggle visibility"> Review Comment: The password visibility toggle is removed from the tab order via `tabIndex={-1}`, which prevents keyboard-only users from toggling visibility. Leaving it focusable (default tabIndex) improves accessibility without changing behavior for mouse users. ########## canopy/web/src/data/api.ts: ########## @@ -17,30 +17,294 @@ * under the License. */ -import type { GroupListResponse, QueryRequest, QueryResponse } from 'canopy-shared'; +import type { + GroupListResponse, Group, + CreateGroupRequest, UpdateGroupRequest, + CreateStreamRequest, UpdateStreamRequest, StreamSchema, + CreateMeasureRequest, UpdateMeasureRequest, MeasureSchema, + CreateTraceRequest, UpdateTraceRequest, TraceSchema, + CreateIndexRuleRequest, UpdateIndexRuleRequest, IndexRuleSchema, + CreateIndexRuleBindingRequest, UpdateIndexRuleBindingRequest, IndexRuleBindingSchema, + PropertySchema, + QueryRequest, QueryResponse, +} from 'canopy-shared'; import type { DataSource } from './DataSource.js'; -interface BanyanGroupListResponse { - groups?: GroupListResponse['groups']; +async function apiFetch<T>(url: string, init?: RequestInit): Promise<T> { + const res = await fetch(url, init); + if (!res.ok) { + let msg = `${res.status} ${res.statusText}`; + try { + const body = await res.json() as { message?: string }; + if (body.message) msg = body.message; + } catch { /* ignore */ } + throw new Error(msg); + } + if (res.status === 204) return undefined as unknown as T; + return res.json() as Promise<T>; +} + +const JSON_HEADERS = { 'content-type': 'application/json' }; + +// BanyanDB liaison speaks grpc-gateway, which serializes `google.protobuf.Timestamp` +// as RFC3339 strings (`2026-01-01T00:00:00Z`). Internally the web app works with +// epoch ms (smaller, comparable, easy to feed into `<input type="datetime-local">`), +// so we convert at the boundary instead of carrying strings through the UI. +type RawBinding = Omit<IndexRuleBindingSchema, 'beginAt' | 'expireAt'> & { + readonly beginAt?: string | number; + readonly expireAt?: string | number; +}; + +function decodeBinding(raw: RawBinding): IndexRuleBindingSchema { + return { + ...raw, + beginAt: toMs(raw.beginAt), + expireAt: toMs(raw.expireAt), + }; } +function toMs(v: string | number | undefined | null): number { + if (v == null) return 0; + if (typeof v === 'number') return v; + const t = Date.parse(v); + return Number.isNaN(t) ? 0 : t; +} + +function encodeBinding( + b: IndexRuleBindingSchema, +): Omit<RawBinding, 'beginAt' | 'expireAt'> & { beginAt: string; expireAt: string } { + // BanyanDB liaison rejects epoch-ms for `google.protobuf.Timestamp` (returns + // 400 "type mismatch"). Convert to RFC3339 strings here. Guard against a + // missing or malformed payload so the failure surfaces as a readable error + // instead of the cryptic "undefined is not an object (evaluating 'e.beginAt')" + // we'd otherwise hit inside `new Date(...)`. + if (b == null) { + throw new Error('encodeBinding: payload is missing (got ' + String(b) + ')'); + } + if (typeof b.beginAt !== 'number' || !Number.isFinite(b.beginAt)) { + throw new Error('encodeBinding: beginAt must be a finite number (got ' + String(b.beginAt) + ')'); + } + if (typeof b.expireAt !== 'number' || !Number.isFinite(b.expireAt)) { + throw new Error('encodeBinding: expireAt must be a finite number (got ' + String(b.expireAt) + ')'); + } + return { + ...b, + beginAt: new Date(b.beginAt).toISOString(), + expireAt: new Date(b.expireAt).toISOString(), + }; +} + +// BanyanDB REST API uses singular resource type names in paths; the app routes use plural. +const TYPE_SINGULAR: Record<string, string> = { + measures: 'measure', + streams: 'stream', + traces: 'trace', + properties: 'property', +}; + export class ApiDataSource implements DataSource { + // ── Groups ────────────────────────────────────────────────────────────── + async listGroups(): Promise<GroupListResponse> { - const res = await fetch('/api/v1/group/schema/lists'); - if (!res.ok) { - throw new Error(`Failed to load groups: ${res.status} ${res.statusText}`); + type RawGroup = Omit<Group, 'name'> & { metadata: { name: string } }; + const data = await apiFetch<{ group?: RawGroup[] }>('/api/v1/group/schema/lists'); + // BanyanDB liaison encodes the proto `repeated Group` field as a map keyed + // by index ("1": {...}, "2": {...}), so the resulting JS object's + // iteration order is whatever the server emitted — non-deterministic + // across requests. Sort by name on the client for a stable list order. + // + // BanyanDB also exposes internal objects prefixed with `_` (e.g. + // `_deletion_task`, backing the property schema registry). Strip them at + // the data layer so no consumer sees them as user data. + const groups = (data.group ?? []) + .filter((g) => !g.metadata.name.startsWith('_')) + .map((g) => ({ ...g, name: g.metadata.name })) + .sort((a, b) => a.name.localeCompare(b.name)); + return { groups }; + } + + async createGroup(req: CreateGroupRequest): Promise<Group> { + const data = await apiFetch<{ group: Group }>('/api/v1/group/schema', { + method: 'POST', headers: JSON_HEADERS, body: JSON.stringify(req), + }); + return data.group; + } + + async updateGroup(name: string, req: UpdateGroupRequest): Promise<Group> { + const data = await apiFetch<{ group: Group }>(`/api/v1/group/schema/${name}`, { + method: 'PUT', headers: JSON_HEADERS, body: JSON.stringify(req), + }); + return data.group; + } Review Comment: `updateGroup()` returns the raw `/api/v1/group/schema/:name` response, but BanyanDB group payloads use `metadata.name` (as handled in `listGroups()`). Normalizing the response to always include a top-level `name` avoids breaking any callers that expect `Group.name`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
