refactor: retrieve jobs using new method if engine flag is enabled#500
refactor: retrieve jobs using new method if engine flag is enabled#500adityachoudhari26 merged 9 commits intomainfrom
Conversation
|
""" WalkthroughThis update introduces a modular approach to retrieving job details by splitting the logic into two distinct modules: one for legacy jobs and another for jobs using the new policy engine. The API route handler now delegates job fetching to these modules by concurrently calling both and returning the first non-null result. Database schema relations are updated to support the new job structure, and authorization utilities are adjusted to handle both legacy and new job models. Variable naming and sorting logic are refined in deployment version job processing to ensure robustness when handling job lists and missing data. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Route
participant LegacyJobModule
participant NewEngineJobModule
participant DB
Client->>API_Route: GET /api/v1/jobs/:jobId
API_Route->>LegacyJobModule: getLegacyJob(db, jobId)
API_Route->>NewEngineJobModule: getNewEngineJob(db, jobId)
LegacyJobModule->>DB: Query legacy job tables with joins
NewEngineJobModule->>DB: Query new engine job tables with nested relations
DB-->>LegacyJobModule: Legacy job data or null
DB-->>NewEngineJobModule: New engine job data or null
alt New engine job found
NewEngineJobModule-->>API_Route: Job object
else Legacy job found
LegacyJobModule-->>API_Route: Job object
else No job found
API_Route-->>Client: 404 Job not found
end
API_Route-->>Client: JSON response with job data or error
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
apps/webservice/src/app/api/v1/jobs/[jobId]/new-engine-job.ts (1)
52-60: Micro‑optimisation: avoid recreating the AES helper in a tight loop
variablesAES256()is instantiated for every variable value. Initialising it once improves readability and avoids unnecessary work:-const jobVariables = Object.fromEntries( - values.map(({ variableValueSnapshot }) => { +const crypto = variablesAES256(); +const jobVariables = Object.fromEntries( + values.map(({ variableValueSnapshot }) => { const { key, value, sensitive } = variableValueSnapshot; const strval = String(value); - const resolvedValue = sensitive - ? variablesAES256().decrypt(strval) - : strval; + const resolvedValue = sensitive ? crypto.decrypt(strval) : strval;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/webservice/src/app/api/v1/jobs/[jobId]/legacy-job.ts(1 hunks)apps/webservice/src/app/api/v1/jobs/[jobId]/new-engine-job.ts(1 hunks)apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts(2 hunks)packages/db/src/schema/job.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/db/src/schema/job.tsapps/webservice/src/app/api/v1/jobs/[jobId]/new-engine-job.tsapps/webservice/src/app/api/v1/jobs/[jobId]/route.tsapps/webservice/src/app/api/v1/jobs/[jobId]/legacy-job.ts
🧬 Code Graph Analysis (3)
packages/db/src/schema/job.ts (1)
packages/db/src/schema/release.ts (1)
releaseJob(126-134)
apps/webservice/src/app/api/v1/jobs/[jobId]/new-engine-job.ts (5)
packages/db/src/client.ts (1)
db(15-15)packages/db/src/common.ts (1)
Tx(22-22)packages/db/src/schema/job.ts (1)
job(75-107)packages/db/src/schema/release.ts (6)
releaseJob(126-134)release(113-124)versionRelease(46-60)variableSetRelease(62-70)variableValueSnapshot(93-111)releaseTarget(20-42)packages/db/src/schema/resource.ts (1)
resource(59-87)
apps/webservice/src/app/api/v1/jobs/[jobId]/legacy-job.ts (3)
packages/db/src/client.ts (1)
db(15-15)packages/db/src/common.ts (2)
Tx(22-22)takeFirstOrNull(15-20)packages/db/src/schema/deployment-version.ts (1)
deploymentVersion(109-136)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (2)
packages/db/src/schema/job.ts (1)
51-51: Potential circular‑import trap between job.ts and release.ts
job.tsnow importsreleaseJobwhilerelease.tsalready importsjob. In ESM/TS this creates a bi‑directional module cycle.
Because the Drizzle table constants are evaluated eagerly at module top‑level, whichever side is evaluated first will find the other side’s constant stillundefined, which can surface asTypeError: Cannot read properties of undefined (reading "jobId")at runtime or during migrations.Please verify the load order in your build/testing environment and, if necessary, break the cycle (e.g. move cross‑referenced tables into a single module or use late binding with
lazy/function accessors).apps/webservice/src/app/api/v1/jobs/[jobId]/legacy-job.ts (1)
81-82:⚠️ Potential issueResource filter unintentionally discards jobs without a resource
isNull(schema.resource.deletedAt)converts the LEFT JOIN withresourceinto an INNER‐like filter, because the predicate evaluates toNULL(i.e. false) when the entire row is absent.
Jobs that legitimately have no resource will therefore fall through and be treated as “not found”.-.where(and(eq(schema.job.id, jobId), isNull(schema.resource.deletedAt))); +.where( + and( + eq(schema.job.id, jobId), + or(isNull(schema.resource.id), isNull(schema.resource.deletedAt)), + ), +);Likely an incorrect or invalid review comment.
| releaseJob: one(releaseJob, { | ||
| fields: [job.id], | ||
| references: [releaseJob.jobId], | ||
| }), |
There was a problem hiding this comment.
Cardinality mismatch: one() may be wrong when job_id is not unique
release_job.job_id is not declared UNIQUE in release.ts, meaning a job could, in theory, appear in multiple rows. Declaring the relation as one(releaseJob, …) assumes a 1‑to‑1 mapping and Drizzle will only return the first matching record, silently discarding the rest.
If the business rule truly enforces uniqueness, add a uniqueIndex().on(releaseJob.jobId) in release.ts; otherwise, change this to many(releaseJob, …) so that no data is lost.
There was a problem hiding this comment.
in the new system, a release job should only ever point to one job and vice versa
There was a problem hiding this comment.
@adityachoudhari26 Thanks for the clarification! That makes perfect sense - a one-to-one relationship between job and release job is correctly represented with the one(releaseJob, ...) relation. The implementation is aligned with your intended design.
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#500
File: packages/db/src/schema/job.ts:117-120
Timestamp: 2025-04-21T18:34:54.740Z
Learning: In the system's job schema, the relationship between job and releaseJob is a true one-to-one relationship - a release job should only ever point to one job and vice versa. The implementation uses `one(releaseJob, ...)` in the jobRelations to reflect this business rule.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| if (jobResult == null) return null; | ||
|
|
||
| const { releaseJob, ...job } = jobResult; | ||
| const { release } = releaseJob; | ||
| const { versionRelease, variableSetRelease } = release; | ||
| const { version, releaseTarget } = versionRelease; | ||
|
|
||
| const { values } = variableSetRelease; | ||
| const jobVariables = Object.fromEntries( | ||
| values.map(({ variableValueSnapshot }) => { | ||
| const { key, value, sensitive } = variableValueSnapshot; | ||
| const strval = String(value); | ||
| const resolvedValue = sensitive | ||
| ? variablesAES256().decrypt(strval) | ||
| : strval; | ||
| return [key, resolvedValue]; | ||
| }), |
There was a problem hiding this comment.
Null‑safety: crashes when a job lacks a releaseJob
releaseJob is destructured without a guard:
const { releaseJob, ...job } = jobResult;
const { release } = releaseJob; // ← will throw if releaseJob === nullAny job inserted before the new engine rollout – or future edge‑cases – will trigger a runtime TypeError.
-const { releaseJob, ...job } = jobResult;
-if (releaseJob == null) return null; // or build a sane fallback
-const { release } = releaseJob;
+const { releaseJob, ...job } = jobResult;
+if (releaseJob == null) return null; // early‑return so callers treat it as “not found”
+const { release } = releaseJob;Apply the same defensive checks for variableSetRelease, versionRelease, resource, and resource.metadata to avoid similar crashes.
There was a problem hiding this comment.
tbh this should fail if there is no release job for a job, that mens something went wrong. I wrapped this in a try catch to gracefully return null, but I would want that to error because it means something is wrong
There was a problem hiding this comment.
@adityachoudhari26 I understand your approach now. You've implemented a pragmatic error handling pattern:
- Let the code fail internally if invariants are violated (like a job without a releaseJob)
- Catch those errors at the function boundary
- Log them for visibility and debugging
- Return null to the caller
This is a reasonable pattern that balances code readability (avoiding excessive null checks) while maintaining API stability. The try-catch you've added will indeed catch and log any TypeError that occurs if releaseJob is null, making it easier to detect and investigate unexpected data states.
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#500
File: apps/webservice/src/app/api/v1/jobs/[jobId]/new-engine-job.ts:0-0
Timestamp: 2025-04-21T18:47:47.613Z
Learning: In the job API endpoints, it's acceptable to let internal code fail with TypeErrors when encountering unexpected null values (like missing releaseJob) as long as the errors are caught at the function boundary, logged for debugging, and a consistent response is returned to the caller.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| // eslint-disable-next-line no-restricted-properties | ||
| const isUsingNewEngine = process.env.ENABLE_NEW_POLICY_ENGINE === "true"; | ||
| const job = isUsingNewEngine | ||
| ? await getNewEngineJob(db, params.jobId) | ||
| : await getLegacyJob(db, params.jobId); | ||
|
|
||
| if (row == null) | ||
| if (job == null) | ||
| return NextResponse.json( | ||
| { error: "Job execution not found." }, | ||
| { status: 404 }, | ||
| { error: "Job not found" }, | ||
| { status: NOT_FOUND }, | ||
| ); | ||
|
|
||
| const deploymentVersion = | ||
| row.deployment_version != null | ||
| ? { ...row.deployment_version, metadata: {} } | ||
| : null; | ||
|
|
||
| const je = { | ||
| job: row.job, | ||
| runbook: row.runbook, | ||
| environment: row.environment, | ||
| resource: row.resource, | ||
| deployment: row.deployment, | ||
| deploymentVersion, | ||
| }; | ||
|
|
||
| const policyId = je.environment?.policyId; | ||
|
|
||
| const approval = | ||
| je.deploymentVersion?.id && policyId | ||
| ? await getApprovalDetails(je.deploymentVersion.id, policyId) | ||
| : undefined; | ||
|
|
||
| const jobVariableRows = await db | ||
| .select() | ||
| .from(schema.jobVariable) | ||
| .where(eq(schema.jobVariable.jobId, params.jobId)); | ||
|
|
||
| const variables = Object.fromEntries( | ||
| jobVariableRows.map((v) => { | ||
| const strval = String(v.value); | ||
| const value = v.sensitive ? variablesAES256().decrypt(strval) : strval; | ||
| return [v.key, value]; | ||
| }), | ||
| ); | ||
|
|
||
| const jobWithVariables = { | ||
| ...je, | ||
| variables, | ||
| release: | ||
| je.deploymentVersion != null | ||
| ? { ...je.deploymentVersion, version: je.deploymentVersion.tag } | ||
| : { version: undefined }, | ||
| }; | ||
| if (je.resource == null) return NextResponse.json(jobWithVariables); | ||
|
|
||
| const metadata = await db | ||
| .select() | ||
| .from(schema.resourceMetadata) | ||
| .where(eq(schema.resourceMetadata.resourceId, je.resource.id)) | ||
| .then((rows) => Object.fromEntries(rows.map((m) => [m.key, m.value]))); | ||
|
|
||
| return NextResponse.json({ | ||
| ...jobWithVariables, | ||
| resource: { ...jobWithVariables.resource, metadata }, | ||
| approval, | ||
| }); | ||
| return NextResponse.json(job); |
There was a problem hiding this comment.
Route must align with helper’s return contract
getLegacyJob may now return null (per earlier comment) rather than a pre‑built Response.
Add a type‑guard so the handler doesn’t inadvertently call NextResponse.json() on a Response object, and propagate the 404 correctly:
-const job = isUsingNewEngine
- ? await getNewEngineJob(db, params.jobId)
- : await getLegacyJob(db, params.jobId);
-
-if (job == null)
+const jobData = isUsingNewEngine
+ ? await getNewEngineJob(db, params.jobId)
+ : await getLegacyJob(db, params.jobId);
+
+if (jobData == null)
return NextResponse.json(
{ error: "Job not found" },
{ status: NOT_FOUND },
);
-
-return NextResponse.json(job);
+return NextResponse.json(jobData);This keeps the HTTP semantics consistent across the feature‑flag branches.
📝 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 no-restricted-properties | |
| const isUsingNewEngine = process.env.ENABLE_NEW_POLICY_ENGINE === "true"; | |
| const job = isUsingNewEngine | |
| ? await getNewEngineJob(db, params.jobId) | |
| : await getLegacyJob(db, params.jobId); | |
| if (row == null) | |
| if (job == null) | |
| return NextResponse.json( | |
| { error: "Job execution not found." }, | |
| { status: 404 }, | |
| { error: "Job not found" }, | |
| { status: NOT_FOUND }, | |
| ); | |
| const deploymentVersion = | |
| row.deployment_version != null | |
| ? { ...row.deployment_version, metadata: {} } | |
| : null; | |
| const je = { | |
| job: row.job, | |
| runbook: row.runbook, | |
| environment: row.environment, | |
| resource: row.resource, | |
| deployment: row.deployment, | |
| deploymentVersion, | |
| }; | |
| const policyId = je.environment?.policyId; | |
| const approval = | |
| je.deploymentVersion?.id && policyId | |
| ? await getApprovalDetails(je.deploymentVersion.id, policyId) | |
| : undefined; | |
| const jobVariableRows = await db | |
| .select() | |
| .from(schema.jobVariable) | |
| .where(eq(schema.jobVariable.jobId, params.jobId)); | |
| const variables = Object.fromEntries( | |
| jobVariableRows.map((v) => { | |
| const strval = String(v.value); | |
| const value = v.sensitive ? variablesAES256().decrypt(strval) : strval; | |
| return [v.key, value]; | |
| }), | |
| ); | |
| const jobWithVariables = { | |
| ...je, | |
| variables, | |
| release: | |
| je.deploymentVersion != null | |
| ? { ...je.deploymentVersion, version: je.deploymentVersion.tag } | |
| : { version: undefined }, | |
| }; | |
| if (je.resource == null) return NextResponse.json(jobWithVariables); | |
| const metadata = await db | |
| .select() | |
| .from(schema.resourceMetadata) | |
| .where(eq(schema.resourceMetadata.resourceId, je.resource.id)) | |
| .then((rows) => Object.fromEntries(rows.map((m) => [m.key, m.value]))); | |
| return NextResponse.json({ | |
| ...jobWithVariables, | |
| resource: { ...jobWithVariables.resource, metadata }, | |
| approval, | |
| }); | |
| return NextResponse.json(job); | |
| // eslint-disable-next-line no-restricted-properties | |
| const isUsingNewEngine = process.env.ENABLE_NEW_POLICY_ENGINE === "true"; | |
| const jobData = isUsingNewEngine | |
| ? await getNewEngineJob(db, params.jobId) | |
| : await getLegacyJob(db, params.jobId); | |
| if (jobData == null) | |
| return NextResponse.json( | |
| { error: "Job not found" }, | |
| { status: NOT_FOUND }, | |
| ); | |
| return NextResponse.json(jobData); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
e2e/tests/api/jobs.spec.ts (1)
66-124: Consider refactoring to reduce duplication between tests.There's significant code duplication between the two test cases. The only differences are the test name, job name, and engine type ("new" vs "legacy"), while the structure, assertions, and error handling are identical.
Consider refactoring to a parameterized test approach:
-test("should create and retrieve a job with new engine", async ({ api }) => { +const testCases = [ + { engineType: "new", jobName: "test-job" }, + { engineType: "legacy", jobName: "test-job-legacy" } +]; + +for (const { engineType, jobName } of testCases) { + test(`should create and retrieve a job with ${engineType} engine`, async ({ api }) => { // Create a job const createResponse = await api.POST( "/v1/jobs" as PathsWithMethod<paths, "post">, { body: { deploymentId: "test-deployment", - name: "test-job", + name: jobName, status: "ready", versionSelector: { version: "1.0.0", }, jobAgentConfig: { - engine: "new", + engine: engineType, }, }, }, ); // ... rest of the test ... - expect(jobData.jobAgentConfig.engine).toBe("new"); + expect(jobData.jobAgentConfig.engine).toBe(engineType); // ... rest of the test ... - expect(retrievedJob.jobAgentConfig.engine).toBe("new"); + expect(retrievedJob.jobAgentConfig.engine).toBe(engineType); // ... rest of the test ... - }); + }); +} - -test("should create and retrieve a job with legacy engine", async ({ api }) => { - // ... duplicate code ... -});This approach would reduce code duplication while still testing both engine types thoroughly.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
e2e/tests/api/jobs.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
e2e/tests/api/jobs.spec.ts
🧬 Code Graph Analysis (1)
e2e/tests/api/jobs.spec.ts (1)
e2e/tests/fixtures.ts (1)
test(11-26)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
🔇 Additional comments (4)
e2e/tests/api/jobs.spec.ts (4)
1-5: Imports and setup look good.The imports are concise and appropriate, using typed API paths from OpenAPI definitions for request type safety, which is a good practice.
6-64: Well-structured test for the new engine job.This test effectively validates both job creation and retrieval with the new engine. I particularly like:
- Strong typing with
PathsWithMethodfor API routes- Thorough error handling for unexpected responses
- Comprehensive assertions checking all relevant job properties
- Clear separation between creation and retrieval steps
The implementation correctly tests the new job engine functionality according to the PR objectives.
26-28: Good error handling for API responses.The code properly checks for unexpected response structures before proceeding with assertions, which helps prevent cryptic test failures and provides clearer error messages when API responses don't match expected formats.
Also applies to: 50-56, 86-88, 110-116
8-23: Well-typed API requests using OpenAPI schema.Using
PathsWithMethod<paths, "post">andPathsWithMethod<paths, "get">ensures that API requests conform to the defined schema, providing compile-time type safety and reducing the risk of invalid requests.Also applies to: 38-47, 68-83, 98-107
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/auth/src/utils/rbac.ts (2)
342-358: Soft‑deletion filter is applied only toresource– consider extending it or documenting why other entities are exempt
legacyJobScopesguards against orphaned/soft‑deleted resources withisNull(resource.deletedAt)– good!
However, similar soft‑delete columns often exist onenvironment,deploymentVersion, etc. If they do exist in your schema, missing filters could surface unexpected rows and leak data. Either:
- Extend the
and(...)predicate to cover every entity that supports soft deletes, or- Add a comment explaining that only
resource.deletedAtis relevant here.Keeping the rule consistent across queries makes later maintenance safer.
401-407: Environment‑flag check can be more robust & self‑documenting
process.env.ENABLE_NEW_POLICY_ENGINE === "true"works, but it:
- Fails silently for
"TRUE","1", etc.- Reads the env var every call (minor perf hit, but mostly readability).
Consider extracting a single constant at module scope:
const IS_NEW_ENGINE_ENABLED = /^true|1$/i.test( process.env.ENABLE_NEW_POLICY_ENGINE ?? "", );and reuse it inside
getJobScopes.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts(1 hunks)packages/auth/src/utils/rbac.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/auth/src/utils/rbac.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts (1)
27-30: Consider sequential retrieval for improved efficiency.While concurrent retrieval using Promise.all is performant, it always executes both functions even if the first one succeeds. Consider a sequential approach that only falls back to legacy if new engine returns null.
- const [newEngine, legacy] = await Promise.all([ - getNewEngineJob(db, jobId), - getLegacyJob(db, jobId), - ]); - - const job = newEngine ?? legacy; + // Try new engine first, fall back to legacy if needed + const newEngine = await getNewEngineJob(db, jobId); + const job = newEngine ?? await getLegacyJob(db, jobId);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts(1 hunks)packages/auth/src/utils/rbac.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/auth/src/utils/rbac.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts
🧬 Code Graph Analysis (1)
apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts (5)
apps/webservice/src/app/api/v1/middleware.ts (1)
request(24-53)apps/webservice/src/app/api/v1/auth.ts (2)
authn(15-21)authz(23-58)apps/webservice/src/app/api/v1/jobs/[jobId]/new-engine-job.ts (1)
getNewEngineJob(13-90)packages/db/src/client.ts (1)
db(15-15)apps/webservice/src/app/api/v1/jobs/[jobId]/legacy-job.ts (1)
getLegacyJob(48-141)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (4)
apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts (4)
3-3: Good use of HTTP status constants.Using the
NOT_FOUNDconstant from http-status improves code readability and maintainability compared to using numeric status codes directly.
12-13: Good modularization of job retrieval logic.The separation of job retrieval into specialized helper functions (legacy and new engine) improves code organization and maintainability.
18-20: Properly awaiting parameters for authorization.Good improvement to ensure the jobId is properly extracted from params before performing permission checks.
32-37: Proper null handling and standardized error response.The code correctly handles the case where both job retrieval methods return null and provides a clear error message with the appropriate status code.
| async ({ db }, { params }) => { | ||
| const { jobId } = await params; | ||
|
|
||
| const [newEngine, legacy] = await Promise.all([ | ||
| getNewEngineJob(db, jobId), | ||
| getLegacyJob(db, jobId), | ||
| ]); | ||
|
|
||
| const job = newEngine ?? legacy; | ||
| if (job == null) | ||
| return NextResponse.json( | ||
| { error: "Job not found" }, | ||
| { status: NOT_FOUND }, | ||
| ); | ||
|
|
||
| return NextResponse.json(job); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify error handling in getLegacyJob function.
While the getNewEngineJob function has error handling that returns null on exceptions, getLegacyJob doesn't appear to have similar protection. This could lead to unhandled exceptions being propagated to this handler.
🏁 Script executed:
#!/bin/bash
# Check if getLegacyJob has error handling
ast-grep --pattern $'export const getLegacyJob = async (db: $_) {
$$$
try {
$$$
} catch {
$$$
}
$$$
}'Length of output: 121
🏁 Script executed:
#!/bin/bash
# Locate getLegacyJob implementation with surrounding context
rg -n "getLegacyJob" -C5Length of output: 2849
🏁 Script executed:
#!/bin/bash
# Extract the full getLegacyJob function to check for error handling
sed -n '/export const getLegacyJob/,/^}$/p' 'apps/webservice/src/app/api/v1/jobs/[jobId]/legacy-job.ts'Length of output: 2647
🏁 Script executed:
#!/bin/bash
# Extract the full getNewEngineJob function to inspect its error handling
sed -n '/export const getNewEngineJob/,/^};/p' 'apps/webservice/src/app/api/v1/jobs/[jobId]/new-engine-job.ts'Length of output: 2373
Add error handling to getLegacyJob to mirror getNewEngineJob
The getLegacyJob function currently has no try/catch guard, so any database or decryption error will bubble up to the route handler instead of returning null. Wrap its entire body in a try/catch and return null on failure:
• File:
apps/webservice/src/app/api/v1/jobs/[jobId]/legacy-job.ts
• Around line 48 (the start of export const getLegacyJob), update to:
export const getLegacyJob = async (db: Tx, jobId: string) => {
+ try {
const rows = await db
.select()
.from(schema.job)
… // existing joins and logic
return { … }; // your current return value
+ } catch (error) {
+ log.error("Error getting legacy job", { error });
+ return null;
+ }
};This ensures that any unexpected errors result in null—letting your route handler correctly fall back and return a 404 instead of crashing.
📝 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.
| async ({ db }, { params }) => { | |
| const { jobId } = await params; | |
| const [newEngine, legacy] = await Promise.all([ | |
| getNewEngineJob(db, jobId), | |
| getLegacyJob(db, jobId), | |
| ]); | |
| const job = newEngine ?? legacy; | |
| if (job == null) | |
| return NextResponse.json( | |
| { error: "Job not found" }, | |
| { status: NOT_FOUND }, | |
| ); | |
| return NextResponse.json(job); | |
| export const getLegacyJob = async (db: Tx, jobId: string) => { | |
| try { | |
| const rows = await db | |
| .select() | |
| .from(schema.job) | |
| … // existing joins and logic | |
| return { … }; // your current return value | |
| } catch (error) { | |
| log.error("Error getting legacy job", { error }); | |
| return null; | |
| } | |
| }; |
There was a problem hiding this comment.
the authz handler catches errors
There was a problem hiding this comment.
@adityachoudhari26 I see, thanks for clarifying. Since the authz handler already has error handling in place at the middleware level, and your coding guidelines mention flexibility in error handling approaches beyond try/catch blocks, my suggestion isn't necessary.
I appreciate you sharing this information about your error handling architecture. Thanks for the feedback!
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#500
File: apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts:24-39
Timestamp: 2025-04-21T20:24:11.545Z
Learning: In the ctrlplane project, the authz handler middleware provides error handling that catches errors from functions like `getLegacyJob`, making additional function-level try/catch blocks unnecessary in some cases.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Summary by CodeRabbit