Skip to content

refactor(webui): Migrate archive metadata SQL query route to new Fastify architecture. #1028

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Jul 10, 2025
Merged

refactor(webui): Migrate archive metadata SQL query route to new Fastify architecture. #1028

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7a5449b
latest
Jun 23, 2025
5e88f4d
latest
Jun 23, 2025
ecc6199
latest
Jun 23, 2025
1fde043
latest
Jun 23, 2025
5378604
latest
Jul 2, 2025
644cc26
latest
Jul 2, 2025
65197de
Merge branch 'main' into sqlRe
hoophalab Jul 7, 2025
88fdee7
merge main
Jul 10, 2025
3a2944e
Merge branch 'main' into sqlRe
hoophalab Jul 10, 2025
b1d09c7
Merge branch 'main' into sqlRe
davemarco Jul 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion components/webui/client/src/api/sql/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import axios from "axios";
* @return
*/
const querySql = async <T>(queryString: string) => {
return axios.post<T>("/query/sql", {queryString});
return axios.post<T>("/api/archive-metadata/sql", {queryString});
};

export {querySql};
1 change: 1 addition & 0 deletions components/webui/server/eslint.config.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ const EslintConfig = [
{
// TypeBox imports
capIsNewExceptions: [
"Type.Any",
"Type.Enum",
"Type.Integer",
"Type.Literal",
Expand Down
39 changes: 39 additions & 0 deletions components/webui/server/src/fastify-v2/routes/api/archive-metadata/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import {
FastifyPluginAsyncTypebox,
Type,
} from "@fastify/type-provider-typebox";
import {StatusCodes} from "http-status-codes";

import {SqlSchema} from "../../../schemas/archive-metadata.js";


/**
* Archive metadata API routes.
*
* @param fastify
*/
const plugin: FastifyPluginAsyncTypebox = async (fastify) => {
const mysqlConnectionPool = fastify.mysql.pool;

fastify.post(
"/sql",
{
schema: {
body: SqlSchema,
response: {
[StatusCodes.OK]: Type.Any(),
Copy link
Contributor

@coderabbitai coderabbitai bot Jul 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

Copy link
Member

@junhaoliao junhaoliao Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Contributor

@coderabbitai coderabbitai bot Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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:

  1. Structure - Consistent response wrapper
  2. Documentation - Clear API contract
  3. Flexibility - data field can still accommodate any query result
  4. 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.

Copy link
Contributor Author

@davemarco davemarco Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@coderabbitai coderabbitai bot Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

@junhaoliao junhaoliao Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@davemarco davemarco Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes i think since its generic there is not much we can do for now, but the client can specify typing

},
tags: ["Archive Metadata"],
},
},
async (req, reply) => {
const {queryString} = req.body;
const [result] = await mysqlConnectionPool.query(queryString);
reply.code(StatusCodes.OK);
Copy link
Contributor

@hoophalab hoophalab Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel we don't actually need explicit statuscode, but feel free to keep it here.

Copy link
Contributor Author

@davemarco davemarco Jul 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@hoophalab hoophalab Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds fairly reasonable. Let's keep it here.


return result;
},
);
};

export default plugin;
13 changes: 13 additions & 0 deletions components/webui/server/src/fastify-v2/schemas/archive-metadata.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import {Type} from "@sinclair/typebox";

import {StringSchema} from "./common.js";


/**
* Schema for SQL query request.
*/
const SqlSchema = Type.Object({
queryString: StringSchema,
});

export {SqlSchema};
12 changes: 0 additions & 12 deletions components/webui/server/src/plugins/DbManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,18 +82,6 @@ class DbManager {
});
}

/**
* Submits a query to MySQL.
*
* @param queryString
* @return The result from MySQL.
*/
async queryMySql (queryString: string) {
const [result] = await this.#mysqlConnectionPool.query(queryString);
return result;
}


/**
* Submits a stream extraction job to the scheduler and waits for it to finish.
*
Expand Down
16 changes: 0 additions & 16 deletions components/webui/server/src/routes/query.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,25 +16,9 @@ import {
* @param app
* @return
*/
// eslint-disable-next-line max-lines-per-function
const routes: FastifyPluginAsync = async (app) => {
const fastify = app.withTypeProvider<TypeBoxTypeProvider>();

fastify.post(
"/query/sql",
{
schema: {
body: Type.Object({
queryString: Type.String({minLength: 1}),
}),
},
},
async (req) => {
const {queryString} = req.body;
return await fastify.dbManager.queryMySql(queryString);
},
);

fastify.post("/query/extract-stream", {
schema: {
body: Type.Object({
Expand Down