Ref(api-specification): refactoring folder structure for occupation group relations api-specification definetion#483
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the api-specifications for OccupationGroup relation endpoints by splitting relation-specific pieces into relations/parent and relations/children folders and updating imports/exports accordingly.
Changes:
- Update relation schema/test imports to reflect the new
relations/*folder structure. - Add new relation-specific enum modules (
relations/parent/enum.ts,relations/children/enum.ts) and export them fromoccupationGroup/index.ts. - Update the occupationGroup module export snapshot to include the new enum exports.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/globalConfig.json | Adds a hard-coded local MongoDB URI config file. |
| api-specifications/src/esco/occupationGroup/relations/parent/schema.GET.parent.response.ts | Fixes _baseResponseSchema import path after moving under relations/parent. |
| api-specifications/src/esco/occupationGroup/relations/parent/schema.GET.parent.response.test.ts | Updates imports to reference occupationGroup root module/enums/regex after refactor. |
| api-specifications/src/esco/occupationGroup/relations/parent/enum.ts | Introduces parent-relation enum namespace and error codes. |
| api-specifications/src/esco/occupationGroup/relations/children/schema.GET.child.response.ts | Fixes _baseChildrenResponseSchema import path after moving under relations/children. |
| api-specifications/src/esco/occupationGroup/relations/children/schema.GET.child.response.test.ts | Updates imports to reference occupationGroup root module/enums/regex after refactor. |
| api-specifications/src/esco/occupationGroup/relations/children/enum.ts | Introduces children-relation enum namespace and error codes. |
| api-specifications/src/esco/occupationGroup/index.ts | Exposes moved schemas and newly added ParentEnums/ChildrenEnums via the main module export. |
| api-specifications/src/esco/occupationGroup/snapshots/index.test.ts.snap | Snapshot update to include the new exported enum namespaces. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1 @@ | |||
| {"mongoUri":"mongodb://127.0.0.1:24780/"} No newline at end of file | |||
There was a problem hiding this comment.
backend/globalConfig.json introduces a hard-coded local MongoDB URI. This file doesn’t appear to be referenced anywhere in backend/src (config is read from env elsewhere), and committing environment-specific connection strings can cause confusion in CI/prod. Consider removing this file from the repo (or replacing it with a documented example like globalConfig.example.json) and loading the URI from environment variables; if it must exist locally, add it to .gitignore.
| {"mongoUri":"mongodb://127.0.0.1:24780/"} | |
| {"mongoUri":"YOUR_MONGODB_URI_HERE"} |
|
|
||
| namespace OccupationGroupParentEnums { | ||
| export enum ObjectTypes { | ||
| ISCOGroup = CommonGroupTypes.ISCOGroup, | ||
| LocalGroup = CommonGroupTypes.LocalGroup, | ||
| } | ||
| export namespace Relations { | ||
| export namespace Parent { | ||
| export enum ObjectTypes { | ||
| ISCOGroup = CommonGroupTypes.ISCOGroup, | ||
| LocalGroup = CommonGroupTypes.LocalGroup, | ||
| } | ||
| } | ||
| export namespace Children { | ||
| export enum ObjectTypes { | ||
| ISCOGroup = CommonGroupTypes.ISCOGroup, | ||
| LocalGroup = CommonGroupTypes.LocalGroup, | ||
| ESCOOccupation = CommonGroupTypes.ESCOOccupation, | ||
| LocalOccupation = CommonGroupTypes.LocalOccupation, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This new file duplicates constants already defined in api-specifications/src/esco/occupationGroup/enums.ts (e.g., Relations.Parent.ObjectTypes and Relations.Children.ObjectTypes) and also duplicates Relations.Children.ObjectTypes again in OccupationGroupChildrenEnums. Having multiple sources of truth for the same enum values/error codes is likely to drift over time. Consider consolidating to a single definition (e.g., export the relation enums from one module and import/re-export them where needed) and removing the duplicated enum blocks here.
| namespace OccupationGroupParentEnums { | |
| export enum ObjectTypes { | |
| ISCOGroup = CommonGroupTypes.ISCOGroup, | |
| LocalGroup = CommonGroupTypes.LocalGroup, | |
| } | |
| export namespace Relations { | |
| export namespace Parent { | |
| export enum ObjectTypes { | |
| ISCOGroup = CommonGroupTypes.ISCOGroup, | |
| LocalGroup = CommonGroupTypes.LocalGroup, | |
| } | |
| } | |
| export namespace Children { | |
| export enum ObjectTypes { | |
| ISCOGroup = CommonGroupTypes.ISCOGroup, | |
| LocalGroup = CommonGroupTypes.LocalGroup, | |
| ESCOOccupation = CommonGroupTypes.ESCOOccupation, | |
| LocalOccupation = CommonGroupTypes.LocalOccupation, | |
| } | |
| } | |
| } | |
| import OccupationGroupEnums from "../../enums"; | |
| namespace OccupationGroupParentEnums { | |
| export enum ObjectTypes { | |
| ISCOGroup = CommonGroupTypes.ISCOGroup, | |
| LocalGroup = CommonGroupTypes.LocalGroup, | |
| } | |
| export import Relations = OccupationGroupEnums.Relations; |
| import { ObjectTypes as CommonGroupTypes } from "../../../common/objectTypes"; | ||
|
|
||
| namespace OccupationGroupChildrenEnums { |
There was a problem hiding this comment.
The filename enum.ts is inconsistent with the existing convention in this repo for enum modules (commonly enums.ts, including other relation enum modules). Renaming to enums.ts would improve discoverability and consistency across api-specifications/src/esco/**/relations/** folders.
| export import Schemas = OccupationGroupSchemas; | ||
| export import Patterns = OccupationGroupRegexes; | ||
| export import ParentEnums = OccupationGroupParentEnums; | ||
| export import ChildrenEnums = OccupationGroupChildrenEnums; |
There was a problem hiding this comment.
OccupationGroupAPISpecs now exports both Enums (which already contains Relations.Parent/Children enums) and the new ParentEnums/ChildrenEnums. This creates two parallel public enum hierarchies for the same concepts, which is easy for API consumers to misuse and hard to keep in sync. Consider either (1) moving the relation enums out of ./enums.ts and re-exporting them from a single place, or (2) not exporting ParentEnums/ChildrenEnums and keeping Enums.Relations.* as the only supported path.
| @@ -0,0 +1 @@ | |||
| {"mongoUri":"mongodb://127.0.0.1:24780/"} No newline at end of file | |||
There was a problem hiding this comment.
This change adds backend/globalConfig.json, but the PR description is about refactoring the api-specifications folder structure for OccupationGroup relations. If globalConfig.json is unrelated, it should be removed from this PR to keep the change scoped and easier to review.
There was a problem hiding this comment.
This file, should not be commited
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export enum ErrorCodes { | ||
| INVALID_LIMIT_PARAMETER = "INVALID_LIMIT_PARAMETER", | ||
| INVALID_NEXT_CURSOR_PARAMETER = "INVALID_NEXT_CURSOR_PARAMETER", | ||
| } |
There was a problem hiding this comment.
OccupationGroupParentEnums duplicates values that already exist in OccupationGroupEnums (e.g. object types and the shared 400 error codes). This creates a drift risk if one set is updated without the other. Prefer re-exporting the existing enums (or a shared/common enum) instead of redefining the same members here.
| export enum ErrorCodes { | |
| INVALID_LIMIT_PARAMETER = "INVALID_LIMIT_PARAMETER", | |
| INVALID_NEXT_CURSOR_PARAMETER = "INVALID_NEXT_CURSOR_PARAMETER", | |
| } | |
| export import ErrorCodes = OccupationGroupEnums.GET.Response.Status400.ErrorCodes; |
| ISCOGroup = CommonGroupTypes.ISCOGroup, | ||
| LocalGroup = CommonGroupTypes.LocalGroup, | ||
| } | ||
| export import Relations = OccupationGroupEnums.Relations; |
There was a problem hiding this comment.
export import Relations = OccupationGroupEnums.Relations; re-exports all relation enums (including children) from within the parent-specific enum module, which makes this module’s API broader and potentially misleading. Consider removing this re-export or narrowing it to only the parent-related types so ParentEnums stays focused on the parent relation.
| export import Relations = OccupationGroupEnums.Relations; |
| export enum ObjectTypes { | ||
| ISCOGroup = CommonGroupTypes.ISCOGroup, | ||
| LocalGroup = CommonGroupTypes.LocalGroup, | ||
| ESCOOccupation = CommonGroupTypes.ESCOOccupation, | ||
| LocalOccupation = CommonGroupTypes.LocalOccupation, | ||
| } | ||
| export namespace GET { | ||
| export namespace Response { | ||
| export namespace Status400 { | ||
| export enum ErrorCodes { | ||
| INVALID_LIMIT_PARAMETER = "INVALID_LIMIT_PARAMETER", | ||
| INVALID_NEXT_CURSOR_PARAMETER = "INVALID_NEXT_CURSOR_PARAMETER", | ||
| } |
There was a problem hiding this comment.
OccupationGroupChildrenEnums repeats enum members that already exist in OccupationGroupEnums.Relations.Children.ObjectTypes and the shared 400 error codes. Duplicating these constants across modules increases maintenance overhead and can lead to inconsistent values over time. Prefer re-exporting from a single source of truth where possible.
0c15830 to
da865d5
Compare
| OccupationGroupAPISpecs.GETDetailAPISpecs.GETDetailAPISpecs.Schemas.Response.Payload, | ||
| OccupationGroupAPISpecs.GETDetailAPISpecs.GETDetailAPISpecs.Schemas.Response.Payload.$id | ||
| ); |
There was a problem hiding this comment.
Bug: An incorrect property access path ...GETDetailAPISpecs.GETDetailAPISpecs... will cause a TypeError on application startup because the nested GETDetailAPISpecs property is undefined.
Severity: CRITICAL
Suggested Fix
Remove the redundant .GETDetailAPISpecs from the property access chain in both backend/src/validator.ts and backend/openapi/generateOpenApiDoc.ts. The correct path should be OccupationGroupAPISpecs.GETDetailAPISpecs.Schemas....
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: backend/src/validator.ts#L57-L59
Potential issue: A refactoring of the API specifications introduced an incorrect
property access path. The code attempts to access
`OccupationGroupAPISpecs.GETDetailAPISpecs.GETDetailAPISpecs` in
`backend/src/validator.ts` and `backend/openapi/generateOpenApiDoc.ts`. However, the
correct namespace structure is `OccupationGroupAPISpecs.GETDetailAPISpecs`, which does
not contain a nested `GETDetailAPISpecs` property. This results in an attempt to access
a property on an `undefined` value, which will throw a `TypeError` when the modules are
loaded. Because this error occurs during application initialization, the backend server
will fail to start.
Did we get this right? 👍 / 👎 to inform future reviews.
…roup relations api-specification definetion
…ucture for occupationGroup and implement it's usage in backend [pulumi up]
…d there usage in backend
17be0d0 to
4ba2cbe
Compare
api-specificationofOccupationGroupfor relations api separeting based on there hierarchy