-
Notifications
You must be signed in to change notification settings - Fork 88
feat(new-webui): Port search services from old-webui to Fastify plugins. #912
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.
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
Changes from 16 commits
206f755
8a00cc7
592f5e4
c93574e
f22304e
14c2f00
4a74063
3be40ca
4ba04b3
a4548c5
50e2ce7
45e623c
c9dea3b
8dc3883
440b674
8dd1341
99b8d04
215bdf4
0126404
cd43194
7394d32
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ import FastifyV1App from "../app.js"; | |
| const INTERNAL_SERVER_ERROR_CODE = 500; | ||
| const RATE_LIMIT_MAX_REQUESTS = 3; | ||
| const RATE_LIMIT_TIME_WINDOW_MS = 500; | ||
| const IGNORED_FILES_REGEX = /^.*(?:utils|typings)\.js$/; | ||
|
|
||
| /** | ||
| * Registers all plugins and routes. | ||
|
|
@@ -47,14 +48,16 @@ export default async function serviceApp ( | |
| // Loads all application plugins. | ||
| fastify.register(fastifyAutoload, { | ||
| dir: path.join(import.meta.dirname, "plugins/app"), | ||
| ignorePattern: IGNORED_FILES_REGEX, | ||
| options: {...opts}, | ||
| }); | ||
|
|
||
| // Loads all routes. | ||
| fastify.register(fastifyAutoload, { | ||
| dir: path.join(import.meta.dirname, "routes"), | ||
| autoHooks: true, | ||
| cascadeHooks: true, | ||
| dir: path.join(import.meta.dirname, "routes"), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as plugins
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This also creates nice routes like api/search/query. Like it will add the folder structure to the route, so i like it |
||
| ignorePattern: IGNORED_FILES_REGEX, | ||
| options: {...opts}, | ||
| }); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,172 @@ | ||||||||||||||||||||||||||||||
| import type {MySQLPromisePool} from "@fastify/mysql"; | ||||||||||||||||||||||||||||||
| import {encode} from "@msgpack/msgpack"; | ||||||||||||||||||||||||||||||
| import settings from "@settings" with { type: "json" }; | ||||||||||||||||||||||||||||||
| import {FastifyInstance} from "fastify"; | ||||||||||||||||||||||||||||||
| import fp from "fastify-plugin"; | ||||||||||||||||||||||||||||||
| import {ResultSetHeader} from "mysql2"; | ||||||||||||||||||||||||||||||
| import {setTimeout} from "timers/promises"; | ||||||||||||||||||||||||||||||
coderabbitai[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||||||||
| QUERY_JOB_STATUS, | ||||||||||||||||||||||||||||||
| QUERY_JOB_STATUS_WAITING_STATES, | ||||||||||||||||||||||||||||||
| QUERY_JOB_TYPE, | ||||||||||||||||||||||||||||||
| QUERY_JOBS_TABLE_COLUMN_NAMES, | ||||||||||||||||||||||||||||||
| QueryJob, | ||||||||||||||||||||||||||||||
| } from "../../../../../typings/query.js"; | ||||||||||||||||||||||||||||||
| import {JOB_COMPLETION_STATUS_POLL_INTERVAL_MILLIS} from "./typings.js"; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||
| * Class for submitting and monitoring query jobs in the database. | ||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||
| class QueryJobsDbManager { | ||||||||||||||||||||||||||||||
| #sqlDbConnPool: MySQLPromisePool; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| private constructor (sqlDbConnPool: MySQLPromisePool) { | ||||||||||||||||||||||||||||||
| this.#sqlDbConnPool = sqlDbConnPool; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||
| * Creates a new QueryJobsDbManager. | ||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||
| * @param fastify | ||||||||||||||||||||||||||||||
| * @return | ||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||
| static create (fastify: FastifyInstance): QueryJobsDbManager { | ||||||||||||||||||||||||||||||
| const sqlDbConnPool = fastify.mysql; | ||||||||||||||||||||||||||||||
| return new QueryJobsDbManager(sqlDbConnPool); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||
| * Submits a search job to the database. | ||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||
| * @param searchConfig The arguments for the query. | ||||||||||||||||||||||||||||||
| * @return The job's ID. | ||||||||||||||||||||||||||||||
| * @throws {Error} on error. | ||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||
| async submitSearchJob (searchConfig: object): Promise<number> { | ||||||||||||||||||||||||||||||
| const [queryInsertResults] = await this.#sqlDbConnPool.query<ResultSetHeader>( | ||||||||||||||||||||||||||||||
| `INSERT INTO ${settings.SqlDbQueryJobsTableName} | ||||||||||||||||||||||||||||||
| (${QUERY_JOBS_TABLE_COLUMN_NAMES.JOB_CONFIG}, | ||||||||||||||||||||||||||||||
| ${QUERY_JOBS_TABLE_COLUMN_NAMES.TYPE}) | ||||||||||||||||||||||||||||||
| VALUES (?, ?)`, | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
| `INSERT INTO ${settings.SqlDbQueryJobsTableName} | |
| (${QUERY_JOBS_TABLE_COLUMN_NAMES.JOB_CONFIG}, | |
| ${QUERY_JOBS_TABLE_COLUMN_NAMES.TYPE}) | |
| VALUES (?, ?)`, | |
| ` | |
| INSERT INTO ${settings.SqlDbQueryJobsTableName} ( | |
| ${QUERY_JOBS_TABLE_COLUMN_NAMES.JOB_CONFIG}, | |
| ${QUERY_JOBS_TABLE_COLUMN_NAMES.TYPE} | |
| ) | |
| VALUES (?, ?) | |
| `, |
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
Outdated
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.
| `UPDATE ${settings.SqlDbQueryJobsTableName} | |
| SET ${QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS} = ${QUERY_JOB_STATUS.CANCELLING} | |
| WHERE ${QUERY_JOBS_TABLE_COLUMN_NAMES.ID} = ? | |
| AND ${QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS} | |
| IN (${QUERY_JOB_STATUS.PENDING}, ${QUERY_JOB_STATUS.RUNNING})`, | |
| ` | |
| UPDATE ${settings.SqlDbQueryJobsTableName} | |
| SET ${QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS} = ${QUERY_JOB_STATUS.CANCELLING} | |
| WHERE ${QUERY_JOBS_TABLE_COLUMN_NAMES.ID} = ? | |
| AND ${QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS} | |
| IN (${QUERY_JOB_STATUS.PENDING}, ${QUERY_JOB_STATUS.RUNNING}) | |
| `, |
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.
🧹 Nitpick (assertive)
Consider using a condition in the while loop.
The infinite loop with a break statement could be replaced with a conditional while loop for better readability.
- // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
- while (true) {
+ let jobCompleted = false;
+ while (!jobCompleted) {And then replace the break; on line 151 with:
- break;
+ jobCompleted = true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | |
| while (true) { | |
| - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | |
| - while (true) { | |
| + let jobCompleted = false; | |
| + while (!jobCompleted) { | |
| @@ | |
| - break; | |
| + jobCompleted = true; |
🤖 Prompt for AI Agents
In
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.ts
around lines 111 to 112, replace the infinite while(true) loop with a while loop
that uses an explicit condition to control the loop execution. Identify the
condition that currently leads to the break statement on line 151 and use that
condition directly in the while loop. Remove the break statement on line 151
after updating the loop condition to improve readability and maintainability.
Outdated
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.
| ` | |
| SELECT ${QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS} | |
| FROM ${settings.SqlDbQueryJobsTableName} | |
| WHERE ${QUERY_JOBS_TABLE_COLUMN_NAMES.ID} = ? | |
| `, | |
| ` | |
| SELECT ${QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS} | |
| FROM ${settings.SqlDbQueryJobsTableName} | |
| WHERE ${QUERY_JOBS_TABLE_COLUMN_NAMES.ID} = ? | |
| `, |
Outdated
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.
| } catch (e: unknown) { | |
| let errorMessage: string; | |
| if (e instanceof Error) { | |
| errorMessage = e.message; | |
| } else { | |
| errorMessage = String(e); | |
| } | |
| throw new Error(`Failed to query status for job ${jobId} - ${errorMessage}`); | |
| } catch (e: unknown) { | |
| console.error("Failed to query status for job ${jobId}") | |
| throw e; | |
| } |
Current solution removes error's stack trace, which makes the error hard to debug. I prefer two solutions:
- simply
console.error - create a
class JobQueryError { constructor(message, jobId, rawError)}
e 's info is not discarded in both way
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.
I agree we do not want to discard the stack trace in the original Error. how about
throw new Error("Failed to query status for job ${jobId}", { cause: e });
Outdated
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.
how about:
let queryJob: QueryJob | undefined;
try {
...
[queryJob] = queryRows;
} catch () {...}
if ("undefined" === typeof queryJob) {
throw new Error(`Job ${jobId} not found in database.`);
}
const status = queryJob[QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS];
...
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.
ill try something like this
hoophalab marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| /** | ||
| * Interval in milliseconds for polling the completion status of a job. | ||
| */ | ||
| export const JOB_COMPLETION_STATUS_POLL_INTERVAL_MILLIS = 500; |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,101 @@ | ||||||||||||||||||||
| import {FastifyInstance} from "fastify"; | ||||||||||||||||||||
| import fp from "fastify-plugin"; | ||||||||||||||||||||
| import type { | ||||||||||||||||||||
| Collection, | ||||||||||||||||||||
| Db, | ||||||||||||||||||||
| } from "mongodb"; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| import {Nullable} from "../../../../../typings/common.js"; | ||||||||||||||||||||
| import { | ||||||||||||||||||||
| CollectionDroppedError, | ||||||||||||||||||||
| SearchResultsDocument, | ||||||||||||||||||||
| } from "./typings.js"; | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| /** | ||||||||||||||||||||
| * Class to keep track of MongoDB collections created for search jobs, ensuring all collections | ||||||||||||||||||||
| * have unique names. | ||||||||||||||||||||
| */ | ||||||||||||||||||||
| class SearchJobCollectionsManager { | ||||||||||||||||||||
|
||||||||||||||||||||
| #jobIdToCollectionsMap: Map<string, Nullable<Collection<SearchResultsDocument>>> = | ||||||||||||||||||||
| new Map(); | ||||||||||||||||||||
|
|
||||||||||||||||||||
coderabbitai[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||
| #db: Db; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| private constructor (db: Db) { | ||||||||||||||||||||
| this.#db = db; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /** | ||||||||||||||||||||
| * Creates a new SearchJobCollectionsManager. | ||||||||||||||||||||
| * | ||||||||||||||||||||
| * @param fastify | ||||||||||||||||||||
| * @return | ||||||||||||||||||||
| * @throws {Error} if the MongoDB database is not found. | ||||||||||||||||||||
| */ | ||||||||||||||||||||
| static create (fastify: FastifyInstance): SearchJobCollectionsManager { | ||||||||||||||||||||
| if ("undefined" === typeof fastify.mongo.db) { | ||||||||||||||||||||
| throw new Error("MongoDB database not found"); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| return new SearchJobCollectionsManager(fastify.mongo.db); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /** | ||||||||||||||||||||
| * Gets, or if it doesn't exist, creates a MongoDB collection named with the given job ID. | ||||||||||||||||||||
| * | ||||||||||||||||||||
| * @param jobId | ||||||||||||||||||||
| * @return MongoDB collection | ||||||||||||||||||||
| * @throws {CollectionDroppedError} if the collection was already dropped. | ||||||||||||||||||||
| */ | ||||||||||||||||||||
| async getOrCreateCollection (jobId: number): Promise<Collection<SearchResultsDocument>> { | ||||||||||||||||||||
|
||||||||||||||||||||
| const name = jobId.toString(); | ||||||||||||||||||||
| if (false === this.#jobIdToCollectionsMap.has(name)) { | ||||||||||||||||||||
| this.#jobIdToCollectionsMap.set(name, this.#db.collection(name)); | ||||||||||||||||||||
| } else if (null === this.#jobIdToCollectionsMap.get(name)) { | ||||||||||||||||||||
coderabbitai[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||
| throw new CollectionDroppedError(name); | ||||||||||||||||||||
|
||||||||||||||||||||
| if (false === this.#jobIdToCollectionsMap.has(name)) { | |
| this.#jobIdToCollectionsMap.set(name, this.#db.collection(name)); | |
| } else if (null === this.#jobIdToCollectionsMap.get(name)) { | |
| throw new CollectionDroppedError(name); | |
| const collection = this.#jobIdToCollectionsMap.get(name) | |
| if ("undefined" === typeof collection) { | |
| this.#jobIdToCollectionsMap.set(name, this.#db.collection(name)); | |
| } else if (null === collection) { | |
| throw new CollectionDroppedError(name); |
davemarco marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Outdated
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.
Consider using CollectionNotFoundError
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.
we are only using the custom error earlier, since it is caught later in the route see #913 . I dont want to add a custom error if not explicity used.
Outdated
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.
| if ("undefined" === typeof collection || null === collection) { | |
| throw new Error(`Collection ${name} not found`); | |
| } | |
| if ("undefined" === typeof collection) { | |
| throw new Error(`Collection ${name} not found`); | |
| } else if (null === collection) { | |
| throw new CollectionDroppedError(...); | |
| } |
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.
sure i guess this is okay, but i dont believe this error will be caught anywhere. I can make the change though
hoophalab marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| /** | ||
| * MongoDB document for search results. | ||
| */ | ||
| interface SearchResultsDocument { | ||
| _id: string; | ||
| orig_file_id: string; | ||
| orig_file_path: string; | ||
| log_event_ix: number; | ||
| timestamp: number; | ||
| message: string; | ||
| } | ||
|
|
||
| /** | ||
| * Error thrown when a MongoDB collection has been dropped unexpectedly. | ||
| */ | ||
| class CollectionDroppedError extends Error { | ||
| constructor (collectionName: string) { | ||
| super(`Collection ${collectionName} has been dropped.`); | ||
| this.name = "CollectionDroppedError"; | ||
| } | ||
| } | ||
|
|
||
| export { | ||
| CollectionDroppedError, SearchResultsDocument, | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| import settings from "@settings" with {type: "json"}; | ||
| import {FastifyInstance} from "fastify"; | ||
| import fp from "fastify-plugin"; | ||
|
|
||
| import type {SearchResultsMetadataDocument} from "./typings.js"; | ||
|
|
||
|
|
||
| /** | ||
| * Creates a MongoDB collection for search results metadata. | ||
| * | ||
| * @param fastify | ||
| * @return MongoDB collection | ||
| * @throws {Error} if the MongoDB database is not found. | ||
|
Comment on lines
+12
to
+13
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Enhance JSDoc return type documentation. The return type in the JSDoc comment could be more specific to help developers understand what's being returned. - * @return MongoDB collection
+ * @return {Collection<SearchResultsMetadataDocument>} MongoDB collection for search results metadata🤖 Prompt for AI Agents |
||
| */ | ||
| const createSearchResultsMetadataCollection = (fastify: FastifyInstance) => { | ||
| if ("undefined" === typeof fastify.mongo.db) { | ||
| throw new Error("MongoDB database not found"); | ||
| } | ||
|
|
||
| return fastify.mongo.db.collection<SearchResultsMetadataDocument>( | ||
| settings.MongoDbSearchResultsMetadataCollectionName | ||
| ); | ||
| }; | ||
|
|
||
| declare module "fastify" { | ||
| export interface FastifyInstance { | ||
| SearchResultsMetadataCollection: ReturnType<typeof createSearchResultsMetadataCollection>; | ||
| } | ||
| } | ||
|
|
||
| export default fp( | ||
| (fastify) => { | ||
| fastify.decorate( | ||
| "SearchResultsMetadataCollection", | ||
| createSearchResultsMetadataCollection(fastify) | ||
| ); | ||
| }, | ||
| { | ||
| name: "SearchResultsMetadataCollection", | ||
| } | ||
| ); | ||
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.
If we need to ignore files here, I'd prefer to import the plugins at the top of the file and register them individually. There are only three anyway.
We could also avoid
import.meta.dirnamethis wayUh oh!
There was an error while loading. Please reload this page.
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.
Instead, how about naming all plugin sources with
.plugin.tsextension and usingmatchFilterto load only those plugin sources?Uh oh!
There was an error while loading. Please reload this page.
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.
There will be more plugins when we refactor the old webui.
We dont explicitly need the filter. It will only load the index files in a directory. I thought maybe if someone adds a shared typings or utils without an index file. It was safer to add this.
m okay with junhao suggestion or just leaving it
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.
For now i will just remove it. I dont think it will change anything, if someone adds something which breaks it, they can fix then