fix: Optimize job policy query #153
Conversation
WalkthroughThe pull request modifies the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- apps/jobs/src/policy-checker/index.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (3)
apps/jobs/src/policy-checker/index.ts (3)
21-24: Inner joining withenvironmenttableThe
innerJoinwithschema.environmentcorrectly associates eachreleaseJobTriggerwith its correspondingenvironment. This ensures that only triggers linked to existing environments are considered.
25-41: Review of left joins withenvironmentPolicyandenvironmentPolicyApprovalThe use of
leftJoinwithschema.environmentPolicyandschema.environmentPolicyApprovalallows for the inclusion of policy-related data when available while retaining triggers that may not have associated policies or approvals. This approach is appropriate for handling optional relationships.
42-42: Verify the combined conditions in thewhereclauseThe
whereclause usesand(eq(schema.job.status, JobStatus.Pending), isPassingApprovalGate). Given the potentialnullvalues instatus, ensure that this condition accurately filters jobs that are pending and either have passed the approval gate or do not require approval.To confirm the logic, you might run a test query to check the selected records:
This script helps verify that the
whereclause includes all intended jobs.
| const isPassingApprovalGate = notInArray( | ||
| schema.environmentPolicyApproval.status, | ||
| ["pending", "rejected"], | ||
| ); |
There was a problem hiding this comment.
Handle null values in isPassingApprovalGate condition
Since environmentPolicyApproval is left-joined, schema.environmentPolicyApproval.status may be null. Using notInArray might exclude records where status is null. If the intention is to include records with null status (i.e., no approval required), consider adjusting the condition to handle null values.
You could modify isPassingApprovalGate to include null statuses:
-import { and, eq, notInArray } from "@ctrlplane/db";
+import { and, eq, notInArray, or, isNull } from "@ctrlplane/db";
const isPassingApprovalGate = or(
notInArray(
schema.environmentPolicyApproval.status,
["pending", "rejected"],
),
+ isNull(schema.environmentPolicyApproval.status),
);This adjustment ensures that records without an environmentPolicyApproval are included in the results.
Committable suggestion was skipped due to low confidence.
Summary by CodeRabbit
New Features
Bug Fixes