refactor(webui): Migrate archive metadata SQL query route to new Fastify architecture. #1028
Conversation
WalkthroughA new Fastify API route for archive metadata SQL queries was introduced, replacing the previous Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FastifyServer
participant MySQLPool
Client->>FastifyServer: POST /api/archive-metadata/sql { queryString }
FastifyServer->>MySQLPool: Execute queryString
MySQLPool-->>FastifyServer: Query result
FastifyServer-->>Client: 200 OK + result
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
💤 Files with no reviewable changes (2)
🧰 Additional context used📓 Path-based instructions (1)`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
⚙️ Source: CodeRabbit Configuration File List of files the instruction was applied to:
🧠 Learnings (2)📓 Common learningscomponents/webui/client/src/api/sql/index.ts (1)🔇 Additional comments (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
components/log-viewer-webui/client/src/pages/IngestPage/sqlConfig.ts(1 hunks)components/log-viewer-webui/server/src/fastify-v2/routes/api/archive-metadata/index.ts(1 hunks)components/log-viewer-webui/server/src/fastify-v2/schemas/archive-metadata.ts(1 hunks)components/log-viewer-webui/server/src/plugins/DbManager.ts(0 hunks)components/log-viewer-webui/server/src/routes/query.ts(0 hunks)
💤 Files with no reviewable changes (2)
- components/log-viewer-webui/server/src/plugins/DbManager.ts
- components/log-viewer-webui/server/src/routes/query.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/log-viewer-webui/client/src/pages/IngestPage/sqlConfig.tscomponents/log-viewer-webui/server/src/fastify-v2/routes/api/archive-metadata/index.tscomponents/log-viewer-webui/server/src/fastify-v2/schemas/archive-metadata.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint-check (ubuntu-latest)
🔇 Additional comments (1)
components/log-viewer-webui/client/src/pages/IngestPage/sqlConfig.ts (1)
55-55: LGTM! Endpoint URL updated correctly for the new Fastify architecture.The URL change from
/query/sqlto/api/archive-metadata/sqlproperly aligns with the new Fastify v2 API structure while maintaining the same function signature and request payload.
components/log-viewer-webui/server/src/fastify-v2/routes/api/archive-metadata/index.ts
Show resolved
Hide resolved
components/log-viewer-webui/server/src/fastify-v2/routes/api/archive-metadata/index.ts
Show resolved
Hide resolved
components/log-viewer-webui/server/src/fastify-v2/schemas/archive-metadata.ts
Outdated
Show resolved
Hide resolved
components/log-viewer-webui/server/src/fastify-v2/schemas/archive-metadata.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
components/webui/client/src/pages/IngestPage/sqlConfig.ts(1 hunks)components/webui/server/eslint.config.mjs(1 hunks)components/webui/server/src/fastify-v2/schemas/archive-metadata.ts(1 hunks)components/webui/server/src/plugins/DbManager.ts(0 hunks)components/webui/server/src/routes/query.ts(0 hunks)
💤 Files with no reviewable changes (2)
- components/webui/server/src/routes/query.ts
- components/webui/server/src/plugins/DbManager.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
components/webui/client/src/pages/IngestPage/sqlConfig.tscomponents/webui/server/src/fastify-v2/schemas/archive-metadata.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: junhaoliao
PR: y-scope/clp#939
File: components/package-template/src/etc/clp-config.yml:64-64
Timestamp: 2025-06-22T04:01:43.409Z
Learning: The new webui architecture uses Fastify with Pino logging instead of the previous Winston-based logging system that was removed during the webui refactoring.
components/webui/server/eslint.config.mjs (1)
Learnt from: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In `components/log-viewer-webui/client/src/api/query.js`, the `ExtractJsonResp` type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
components/webui/client/src/pages/IngestPage/sqlConfig.ts (1)
Learnt from: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In `components/log-viewer-webui/client/src/api/query.js`, the `ExtractJsonResp` type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
components/webui/server/src/fastify-v2/schemas/archive-metadata.ts (1)
Learnt from: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In `components/log-viewer-webui/client/src/api/query.js`, the `ExtractJsonResp` type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (2)
components/webui/client/src/pages/IngestPage/sqlConfig.ts (1)
49-49: Cannot locate new API endpoint on serverI wasn’t able to find an implementation of the
/api/archive-metadata/sqlroute in the server code. Please verify that this endpoint is defined and registered in your Fastify routes, or adjust the client call to match the actual server path.• Check under
components/webui/server/for the archive-metadata SQL route (e.g. in your routes or plugins files)
• Ensure the endpoint is registered with Fastify (e.g.fastify.post('/api/archive-metadata/sql', …))components/webui/server/src/fastify-v2/schemas/archive-metadata.ts (1)
1-14: Import path verification successfulVerified that
components/webui/server/src/fastify-v2/schemas/common.tsexportsStringSchemaand that importing from./common.jsmatches the pattern used insearch.ts. No further changes required.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/webui/server/src/fastify-v2/routes/api/archive-metadata/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
components/webui/server/src/fastify-v2/routes/api/archive-metadata/index.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: junhaoliao
PR: y-scope/clp#939
File: components/package-template/src/etc/clp-config.yml:64-64
Timestamp: 2025-06-22T04:01:43.409Z
Learning: The new webui architecture uses Fastify with Pino logging instead of the previous Winston-based logging system that was removed during the webui refactoring.
components/webui/server/src/fastify-v2/routes/api/archive-metadata/index.ts (1)
Learnt from: davemarco
PR: y-scope/clp#1015
File: components/log-viewer-webui/server/src/routes/static.ts:65-70
Timestamp: 2025-06-16T13:05:27.349Z
Learning: In components/log-viewer-webui/server/src/routes/static.ts, when decorateReply is set to true in fastifyStatic configuration, the reply.sendFile() method automatically uses the root directory configured in the static plugin registration, eliminating the need to pass the root directory as a second parameter.
🔇 Additional comments (1)
components/webui/server/src/fastify-v2/routes/api/archive-metadata/index.ts (1)
39-39: LGTM!The export follows standard Fastify plugin conventions.
| schema: { | ||
| body: SqlSchema, | ||
| response: { | ||
| [StatusCodes.OK]: Type.Any(), |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider improving response type safety.
Using Type.Any() for the response schema eliminates type safety benefits. Consider defining a more specific schema based on expected query results.
-[StatusCodes.OK]: Type.Any(),
+[StatusCodes.OK]: Type.Object({
+ // Define expected response structure based on your use case
+ rows: Type.Array(Type.Any()),
+ fields: Type.Optional(Type.Array(Type.Any())),
+}),🤖 Prompt for AI Agents
In components/webui/server/src/fastify-v2/routes/api/archive-metadata/index.ts
at line 24, the response schema uses Type.Any(), which removes type safety.
Replace Type.Any() with a more specific TypeBox schema that accurately
represents the expected structure of the query results to improve type safety
and validation.
There was a problem hiding this comment.
quick question: why do we want to specify Any() here? would it be beneficial to list out what fields are available in the response (as some documentation of the response schema)?
There was a problem hiding this comment.
@junhaoliao You raise an excellent point about the trade-offs between flexibility and type safety/documentation.
The current Type.Any() approach has both pros and cons:
Why Type.Any() might be used here:
- This endpoint accepts arbitrary SQL queries, so the response structure is inherently dynamic
- Different queries return vastly different structures (SELECT vs INSERT/UPDATE/DELETE results)
- Maximum flexibility for any SQL query type
Benefits of defining specific response fields:
- Better API documentation and developer experience
- Some level of type safety and validation
- Clearer expectations for API consumers
Suggested middle-ground approach:
[StatusCodes.OK]: Type.Object({
data: Type.Any(), // The actual query results
queryType: Type.Optional(Type.String()), // e.g., "SELECT", "INSERT", etc.
rowCount: Type.Optional(Type.Number()),
fields: Type.Optional(Type.Array(Type.Object({
name: Type.String(),
type: Type.String(),
}))),
}),This provides:
- Structure - Consistent response wrapper
- Documentation - Clear API contract
- Flexibility -
datafield can still accommodate any query result - Metadata - Additional info about the query execution
The challenge is that MySQL query results have different shapes (result sets vs execution metadata), so some level of Any() typing may be unavoidable while still providing useful structure and documentation.
What's your preference for this API design?
🧠 Learnings used
Learnt from: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In `components/log-viewer-webui/client/src/api/query.js`, the `ExtractJsonResp` type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
Learnt from: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:35-41
Timestamp: 2024-11-19T19:52:43.429Z
Learning: For internal APIs in `components/log-viewer-webui/client/src/api/query.js`, runtime validation of parameters may not be necessary since the APIs are not exposed to end users, and JsDoc type annotations may be sufficient.
Learnt from: junhaoliao
PR: y-scope/clp#939
File: components/package-template/src/etc/clp-config.yml:64-64
Timestamp: 2025-06-22T04:01:43.409Z
Learning: The new webui architecture uses Fastify with Pino logging instead of the previous Winston-based logging system that was removed during the webui refactoring.
There was a problem hiding this comment.
Similiarly to what the AI responded, the response will be different for types of queries see resultheader or a custom row. Typing of response is not exactly clear with arbitrary query
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Right, looking at https://sidorares.github.io/node-mysql2/docs/examples/queries/simple-queries/select#queryoptions , i was thinking that the return type can be written as Type.Array(Type.Any()) if I understand correctly.
Anyways as you suggested, it probably won't make a difference given we can't specify the element's types. This is fine to leave as-is until we come up with better API endpoints testing / documentation infra like Swagger
There was a problem hiding this comment.
yes i think since its generic there is not much we can do for now, but the client can specify typing
components/webui/server/src/fastify-v2/routes/api/archive-metadata/index.ts
Show resolved
Hide resolved
components/webui/server/src/fastify-v2/routes/api/archive-metadata/index.ts
Show resolved
Hide resolved
|
@hoophalab - i merged main into this, please rereview |
davemarco
changed the title
|
@hoophalab - i merged main again |
There was a problem hiding this comment.
lgtm. We need write permission from @junhaoliao. Also, any suggestions on the pr title?
Validations:
- run clp-text and clp-json
- compress several files
- all cards displayed correctly
| async (req, reply) => { | ||
| const {queryString} = req.body; | ||
| const [result] = await mysqlConnectionPool.query(queryString); | ||
| reply.code(StatusCodes.OK); |
There was a problem hiding this comment.
I feel we don't actually need explicit statuscode, but feel free to keep it here.
There was a problem hiding this comment.
in the search routes i am explicity using fancier positive status codes(not always OK) like NO CONTENT, and CREATE. I like the explicit status code since it tells dev that something must be returned to client. I would prefer to leave it here
There was a problem hiding this comment.
sounds fairly reasonable. Let's keep it here.
|
@junhaoliao tagging for write access |
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
the PR title lgtm; deferring to @hoophalab 's review
this is a separate request - we might want to update the target path at
to be more specific once we're done moving all API routes to /api| schema: { | ||
| body: SqlSchema, | ||
| response: { | ||
| [StatusCodes.OK]: Type.Any(), |
There was a problem hiding this comment.
Right, looking at https://sidorares.github.io/node-mysql2/docs/examples/queries/simple-queries/select#queryoptions , i was thinking that the return type can be written as Type.Array(Type.Any()) if I understand correctly.
Anyways as you suggested, it probably won't make a difference given we can't specify the element's types. This is fine to leave as-is until we come up with better API endpoints testing / documentation infra like Swagger
makes sense to update this when finished this refactor |
3 participants
Description
PR moves sql query route into new fastify architecture. This is a chain of PRs which started with #1027
In addition, I just removed the querySql function out of dbManager. It can just be declared in route which in simpler.
I also renamed the route, and did some minor refactor.
Checklist
breaking change.
Validation performed
Metadata tables load correctly
Summary by CodeRabbit
New Features
/api/archive-metadata/sql.Bug Fixes
Chores