-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat(be): extend departure endpoint #35
Changes from 3 commits
d95b14e
fdf0230
cf0fd43
49680f0
7498806
c24bde6
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 |
---|---|---|
|
@@ -10,12 +10,15 @@ import { ApiTags } from "@nestjs/swagger"; | |
import { z } from "zod"; | ||
|
||
import { QUERY_IDS_COUNT_MAX } from "src/constants/constants"; | ||
import { ApiQueries } from "src/decorators/swagger.decorator"; | ||
import { DepartureService } from "src/modules/departure/departure.service"; | ||
import { | ||
departureSchema, | ||
type DepartureSchema, | ||
} from "src/modules/departure/schema/departure.schema"; | ||
import { LogInterceptor } from "src/modules/logger/log.interceptor"; | ||
import { metroOnlySchema } from "src/schema/metro-only.schema"; | ||
import { metroOnlyQuery } from "src/swagger/query.swagger"; | ||
import { toArray } from "src/utils/array.utils"; | ||
|
||
@ApiTags("departure") | ||
|
@@ -24,6 +27,57 @@ import { toArray } from "src/utils/array.utils"; | |
export class DepartureController { | ||
constructor(private readonly departureService: DepartureService) {} | ||
|
||
@Get() | ||
@ApiQueries([ | ||
metroOnlyQuery, | ||
{ | ||
name: "platform[]", | ||
description: "Platform IDs", | ||
type: String, | ||
isArray: true, | ||
allowEmptyValue: true, | ||
required: false, | ||
}, | ||
{ | ||
name: "stop[]", | ||
description: "Stop IDs", | ||
type: String, | ||
isArray: true, | ||
allowEmptyValue: true, | ||
required: false, | ||
}, | ||
]) | ||
async getDepartures(@Query() query): Promise<DepartureSchema[]> { | ||
const schema = z.object({ | ||
metroOnly: metroOnlySchema, | ||
platform: z.string().array().optional().default([]), | ||
stop: z.string().array().optional().default([]), | ||
}); | ||
const parsed = schema.safeParse(query); | ||
if (!parsed.success) { | ||
throw new HttpException( | ||
"Invalid query params", | ||
HttpStatus.BAD_REQUEST, | ||
); | ||
Comment on lines
+58
to
+61
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. 🛠️ Refactor suggestion Include validation errors in the Currently, when validation fails, a generic error message is returned. Including detailed validation errors can help clients understand what went wrong with their request. Apply this diff to include validation details in the exception: if (!parsed.success) {
throw new HttpException(
- "Invalid query params",
+ parsed.error.errors,
HttpStatus.BAD_REQUEST,
);
} This change will return the specific validation errors encountered during parsing.
|
||
} | ||
const parsedQuery = parsed.data; | ||
|
||
if (parsedQuery.platform.length + parsedQuery.stop.length === 0) { | ||
throw new HttpException( | ||
"At least one platform or stop ID must be provided", | ||
HttpStatus.BAD_REQUEST, | ||
); | ||
} | ||
|
||
const departures = await this.departureService.getDepartures({ | ||
stopIds: parsedQuery.stop, | ||
platformIds: parsedQuery.platform, | ||
metroOnly: parsedQuery.metroOnly, | ||
}); | ||
|
||
return departureSchema.array().parse(departures); | ||
} | ||
|
||
@Get("/platform") | ||
async getDeparturesByPlatform(@Query("id") id): Promise<DepartureSchema[]> { | ||
const platformSchema = z | ||
|
@@ -40,9 +94,11 @@ export class DepartureController { | |
); | ||
} | ||
|
||
const departures = await this.departureService.getDeparturesByPlatform( | ||
parsed.data, | ||
); | ||
const departures = await this.departureService.getDepartures({ | ||
stopIds: [], | ||
platformIds: parsed.data, | ||
metroOnly: false, | ||
}); | ||
|
||
return departureSchema.array().parse(departures); | ||
} | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,4 +1,5 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { HttpException, HttpStatus, Injectable } from "@nestjs/common"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { unique } from "radash"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { GOLEMIO_API } from "src/constants/golemio.const"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { departureBoardsSchema } from "src/modules/departure/schema/departure-boards.schema"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -10,6 +11,84 @@ import { getDelayInSeconds } from "src/utils/delay"; | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
export class DepartureService { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
constructor(private prisma: PrismaService) {} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
async getDepartures(args: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
stopIds: string[]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
platformIds: string[]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
metroOnly: boolean; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}): Promise<DepartureSchema[]> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+14
to
+18
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. Add input validation for empty arrays. Consider validating the input arrays to ensure at least one of async getDepartures(args: {
stopIds: string[];
platformIds: string[];
metroOnly: boolean;
}): Promise<DepartureSchema[]> {
+ if (args.stopIds.length === 0 && args.platformIds.length === 0) {
+ throw new HttpException(
+ 'At least one stopId or platformId must be provided',
+ HttpStatus.BAD_REQUEST
+ );
+ } 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const dbPlatforms = ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
await this.prisma.platform.findMany({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
select: { id: true }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
where: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
id: { in: args.platformIds }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
...(args.metroOnly ? { isMetro: true } : {}), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
).map((platform) => platform.id); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const stopPlatforms = ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
await this.prisma.stop.findMany({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
select: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
platforms: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
select: { id: true }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
where: { ...(args.metroOnly ? { isMetro: true } : {}) }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
where: { id: { in: args.stopIds } }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
).flatMap((stop) => stop.platforms.map((platform) => platform.id)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const allPlatformIds = unique([...dbPlatforms, ...stopPlatforms]).slice( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
0, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
100, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+19
to
+42
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. 🛠️ Refactor suggestion Improve error handling and extract magic numbers. Several improvements could be made to this section:
+const MAX_PLATFORMS = 100; // Maximum number of platforms to query at once due to API limitations
+
async getDepartures(args: {
stopIds: string[];
platformIds: string[];
metroOnly: boolean;
}): Promise<DepartureSchema[]> {
+ try {
const dbPlatforms = (
await this.prisma.platform.findMany({
select: { id: true },
where: {
id: { in: args.platformIds },
...(args.metroOnly ? { isMetro: true } : {}),
},
})
).map((platform) => platform.id);
// ... rest of the code ...
- ).slice(0, 100);
+ ).slice(0, MAX_PLATFORMS);
+ } catch (error) {
+ throw new HttpException(
+ 'Failed to fetch platform data',
+ HttpStatus.INTERNAL_SERVER_ERROR
+ );
+ }
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const searchParams = new URLSearchParams( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
allPlatformIds | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.map((id) => ["ids", id]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.concat([ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
["skip", "canceled"], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
["mode", "departures"], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
["order", "real"], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
]), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const url = new URL( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
`${GOLEMIO_API}/v2/pid/departureboards?${searchParams.toString()}`, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+44
to
+56
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. 🛠️ Refactor suggestion Extract API query parameters as constants. Consider extracting the API query parameters to improve maintainability and reusability, especially since these parameters are also used in +const DEPARTURE_API_PARAMS = [
+ ['skip', 'canceled'],
+ ['mode', 'departures'],
+ ['order', 'real'],
+] as const;
+
const searchParams = new URLSearchParams(
allPlatformIds
.map((id) => ["ids", id])
- .concat([
- ["skip", "canceled"],
- ["mode", "departures"],
- ["order", "real"],
- ]),
+ .concat(DEPARTURE_API_PARAMS),
); 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const res = await fetch(url, { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
method: "GET", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
headers: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"Content-Type": "application/json", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"X-Access-Token": process.env.GOLEMIO_API_KEY ?? "", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (!res.ok) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
throw new Error( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
`Failed to fetch departure data: ${res.status} ${res.statusText}`, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const json = await res.json(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const parsed = departureBoardsSchema.safeParse(json); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (!parsed.success) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
throw new Error(parsed.error.message); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+58
to
+76
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. 🛠️ Refactor suggestion Enhance API call robustness. The API call could benefit from several improvements:
+private validateApiKey() {
+ if (!process.env.GOLEMIO_API_KEY) {
+ throw new Error('GOLEMIO_API_KEY environment variable is not set');
+ }
+}
+
+constructor(private prisma: PrismaService) {
+ this.validateApiKey();
+}
+
+private async fetchWithTimeout(url: URL, options: RequestInit, timeout = 5000) {
+ const controller = new AbortController();
+ const id = setTimeout(() => controller.abort(), timeout);
+ try {
+ const response = await fetch(url, {
+ ...options,
+ signal: controller.signal,
+ });
+ clearTimeout(id);
+ return response;
+ } catch (error) {
+ clearTimeout(id);
+ throw error;
+ }
+}
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const parsedDepartures = parsed.data.departures.map((departure) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
departure: departure.departure_timestamp, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
delay: getDelayInSeconds(departure.delay), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
headsign: departure.trip.headsign, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
route: departure.route.short_name, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
platformId: departure.stop.id, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
platformCode: departure.stop.platform_code, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return parsedDepartures; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+78
to
+90
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. 🛠️ Refactor suggestion Extract common departure parsing logic. The departure parsing logic is duplicated between +private parseDepartureResponse(data: unknown): DepartureSchema[] {
+ const parsed = departureBoardsSchema.safeParse(data);
+ if (!parsed.success) {
+ throw new Error(parsed.error.message);
+ }
+ return parsed.data.departures.map((departure) => ({
+ departure: departure.departure_timestamp,
+ delay: getDelayInSeconds(departure.delay),
+ headsign: departure.trip.headsign,
+ route: departure.route.short_name,
+ platformId: departure.stop.id,
+ platformCode: departure.stop.platform_code,
+ }));
+}
Comment on lines
+14
to
+90
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. 💡 Codebase verification E2e tests need to be updated for new endpoint functionality The existing e2e tests only cover the basic platform ID scenario and need to be extended to test:
Current test file location: 🔗 Analysis chainReminder: Add e2e tests as mentioned in PR objectives. The implementation successfully adds support for:
However, e2e tests were mentioned in the PR objectives but haven't been implemented yet. Would you like help creating e2e tests for the new functionality? 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Check if e2e tests exist for the departure endpoint
fd -e spec.ts -e test.ts departure apps/backend/test/e2e
Length of output: 167 Script: #!/bin/bash
# Let's first check if there are any e2e test directories in the backend
fd -t d -g "*e2e*" apps/backend
# Also search for any test files related to departures in the backend
fd -e spec.ts -e test.ts departure apps/backend
Length of output: 101 Script: #!/bin/bash
# Let's check the content of e2e directory to understand the test structure
ls -la apps/backend/e2e/
# Also check if there are any departure-related files that might have a different naming pattern
rg -l "departure" apps/backend/e2e/
Length of output: 332 Script: #!/bin/bash
# Let's check the content of the departure e2e test file
cat apps/backend/e2e/tests/departure.e2e-spec.ts
Length of output: 1736 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
async getDeparturesByPlatform( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
requestedIds: string[], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
): Promise<DepartureSchema[]> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -19,4 +19,19 @@ export class PrismaService | |||||||||||||||||||||||||
async onModuleDestroy() { | ||||||||||||||||||||||||||
await this.$disconnect(); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
async getExtensions(): Promise< | ||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||
oid: string | number; | ||||||||||||||||||||||||||
extname: string; | ||||||||||||||||||||||||||
}[] | ||||||||||||||||||||||||||
> { | ||||||||||||||||||||||||||
return await this.$queryRaw`SELECT * FROM pg_extension`; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
krystxf marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
async getExtensionNames(): Promise<string[]> { | ||||||||||||||||||||||||||
const extensions = await this.getExtensions(); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
return extensions.map((ext) => ext.extname); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
Comment on lines
+32
to
+36
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. Add null check and error handling for extension mapping. The mapping operation could fail if the extensions array is null or contains invalid data. Consider adding defensive programming: async getExtensionNames(): Promise<string[]> {
const extensions = await this.getExtensions();
-
- return extensions.map((ext) => ext.extname);
+ if (!extensions?.length) {
+ return [];
+ }
+ return extensions.map((ext) => ext.extname).filter(Boolean);
} 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,85 @@ | ||||||||||||||||||||||||||||||||||||||||||
import { Controller, Get } from "@nestjs/common"; | ||||||||||||||||||||||||||||||||||||||||||
import { ApiResponse, ApiTags } from "@nestjs/swagger"; | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
import { ApiDescription } from "src/decorators/swagger.decorator"; | ||||||||||||||||||||||||||||||||||||||||||
import { StatusService } from "src/modules/status/status.service"; | ||||||||||||||||||||||||||||||||||||||||||
import { | ||||||||||||||||||||||||||||||||||||||||||
StatusObject, | ||||||||||||||||||||||||||||||||||||||||||
SystemStatus, | ||||||||||||||||||||||||||||||||||||||||||
SystemStatusService, | ||||||||||||||||||||||||||||||||||||||||||
} from "src/modules/status/status.types"; | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
@ApiTags("status") | ||||||||||||||||||||||||||||||||||||||||||
@Controller("status") | ||||||||||||||||||||||||||||||||||||||||||
export class StatusController { | ||||||||||||||||||||||||||||||||||||||||||
constructor(private readonly statusService: StatusService) {} | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
@Get() | ||||||||||||||||||||||||||||||||||||||||||
@ApiDescription({ | ||||||||||||||||||||||||||||||||||||||||||
summary: "Backend status", | ||||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||||
@ApiResponse({ | ||||||||||||||||||||||||||||||||||||||||||
status: 200, | ||||||||||||||||||||||||||||||||||||||||||
isArray: true, | ||||||||||||||||||||||||||||||||||||||||||
example: [ | ||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||
service: SystemStatusService.BACKEND, | ||||||||||||||||||||||||||||||||||||||||||
status: SystemStatus.OK, | ||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||
service: SystemStatusService.GEO_FUNCTIONS, | ||||||||||||||||||||||||||||||||||||||||||
status: SystemStatus.OK, | ||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||
service: SystemStatusService.DB_DATA, | ||||||||||||||||||||||||||||||||||||||||||
status: SystemStatus.OK, | ||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||
], | ||||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||||
async getBackendStatus(): Promise<StatusObject[]> { | ||||||||||||||||||||||||||||||||||||||||||
return [ | ||||||||||||||||||||||||||||||||||||||||||
this.statusService.getBackendStatus(), | ||||||||||||||||||||||||||||||||||||||||||
await this.statusService.getGeoFunctionsStatus(), | ||||||||||||||||||||||||||||||||||||||||||
await this.statusService.getDbDataStatus(), | ||||||||||||||||||||||||||||||||||||||||||
]; | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+39
to
+45
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. Improve error handling and async operation consistency. The current implementation has several potential issues:
Consider this safer implementation: async getBackendStatus(): Promise<StatusObject[]> {
- return [
- this.statusService.getBackendStatus(),
- await this.statusService.getGeoFunctionsStatus(),
- await this.statusService.getDbDataStatus(),
- ];
+ try {
+ const [backendStatus, geoStatus, dbStatus] = await Promise.all([
+ Promise.resolve(this.statusService.getBackendStatus()),
+ this.statusService.getGeoFunctionsStatus(),
+ this.statusService.getDbDataStatus(),
+ ]);
+ return [backendStatus, geoStatus, dbStatus];
+ } catch (error) {
+ // Consider creating a custom exception filter
+ throw new Error(`Failed to fetch system status: ${error.message}`);
+ }
} 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
@Get("/geo-functions") | ||||||||||||||||||||||||||||||||||||||||||
@ApiDescription({ summary: "Geo functions status" }) | ||||||||||||||||||||||||||||||||||||||||||
@ApiResponse({ | ||||||||||||||||||||||||||||||||||||||||||
status: 200, | ||||||||||||||||||||||||||||||||||||||||||
example: { | ||||||||||||||||||||||||||||||||||||||||||
service: SystemStatusService.GEO_FUNCTIONS, | ||||||||||||||||||||||||||||||||||||||||||
status: SystemStatus.OK, | ||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||||
async getPlatformsByDistance(): Promise<StatusObject> { | ||||||||||||||||||||||||||||||||||||||||||
const geoFunctionsStatus = | ||||||||||||||||||||||||||||||||||||||||||
await this.statusService.getGeoFunctionsStatus(); | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
if (geoFunctionsStatus.status === "error") { | ||||||||||||||||||||||||||||||||||||||||||
throw new Error("Geo functions status is not OK"); | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+60
to
+62
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. Improve error handling robustness. The current error handling uses string literal comparison and throws a generic Error. Consider using:
-if (geoFunctionsStatus.status === "error") {
- throw new Error("Geo functions status is not OK");
+if (geoFunctionsStatus.status === SystemStatus.ERROR) {
+ throw new ServiceException(
+ `Geo functions check failed: ${geoFunctionsStatus.message || 'Unknown error'}`,
+ 'GEO_FUNCTIONS_ERROR'
+ );
|
||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
return geoFunctionsStatus; | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+56
to
+65
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. Method name doesn't match its functionality. The method name Rename the method to match its functionality: -async getPlatformsByDistance(): Promise<StatusObject> {
+async getGeoFunctionsStatus(): Promise<StatusObject> { 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
@Get("/db-data") | ||||||||||||||||||||||||||||||||||||||||||
@ApiDescription({ summary: "DB data status" }) | ||||||||||||||||||||||||||||||||||||||||||
@ApiResponse({ | ||||||||||||||||||||||||||||||||||||||||||
status: 200, | ||||||||||||||||||||||||||||||||||||||||||
example: { | ||||||||||||||||||||||||||||||||||||||||||
service: SystemStatusService.DB_DATA, | ||||||||||||||||||||||||||||||||||||||||||
status: SystemStatus.OK, | ||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||||
async getDbDataStatus(): Promise<StatusObject> { | ||||||||||||||||||||||||||||||||||||||||||
const dbStatus = await this.statusService.getDbDataStatus(); | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
if (dbStatus.status === "error") { | ||||||||||||||||||||||||||||||||||||||||||
throw new Error("DB data status is not OK"); | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
return dbStatus; | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,11 @@ | ||||||||||
import { Module } from "@nestjs/common"; | ||||||||||
|
||||||||||
import { StatusController } from "src/modules/status/status.controller"; | ||||||||||
import { StatusService } from "src/modules/status/status.service"; | ||||||||||
Comment on lines
+3
to
+4
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. Use relative import paths instead of src-based paths. Using "src/" in import paths can cause issues with build tools and make the code less maintainable. Consider using relative paths instead. Apply this diff to fix the imports: -import { StatusController } from "src/modules/status/status.controller";
-import { StatusService } from "src/modules/status/status.service";
+import { StatusController } from "./status.controller";
+import { StatusService } from "./status.service"; 📝 Committable suggestion
Suggested change
|
||||||||||
|
||||||||||
@Module({ | ||||||||||
controllers: [StatusController], | ||||||||||
providers: [StatusService], | ||||||||||
imports: [], | ||||||||||
}) | ||||||||||
export class StatusModule {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
import { Injectable } from "@nestjs/common"; | ||
|
||
import { PrismaService } from "src/modules/prisma/prisma.service"; | ||
import { | ||
StatusObject, | ||
SystemStatus, | ||
SystemStatusService, | ||
} from "src/modules/status/status.types"; | ||
|
||
@Injectable() | ||
export class StatusService { | ||
constructor(private prisma: PrismaService) {} | ||
Comment on lines
+1
to
+12
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. 🛠️ Refactor suggestion Consider implementing an interface for the status service. The service has well-defined status check responsibilities that could be abstracted into an interface. This would make it easier to maintain, test, and potentially swap implementations. interface IStatusService {
getBackendStatus(): StatusObject;
getGeoFunctionsStatus(): Promise<StatusObject>;
getDbDataStatus(): Promise<StatusObject>;
}
@Injectable()
export class StatusService implements IStatusService {
// ... existing implementation
} |
||
|
||
getBackendStatus(): StatusObject { | ||
return { | ||
service: SystemStatusService.BACKEND, | ||
status: SystemStatus.OK, | ||
}; | ||
} | ||
Comment on lines
+14
to
+19
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. Enhance backend health check implementation. The current implementation always returns OK without performing any actual health checks. Consider adding real health metrics such as:
Consider using NestJS's built-in health checks: import { HealthCheckService, HealthIndicator } from '@nestjs/terminus';
@Injectable()
export class StatusService {
constructor(
private health: HealthCheckService,
private prisma: PrismaService
) {}
async getBackendStatus(): Promise<StatusObject> {
const healthCheck = await this.health.check([]);
return {
service: SystemStatusService.BACKEND,
status: healthCheck.status === 'ok' ? SystemStatus.OK : SystemStatus.ERROR,
};
}
} |
||
|
||
async getGeoFunctionsStatus(): Promise<StatusObject> { | ||
const extensionNames = await this.prisma.getExtensionNames(); | ||
const isOk = | ||
extensionNames.includes("cube") && | ||
extensionNames.includes("earthdistance"); | ||
|
||
return { | ||
service: SystemStatusService.GEO_FUNCTIONS, | ||
status: isOk ? SystemStatus.OK : SystemStatus.ERROR, | ||
}; | ||
} | ||
Comment on lines
+21
to
+31
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. 🛠️ Refactor suggestion Add error handling and consider caching extension checks. The extension check is good but could be enhanced with proper error handling and caching to prevent frequent database queries. private extensionCheckCache: { value: boolean; timestamp: number } | null = null;
private readonly CACHE_DURATION = 5 * 60 * 1000; // 5 minutes
async getGeoFunctionsStatus(): Promise<StatusObject> {
try {
const now = Date.now();
if (this.extensionCheckCache &&
now - this.extensionCheckCache.timestamp < this.CACHE_DURATION) {
return {
service: SystemStatusService.GEO_FUNCTIONS,
status: this.extensionCheckCache.value ? SystemStatus.OK : SystemStatus.ERROR,
};
}
const extensionNames = await this.prisma.getExtensionNames();
const isOk = extensionNames.includes("cube") &&
extensionNames.includes("earthdistance");
this.extensionCheckCache = { value: isOk, timestamp: now };
return {
service: SystemStatusService.GEO_FUNCTIONS,
status: isOk ? SystemStatus.OK : SystemStatus.ERROR,
};
} catch (error) {
return {
service: SystemStatusService.GEO_FUNCTIONS,
status: SystemStatus.ERROR,
error: 'Failed to check database extensions',
};
}
} |
||
|
||
async getDbDataStatus(): Promise<StatusObject> { | ||
const stopCount = await this.prisma.stop.count(); | ||
const routeCount = await this.prisma.route.count(); | ||
const platformCount = await this.prisma.platform.count(); | ||
|
||
const isOk = stopCount > 0 && routeCount > 0 && platformCount > 0; | ||
|
||
return { | ||
service: SystemStatusService.DB_DATA, | ||
status: isOk ? SystemStatus.OK : SystemStatus.ERROR, | ||
}; | ||
} | ||
Comment on lines
+33
to
+44
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. 🛠️ Refactor suggestion Optimize database checks and add error handling. The current implementation makes multiple separate database calls and lacks error handling. Consider using a single query and adding proper error handling. async getDbDataStatus(): Promise<StatusObject> {
try {
const counts = await this.prisma.$transaction([
this.prisma.stop.count(),
this.prisma.route.count(),
this.prisma.platform.count(),
]);
const [stopCount, routeCount, platformCount] = counts;
const isOk = stopCount > 0 && routeCount > 0 && platformCount > 0;
return {
service: SystemStatusService.DB_DATA,
status: isOk ? SystemStatus.OK : SystemStatus.ERROR,
details: {
stops: stopCount,
routes: routeCount,
platforms: platformCount,
},
};
} catch (error) {
return {
service: SystemStatusService.DB_DATA,
status: SystemStatus.ERROR,
error: 'Failed to check database data',
};
}
} |
||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,15 @@ | ||||||||||||||||||||||||
export enum SystemStatusService { | ||||||||||||||||||||||||
BACKEND = "backend", | ||||||||||||||||||||||||
GEO_FUNCTIONS = "geo-functions", | ||||||||||||||||||||||||
DB_DATA = "db-data", | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
export enum SystemStatus { | ||||||||||||||||||||||||
OK = "ok", | ||||||||||||||||||||||||
ERROR = "error", | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
Comment on lines
+7
to
+10
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. 🛠️ Refactor suggestion Consider adding more granular status states. A binary OK/ERROR status might be too simplistic for real-world scenarios. Consider adding intermediate states to better represent system health. export enum SystemStatus {
OK = "ok",
+ DEGRADED = "degraded",
+ MAINTENANCE = "maintenance",
+ UNKNOWN = "unknown",
ERROR = "error",
} 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||
|
||||||||||||||||||||||||
export type StatusObject = { | ||||||||||||||||||||||||
service: SystemStatusService; | ||||||||||||||||||||||||
status: SystemStatus; | ||||||||||||||||||||||||
}; | ||||||||||||||||||||||||
Comment on lines
+12
to
+15
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. 🛠️ Refactor suggestion Consider adding timestamp and message fields. The status object could be more informative by including a timestamp and an optional message field for error details or additional information. export type StatusObject = {
service: SystemStatusService;
status: SystemStatus;
+ timestamp: Date;
+ message?: string;
}; 📝 Committable suggestion
Suggested change
|
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.
Remove '[]' from query parameter names to avoid parsing issues
Using brackets in the query parameter names (
"platform[]"
and"stop[]"
) can lead to unexpected behavior in parameter parsing. It's standard practice to name the parameters without brackets when usingisArray: true
.Apply this diff to correct the parameter names and improve consistency:
📝 Committable suggestion