fix: Job output includes release metadata #258
Conversation
WalkthroughThe pull request introduces significant modifications to the job handling API and the OpenAPI specification. Key changes include updating the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts (1)
109-115: Consider adding metadata type validationWhile the metadata extraction is correct, consider adding type validation to ensure the metadata values conform to the OpenAPI specification (string values only).
- metadata: rows.map((r) => r.release_metadata).filter(isPresent), + metadata: rows + .map((r) => r.release_metadata) + .filter(isPresent) + .reduce((acc, meta) => ({ + ...acc, + [meta.key]: String(meta.value) + }), {}),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts(3 hunks)openapi.v1.json(2 hunks)
🔇 Additional comments (4)
apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts (3)
3-3: LGTM: Import added for metadata filtering
The isPresent utility is appropriately imported to handle null/undefined metadata filtering.
89-92: LGTM: Added metadata join
The left join with schema.releaseMetadata is correctly implemented to fetch associated metadata while preserving releases without metadata.
101-101: LGTM: Safe array access
Using rows.at(0) is a safer approach for accessing the first element compared to direct indexing.
openapi.v1.json (1)
2358-2361: LGTM: Release metadata schema
The metadata schema is correctly defined with:
- Proper type definition as an object with string values
- Required field designation ensuring consistency
Also applies to: 2376-2376
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/webservice/src/app/api/v1/job-agents/[agentId]/jobs/running/route.ts (1)
51-69: Consider Simplifying Data Transformation LogicFor improved readability and performance, consider simplifying the data transformation logic by using native JavaScript methods instead of
_.chain. This can enhance maintainability and reduce dependencies.Apply the following refactor to simplify the code:
- _.chain(rows) - .groupBy((row) => row.job.id) - .map((jobRows) => ({ - ...jobRows[0]!.job, - config: jobRows[0]!.release_job_trigger, - environment: jobRows[0]!.environment, - target: jobRows[0]!.resource, - deployment: jobRows[0]!.deployment, - release: - jobRows[0]!.release != null - ? { - ...jobRows[0]!.release, - metadata: jobRows - .map((r) => r.release_metadata) - .filter(isPresent), - } - : null, - })) - .value(), + Object.values( + rows.reduce((acc, row) => { + const jobId = row.job.id; + if (!acc[jobId]) { + acc[jobId] = { + ...row.job, + config: row.release_job_trigger, + environment: row.environment, + target: row.resource, + deployment: row.deployment, + release: row.release + ? { + ...row.release, + metadata: [], + } + : null, + }; + } + if (row.release_metadata && acc[jobId].release) { + acc[jobId].release.metadata.push(row.release_metadata); + } + return acc; + }, {}) + );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
apps/webservice/src/app/api/v1/job-agents/[agentId]/jobs/running/route.ts(4 hunks)
🔇 Additional comments (4)
apps/webservice/src/app/api/v1/job-agents/[agentId]/jobs/running/route.ts (4)
3-4: Necessary Imports Added
The additions of lodash and isPresent imports are appropriate for the new data processing logic.
14-14: Including releaseMetadata in Schema Imports
Including releaseMetadata in the imports from @ctrlplane/db/schema ensures that it can be used in the query and is correctly added.
35-35: Correct Left Join with releaseMetadata
The left join with releaseMetadata on releaseMetadata.releaseId and release.id correctly associates release metadata with releases.
51-69: Verify Safe Usage of Non-Null Assertions
The use of non-null assertions (!) on jobRows[0]! assumes that each group will have at least one item. Please verify that jobRows will never be empty to prevent potential runtime errors from accessing undefined properties.
To ensure that jobRows[0]! is always safe to access, you can verify that the query results and grouping logic guarantee non-empty groups:
✅ Verification successful
Based on the search results, I can now provide a final response:
Non-null Assertions are Safe in this Context
The non-null assertions on jobRows[0]! are safe because:
- The data comes from an INNER JOIN between
jobandrelease_job_triggertables, which ensures that every job has at least one associated trigger record. - The
groupByoperation is performed onjob.id, and since each job must have a trigger (due to INNER JOIN), each group will contain at least one row. - The code pattern is consistently used across multiple similar implementations in the codebase (as seen in packages/api/src/router/runbook.ts, packages/api/src/router/system.ts, etc.) where the same grouping strategy is employed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the query can produce empty groups
# Test: Run the query and check for any empty groups
# Expectation: No empty groups should be found
# Simulate the query in the database with the same conditions
# and check if any job IDs have zero associated rows.
# Assuming you have access to a database client:
# Replace <database_connection_string> with your connection details.
psql <database_connection_string> -c "
SELECT job.id, COUNT(*)
FROM job
INNER JOIN release_job_trigger ON release_job_trigger.job_id = job.id
LEFT JOIN environment ON environment.id = release_job_trigger.environment_id
LEFT JOIN resource ON resource.id = release_job_trigger.resource_id
LEFT JOIN release ON release.id = release_job_trigger.release_id
LEFT JOIN release_metadata ON release_metadata.release_id = release.id
LEFT JOIN deployment ON deployment.id = release.deployment_id
WHERE job.job_agent_id = '<agentId>'
AND job.status NOT IN (
'failure',
'cancelled',
'skipped',
'completed',
'invalid_job_agent'
)
AND resource.deleted_at IS NULL
GROUP BY job.id
HAVING COUNT(*) = 0;
"
Length of output: 844
Script:
#!/bin/bash
# Let's analyze the code to understand the grouping logic and data flow
# First, let's find where the rows are coming from
rg -B 5 -A 5 "_.chain\(rows\)" --type typescript
# Then, let's check if there's any filtering before the groupBy
rg -B 10 "groupBy.*job\.id" --type typescript
# Also check for any validation or error handling around this operation
rg "jobRows\[0\]" --type typescript
Length of output: 241
Script:
#!/bin/bash
# Let's analyze the code to understand the grouping logic and data flow without specifying file type
# First, let's find where the rows are coming from
rg -B 5 -A 5 "_.chain\(rows\)"
# Then, let's check if there's any filtering before the groupBy
rg -B 10 "groupBy.*job\.id"
# Also check for any validation or error handling around this operation
rg "jobRows\[0\]"
# Let's also check the SQL query that populates these rows
rg -B 5 -A 5 "SELECT.*FROM.*job.*release_job_trigger"
Length of output: 14494
Summary by CodeRabbit
New Features
metadataproperty toReleaseandResourceschemas, andreasonproperty toJobschema.Bug Fixes
Documentation