From a1aab684614b7af97b80e87cdf07bfdf3910e8a6 Mon Sep 17 00:00:00 2001 From: Carlos Monastyrski Date: Mon, 15 Sep 2025 16:15:11 -0300 Subject: [PATCH] Address greptile comments --- .../server/routes/v1/upgrade-path-router.ts | 29 ++---------- .../upgrade-path/upgrade-path-service.ts | 32 ++++++++------ frontend/src/hooks/api/upgradePath/index.ts | 7 +-- .../src/hooks/api/upgradePath/queries.tsx | 34 ++++++-------- .../UpgradePathPage/UpgradePathPage.tsx | 44 +++++++++---------- 5 files changed, 58 insertions(+), 88 deletions(-) diff --git a/backend/src/server/routes/v1/upgrade-path-router.ts b/backend/src/server/routes/v1/upgrade-path-router.ts index 30b5e9561..e431e60d5 100644 --- a/backend/src/server/routes/v1/upgrade-path-router.ts +++ b/backend/src/server/routes/v1/upgrade-path-router.ts @@ -9,16 +9,6 @@ const versionSchema = z .min(1) .max(50) .regex(new RE2(/^[a-zA-Z0-9._/-]+$/), "Invalid version format"); -const booleanSchema = z.boolean().default(false); -const queryBooleanSchema = z - .union([z.boolean(), z.string()]) - .transform((val) => { - if (typeof val === "string") { - return val === "true" || val === "1"; - } - return val; - }) - .default(false); export const registerUpgradePathRouter = async (server: FastifyZodProvider) => { server.route({ @@ -28,9 +18,6 @@ export const registerUpgradePathRouter = async (server: FastifyZodProvider) => { rateLimit: publicEndpointLimit }, schema: { - querystring: z.object({ - includePrerelease: queryBooleanSchema - }), response: { 200: z.object({ versions: z.array( @@ -47,8 +34,7 @@ export const registerUpgradePathRouter = async (server: FastifyZodProvider) => { }, handler: async (req) => { try { - const { includePrerelease } = req.query; - const versions = await req.server.services.upgradePath.getGitHubReleases(includePrerelease); + const versions = await req.server.services.upgradePath.getGitHubReleases(); return { versions @@ -72,8 +58,7 @@ export const registerUpgradePathRouter = async (server: FastifyZodProvider) => { schema: { body: z.object({ fromVersion: versionSchema, - toVersion: versionSchema, - includePrerelease: booleanSchema + toVersion: versionSchema }), response: { 200: z.object({ @@ -112,15 +97,9 @@ export const registerUpgradePathRouter = async (server: FastifyZodProvider) => { }, handler: async (req) => { try { - const { fromVersion, toVersion, includePrerelease } = req.body; + const { fromVersion, toVersion } = req.body; - req.log.info({ fromVersion, toVersion, includePrerelease }, "Calculating upgrade path"); - - const result = await req.server.services.upgradePath.calculateUpgradePath( - fromVersion, - toVersion, - includePrerelease - ); + const result = await req.server.services.upgradePath.calculateUpgradePath(fromVersion, toVersion); req.log.info( { pathLength: result.path.length, hasBreaking: result.breakingChanges.length > 0 }, diff --git a/backend/src/services/upgrade-path/upgrade-path-service.ts b/backend/src/services/upgrade-path/upgrade-path-service.ts index 05bd44e25..f966e670e 100644 --- a/backend/src/services/upgrade-path/upgrade-path-service.ts +++ b/backend/src/services/upgrade-path/upgrade-path-service.ts @@ -19,17 +19,15 @@ const versionSchema = z .min(1) .max(50) .regex(new RE2(/^[a-zA-Z0-9._/-]+$/), "Invalid version format"); -const booleanSchema = z.boolean().default(false); interface CalculateUpgradePathParams { fromVersion: string; toVersion: string; - includePrerelease?: boolean; } export const upgradePathServiceFactory = ({ keyStore }: TUpgradePathServiceFactory) => { - const getGitHubReleases = async (includePrerelease = false): Promise => { - const cacheKey = `upgrade-path:releases:${includePrerelease}`; + const getGitHubReleases = async (): Promise => { + const cacheKey = "upgrade-path:releases"; try { const cached = await keyStore.getItem(cacheKey); @@ -39,7 +37,7 @@ export const upgradePathServiceFactory = ({ keyStore }: TUpgradePathServiceFacto } try { - const releases = await fetchReleases(booleanSchema.parse(includePrerelease)); + const releases = await fetchReleases(false); const filteredReleases = releases.filter((v) => !v.tagName.includes("nightly")); @@ -62,13 +60,19 @@ export const upgradePathServiceFactory = ({ keyStore }: TUpgradePathServiceFacto try { const yamlPath = path.join(__dirname, "..", "..", "..", "upgrade-path.yaml"); + const resolvedPath = path.resolve(yamlPath); + const expectedBaseDir = path.resolve(__dirname, "..", "..", ".."); + if (!resolvedPath.startsWith(expectedBaseDir)) { + throw new Error("Invalid configuration file path"); + } + const yamlContent = await readFile(yamlPath, "utf8"); if (yamlContent.length > 1024 * 1024) { throw new Error("Config file too large"); } - const config = yaml.load(yamlContent) as UpgradePathConfig; + const config = yaml.load(yamlContent, { schema: yaml.FAILSAFE_SCHEMA }) as UpgradePathConfig; const versionConfig = config?.versions || {}; await keyStore.setItemWithExpiry(cacheKey, 24 * 60 * 60, JSON.stringify(versionConfig)); @@ -85,7 +89,8 @@ export const upgradePathServiceFactory = ({ keyStore }: TUpgradePathServiceFacto const normalizeVersion = (version: string): string => { // Extract just the X.X.X.X part from any version format - const versionMatch = version.match(/(\d+\.\d+\.\d+(?:\.\d+)?)/); + const versionRegex = new RE2(/(\d+\.\d+\.\d+(?:\.\d+)?)/); + const versionMatch = version.match(versionRegex); if (versionMatch) { return versionMatch[1]; } @@ -122,7 +127,7 @@ export const upgradePathServiceFactory = ({ keyStore }: TUpgradePathServiceFacto }; const validateParams = (params: CalculateUpgradePathParams) => { - const { fromVersion, toVersion, includePrerelease = false } = params; + const { fromVersion, toVersion } = params; versionSchema.parse(fromVersion); versionSchema.parse(toVersion); @@ -135,12 +140,12 @@ export const upgradePathServiceFactory = ({ keyStore }: TUpgradePathServiceFacto throw new Error("Nightly releases are not supported for upgrade path calculation"); } - return { fromVersion, toVersion, includePrerelease: booleanSchema.parse(includePrerelease) }; + return { fromVersion, toVersion }; }; const calculateUpgradePath = async (params: CalculateUpgradePathParams): Promise => { - const { fromVersion, toVersion, includePrerelease } = validateParams(params); - const cacheKey = `upgrade-path:${fromVersion}:${toVersion}:${includePrerelease}`; + const { fromVersion, toVersion } = validateParams(params); + const cacheKey = `upgrade-path:${fromVersion}:${toVersion}`; try { const cached = await keyStore.getItem(cacheKey); @@ -149,7 +154,7 @@ export const upgradePathServiceFactory = ({ keyStore }: TUpgradePathServiceFacto // Cache miss, continue to fetch from source } - const [releases, config] = await Promise.all([getGitHubReleases(includePrerelease), getUpgradePathConfig()]); + const [releases, config] = await Promise.all([getGitHubReleases(), getUpgradePathConfig()]); const cleanFrom = normalizeVersion(fromVersion); const cleanTo = normalizeVersion(toVersion); @@ -264,7 +269,6 @@ export const upgradePathServiceFactory = ({ keyStore }: TUpgradePathServiceFacto return { getGitHubReleases, getUpgradePathConfig, - calculateUpgradePath: (fromVersion: string, toVersion: string, includePrerelease = false) => - calculateUpgradePath({ fromVersion, toVersion, includePrerelease }) + calculateUpgradePath: (fromVersion: string, toVersion: string) => calculateUpgradePath({ fromVersion, toVersion }) }; }; diff --git a/frontend/src/hooks/api/upgradePath/index.ts b/frontend/src/hooks/api/upgradePath/index.ts index 73c6d1254..d0eaefdb5 100644 --- a/frontend/src/hooks/api/upgradePath/index.ts +++ b/frontend/src/hooks/api/upgradePath/index.ts @@ -1,7 +1,2 @@ -export type { - CalculateUpgradePathParams, - GetUpgradePathVersionsParams, - GitHubVersion, - UpgradePathResult -} from "./queries"; +export type { CalculateUpgradePathParams, GitHubVersion, UpgradePathResult } from "./queries"; export { useCalculateUpgradePath, useGetUpgradePathVersions } from "./queries"; diff --git a/frontend/src/hooks/api/upgradePath/queries.tsx b/frontend/src/hooks/api/upgradePath/queries.tsx index aa4475c74..d7e8d705e 100644 --- a/frontend/src/hooks/api/upgradePath/queries.tsx +++ b/frontend/src/hooks/api/upgradePath/queries.tsx @@ -1,4 +1,4 @@ -import { useQuery, UseQueryOptions } from "@tanstack/react-query"; +import { useMutation, useQuery, UseQueryOptions } from "@tanstack/react-query"; import { apiRequest } from "@app/config/request"; @@ -35,36 +35,26 @@ export interface UpgradePathResult { config: Record; } -export interface GetUpgradePathVersionsParams { - includePrerelease?: boolean; -} - export interface CalculateUpgradePathParams { fromVersion: string; toVersion: string; - includePrerelease?: boolean; } const upgradePathKeys = { all: ["upgrade-path"] as const, - versions: (params: GetUpgradePathVersionsParams) => - [...upgradePathKeys.all, "versions", params] as const, + versions: () => [...upgradePathKeys.all, "versions"] as const, calculate: (params: CalculateUpgradePathParams) => [...upgradePathKeys.all, "calculate", params] as const }; export const useGetUpgradePathVersions = ( - params: GetUpgradePathVersionsParams, options?: Omit, "queryKey" | "queryFn"> ) => { return useQuery({ - queryKey: upgradePathKeys.versions(params), + queryKey: upgradePathKeys.versions(), queryFn: async () => { const { data } = await apiRequest.get<{ versions: GitHubVersion[] }>( - "/api/v1/upgrade-path/versions", - { - params - } + "/api/v1/upgrade-path/versions" ); return data; }, @@ -73,11 +63,13 @@ export const useGetUpgradePathVersions = ( }; export const useCalculateUpgradePath = () => { - return async (params: CalculateUpgradePathParams): Promise => { - const { data } = await apiRequest.post( - "/api/v1/upgrade-path/calculate", - params - ); - return data; - }; + return useMutation({ + mutationFn: async (params: CalculateUpgradePathParams): Promise => { + const { data } = await apiRequest.post( + "/api/v1/upgrade-path/calculate", + params + ); + return data; + } + }); }; diff --git a/frontend/src/pages/public/UpgradePathPage/UpgradePathPage.tsx b/frontend/src/pages/public/UpgradePathPage/UpgradePathPage.tsx index 74317d02f..ff1f60530 100644 --- a/frontend/src/pages/public/UpgradePathPage/UpgradePathPage.tsx +++ b/frontend/src/pages/public/UpgradePathPage/UpgradePathPage.tsx @@ -1,10 +1,9 @@ /* eslint-disable no-nested-ternary */ -import { useMemo, useState } from "react"; +import React, { useMemo, useState } from "react"; import { Helmet } from "react-helmet"; import { SingleValue } from "react-select"; import { faExternalLink } from "@fortawesome/free-solid-svg-icons"; import { FontAwesomeIcon } from "@fortawesome/react-fontawesome"; -import { useMutation } from "@tanstack/react-query"; import { createNotification } from "@app/components/notifications"; import { Button, FilterableSelect, FormControl } from "@app/components/v2"; @@ -60,29 +59,31 @@ export const UpgradePathPage = () => { data: versions, isLoading: versionsLoading, isFetching: versionsFetching - } = useGetUpgradePathVersions( - { - includePrerelease: false - }, - { - enabled: true, - staleTime: 24 * 60 * 60 * 1000, - refetchOnWindowFocus: false - } - ); + } = useGetUpgradePathVersions({ + enabled: true, + staleTime: 24 * 60 * 60 * 1000, + refetchOnWindowFocus: false + }); - const calculateMutation = useMutation({ - mutationFn: useCalculateUpgradePath(), - onSuccess: (data) => { - setUpgradeResult(data); - }, - onError: (error: unknown) => { + const calculateMutation = useCalculateUpgradePath(); + + // Handle mutation results + React.useEffect(() => { + if (calculateMutation.isSuccess && calculateMutation.data) { + setUpgradeResult(calculateMutation.data); + } + }, [calculateMutation.isSuccess, calculateMutation.data]); + + React.useEffect(() => { + if (calculateMutation.isError) { createNotification({ - text: (error as any)?.response?.data?.message || "Failed to calculate upgrade path", + text: + (calculateMutation.error as any)?.response?.data?.message || + "Failed to calculate upgrade path", type: "error" }); } - }); + }, [calculateMutation.isError, calculateMutation.error]); const versionOptions = useMemo(() => { if (!versions?.versions) return []; @@ -125,8 +126,7 @@ export const UpgradePathPage = () => { calculateMutation.mutate({ fromVersion, - toVersion, - includePrerelease: false + toVersion }); };