Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
43 changes: 43 additions & 0 deletions components/webui/client/src/api/presto-search.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
import {
Static,
Type,
} from "@sinclair/typebox";
import {Value} from "@sinclair/typebox/value";
import axios from "axios";


// eslint-disable-next-line no-warning-comments
// TODO: Replace with shared type from the `@common` directory once refactoring is completed.
// Currently, server schema types require typebox dependency so they cannot be moved to the
// `@common` directory with current implementation.
const StringSchema = Type.String({
minLength: 1,
});

// eslint-disable-next-line @typescript-eslint/no-unused-vars
const PrestoSearchJobCreationSchema = Type.Object({
queryString: StringSchema,
});

type PrestoSearchJobCreation = Static<typeof PrestoSearchJobCreationSchema>;

const PrestoJobSchema = Type.Object(
{
searchJobId: StringSchema,
}
);

type PrestoJob = Static<typeof PrestoJobSchema>;

/**
* Sends post request to server tosubmit presto query.
*
* @param payload
* @return
*/
const submitQuery = async (payload: PrestoSearchJobCreation): Promise<PrestoJob> => {
const ret = await axios.post<PrestoJob>("/api/presto-search/query", payload);
return Value.Parse(PrestoJobSchema, ret.data);
};

export {submitQuery};
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
import {useCallback} from "react";

import {CaretRightOutlined} from "@ant-design/icons";
import {
Button,
Tooltip,
} from "antd";

import {submitQuery} from "../../../../../api/presto-search";
import useSearchStore from "../../../SearchState/index";


Expand All @@ -20,6 +23,17 @@ const RunButton = () => {
"Enter SQL query to run" :
"";

const handleClick = useCallback(() => {
submitQuery({queryString})
.then(({searchJobId}) => {
const {updateSearchJobId} = useSearchStore.getState();
updateSearchJobId(searchJobId);
})
.catch((err: unknown) => {
console.error("Failed to submit query:", err);
});
}, [queryString]);

return (
<Tooltip title={tooltipTitle}>
<Button
Expand All @@ -28,6 +42,7 @@ const RunButton = () => {
icon={<CaretRightOutlined/>}
size={"large"}
variant={"solid"}
onClick={handleClick}
>
Run
</Button>
Expand Down
38 changes: 38 additions & 0 deletions components/webui/server/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion components/webui/server/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@
"@aws-sdk/s3-request-presigner": "^3.758.0",
"@fastify/autoload": "^6.3.0",
"@fastify/env": "^5.0.2",
"@fastify/http-proxy": "^11.3.0",
"@fastify/mongodb": "^9.0.2",
"@fastify/mysql": "^5.0.2",
"@fastify/rate-limit": "^10.2.2",
"@fastify/http-proxy": "^11.3.0",
"@fastify/sensible": "^6.0.3",
"@fastify/static": "^8.1.1",
"@fastify/type-provider-typebox": "^5.1.0",
Expand All @@ -38,10 +38,12 @@
"fastify-plugin": "^5.0.1",
"http-status-codes": "^2.3.0",
"pino-pretty": "^13.0.0",
"presto-client": "^1.1.0",
"socket.io": "^4.8.1",
"typescript": "~5.7.3"
},
"devDependencies": {
"@types/presto-client": "^1.0.2",
"concurrently": "^9.1.2",
"eslint-config-yscope": "latest",
"fastify-cli": "^7.4.0",
Expand Down
6 changes: 5 additions & 1 deletion components/webui/server/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,9 @@
"StreamTargetUncompressedSize": 134217728,
"StreamFilesS3Region": null,
"StreamFilesS3PathPrefix": null,
"StreamFilesS3Profile": null
"StreamFilesS3Profile": null,

"EnablePresto": true,
"PrestoHost": "localhost",
"PrestoPort": 8080
}
89 changes: 89 additions & 0 deletions components/webui/server/src/routes/api/presto-search/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
import {FastifyPluginAsyncTypebox} from "@fastify/type-provider-typebox";
import {StatusCodes} from "http-status-codes";
import {Client} from "presto-client";

import settings from "../../../../settings.json" with {type: "json"};
import {ErrorSchema} from "../../../schemas/error.js";
import {
PrestoJobSchema,
PrestoSearchJobCreationSchema,
} from "../../../schemas/presto-search.js";
import {Nullable} from "../../../typings/common.js";


/**
* Presto search API routes.
*
* @param fastify
*/
const plugin: FastifyPluginAsyncTypebox = async (fastify) => {
if (false === settings.EnablePresto) {
return;
}

const client = new Client({host: settings.PrestoHost, port: settings.PrestoPort});

Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add validation for Presto configuration settings.

The code should validate that PrestoHost and PrestoPort are properly configured before creating the client. Consider adding checks to ensure these settings exist and are valid.

 const plugin: FastifyPluginAsyncTypebox = async (fastify) => {
     if (false === settings.EnablePresto) {
         return;
     }
 
+    if (!settings.PrestoHost || !settings.PrestoPort) {
+        throw new Error("Presto is enabled but PrestoHost or PrestoPort is not configured");
+    }
+
     const client = new Client({host: settings.PrestoHost, port: settings.PrestoPort});
📝 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.

Suggested change
const plugin: FastifyPluginAsyncTypebox = async (fastify) => {
if (false === settings.EnablePresto) {
return;
}
const client = new Client({host: settings.PrestoHost, port: settings.PrestoPort});
const plugin: FastifyPluginAsyncTypebox = async (fastify) => {
if (false === settings.EnablePresto) {
return;
}
if (!settings.PrestoHost || !settings.PrestoPort) {
throw new Error("Presto is enabled but PrestoHost or PrestoPort is not configured");
}
const client = new Client({host: settings.PrestoHost, port: settings.PrestoPort});
// …rest of plugin implementation…
}
🤖 Prompt for AI Agents
In components/webui/server/src/routes/api/presto-search/index.ts around lines 19
to 25, the code creates a Presto client without validating the configuration
settings. Add validation checks to ensure that settings.PrestoHost is a
non-empty string and settings.PrestoPort is a valid number before creating the
client. If either setting is missing or invalid, log an error or return early to
prevent creating the client with invalid configuration.

/**
* Submits a search query and initiates the search process.
*/
fastify.post(
"/query",
{
schema: {
body: PrestoSearchJobCreationSchema,
response: {
[StatusCodes.CREATED]: PrestoJobSchema,
[StatusCodes.INTERNAL_SERVER_ERROR]: ErrorSchema,
},
tags: ["Search"],
},
},

async (request, reply) => {
const {queryString} = request.body;

let searchJobId: Nullable<string> = null;

searchJobId = await new Promise((resolve) => {
client.execute({
data: (error, data, columns) => {
if (null === searchJobId) {
request.log.error("Data arrived from Presto before the search job id.");
}
request.log.info({error, searchJobId, columns, data}, "Presto data");
},
error: (error) => {
request.log.info(error, "Presto search failed");

// The `/query` endpoint will catch this error if `resolve` hasn't
// been called.
throw new Error("Presto search failed.");
},
query: queryString,
state: (error, newSearchJobId, stats) => {
request.log.info({
error: error,
searchJobId: newSearchJobId,
state: stats.state,
}, "Presto search state updated");

resolve(newSearchJobId);
},
success: () => {
request.log.info("Presto search succeeded");
},
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Critical: Improve error handling and async flow in Presto callbacks.

The current implementation has several issues:

  1. The error thrown in the error callback (line 60) won't be caught by the Promise
  2. The Promise resolves immediately when state is updated, not when the query completes
  3. No handling for the case where the query completes without calling the state callback

Consider refactoring to properly handle all callback scenarios:

-            searchJobId = await new Promise((resolve) => {
+            searchJobId = await new Promise((resolve, reject) => {
                 client.execute({
                     data: (error, data, columns) => {
+                        if (error) {
+                            request.log.error({error}, "Error in data callback");
+                            return;
+                        }
                         if (null === searchJobId) {
                             request.log.error("Data arrived from Presto before the search job id.");
                         }
                         request.log.info({error, searchJobId, columns, data}, "Presto data");
                     },
                     error: (error) => {
                         request.log.info(error, "Presto search failed");
-
-                        // The `/query` endpoint will catch this error if `resolve` hasn't
-                        // been called.
-                        throw new Error("Presto search failed.");
+                        reject(new Error("Presto search failed."));
                     },
                     query: queryString,
                     state: (error, newSearchJobId, stats) => {
+                        if (error) {
+                            request.log.error({error}, "Error in state callback");
+                            reject(error);
+                            return;
+                        }
                         request.log.info({
                             error: error,
                             searchJobId: newSearchJobId,
                             state: stats.state,
                         }, "Presto search state updated");
 
-                        resolve(newSearchJobId);
+                        // Only set searchJobId once
+                        if (null === searchJobId && newSearchJobId) {
+                            searchJobId = newSearchJobId;
+                        }
+                        
+                        // Don't resolve here - wait for success or error
                     },
                     success: () => {
                         request.log.info("Presto search succeeded");
+                        resolve(searchJobId);
                     },
                 });
             });
🤖 Prompt for AI Agents
In components/webui/server/src/routes/api/presto-search/index.ts between lines
47 and 76, the Promise handling Presto client callbacks has issues: throwing an
error inside the error callback does not reject the Promise, the Promise
resolves prematurely on state update instead of query completion, and there is
no handling for query completion without state callback. To fix this, refactor
the Promise to reject on error by calling reject with the error, resolve only
when the success callback is invoked indicating query completion, and ensure all
possible callback scenarios (error, state, success) properly trigger resolve or
reject to maintain correct async flow.


reply.code(StatusCodes.CREATED);

if (null === searchJobId) {
throw new Error("searchJobId is null");
}

return {searchJobId};
}
);
};

export default plugin;
20 changes: 20 additions & 0 deletions components/webui/server/src/schemas/presto-search.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import {Type} from "@sinclair/typebox";

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


/**
* Schema for request to create a new query job.
*/
const PrestoSearchJobCreationSchema = Type.Object({
queryString: StringSchema,
});

const PrestoJobSchema = Type.Object({
searchJobId: StringSchema,
});

export {
PrestoJobSchema,
PrestoSearchJobCreationSchema,
};
Loading