fix: Full metadata upsert #290
Conversation
WalkthroughThe changes modify the Changes
Poem
✨ Finishing Touches
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
🧹 Nitpick comments (1)
packages/job-dispatch/src/resource/insert-resource-metadata.ts (1)
3-3: Consider using nativeSetfor uniqueness instead of Lodash.Since you're only using
_.uniqonce, you can replace it with a native JavaScriptSetto reduce dependencies.Suggested change:
- import _ from "lodash"; ... - const resourceIds = _.uniq(resourceMetadataValues.map((r) => r.resourceId)); + const resourceIds = [...new Set(resourceMetadataValues.map((r) => r.resourceId))];
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/job-dispatch/src/resource/insert-resource-metadata.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
packages/job-dispatch/src/resource/insert-resource-metadata.ts (1)
Pattern **/*.{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.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (1)
packages/job-dispatch/src/resource/insert-resource-metadata.ts (1)
26-30: Verified atomic replacement of resource metadata within a transaction.The delete-then-insert approach wrapped in the transaction
txensures that all existing metadata for the specified resource IDs is fully replaced with the new metadata in an atomic operation, maintaining data integrity and consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/job-dispatch/src/resource/insert-resource-metadata.ts (2)
32-45: Consider optimizing the deletion logic for large datasets.The current implementation using
_.chain()and array operations might impact performance with large metadata sets. Consider:
- Using SQL's
NOT INdirectly if supported by your ORM- Batching deletions for large datasets
Example optimization:
// Direct SQL approach (if supported) await tx.delete(schema.resourceMetadata) .where( sql`(resourceId, key) NOT IN (${resourceMetadataValues.map(v => `(${v.resourceId}, ${v.key})` ).join(',')})` );
Line range hint
44-53: Consider adding explicit error handling.While the Promise chain implicitly handles errors, consider adding explicit error handling to:
- Ensure atomicity between delete and insert operations
- Provide specific error messages for different failure scenarios
Example enhancement:
try { await tx.delete(schema.resourceMetadata).where(or(...deletedKeysChecks)); return await tx .insert(schema.resourceMetadata) .values(resourceMetadataValues) .onConflictDoUpdate({ target: [schema.resourceMetadata.key, schema.resourceMetadata.resourceId], set: buildConflictUpdateColumns(schema.resourceMetadata, ["value"]), }); } catch (error) { // Log specific error details throw new Error(`Failed to update resource metadata: ${error.message}`); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/job-dispatch/src/resource/insert-resource-metadata.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
packages/job-dispatch/src/resource/insert-resource-metadata.ts (1)
Pattern **/*.{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.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: Format
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (2)
packages/job-dispatch/src/resource/insert-resource-metadata.ts (2)
3-11: LGTM: Import changes are appropriate.The new imports (lodash and notInArray) are necessary for implementing the new deletion logic.
32-42: LGTM: Clean implementation of metadata cleanup logic.The grouping and filtering logic effectively identifies obsolete metadata entries for deletion.
Please verify the transaction isolation level to ensure consistency between delete and insert operations:
✅ Verification successful
Transaction handling verified: Implementation ensures data consistency
The code correctly uses a shared transaction object for both delete and insert operations, guaranteeing atomicity. The transaction management is properly handled at the caller level through dependency injection.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for transaction configuration rg -A 5 "BEGIN TRANSACTION|startTransaction|isolation"Length of output: 555
Script:
#!/bin/bash # Look for database operations in the metadata file ast-grep --pattern 'await $$$.$$$($$$)' packages/job-dispatch/src/resource/insert-resource-metadata.ts # Also check for database client initialization rg -l "knex|prisma|typeorm|sequelize" packages/job-dispatch/src/resource/Length of output: 322
Script:
#!/bin/bash # Get the complete file content to see function signature and context cat packages/job-dispatch/src/resource/insert-resource-metadata.ts # Find where this function is called rg -l "insertResourceMetadata" packages/job-dispatch/src/Length of output: 1718
Summary by CodeRabbit