more checks to prevent modification of GPS coordinates #3850
Conversation
|
Caution Review failedThe pull request is closed. 📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request focus on enhancing the Changes
Possibly related issues
Possibly related PRs
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
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## staging #3850 +/- ##
===========================================
- Coverage 11.77% 11.76% -0.01%
===========================================
Files 114 114
Lines 15223 15218 -5
Branches 274 274
===========================================
- Hits 1793 1791 -2
+ Misses 13430 13427 -3
|
|
Device registry changes in this PR available for preview here |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
src/device-registry/models/Site.js (1)
405-428: Avoid using thedeleteoperator to improve performanceThe static analysis highlights that using the
deleteoperator can impact performance. Instead of deleting properties from the update object, consider alternative approaches.For instance, you can unset fields in MongoDB updates:
if (updates.$set) { - if (updates.$set.latitude) delete updates.$set.latitude; - if (updates.$set.longitude) delete updates.$set.longitude; + if (updates.$set.latitude) updates.$unset = { ...updates.$unset, latitude: "" }; + if (updates.$set.longitude) updates.$unset = { ...updates.$unset, longitude: "" }; // Remove latitude and longitude from $set + delete updates.$set.latitude; + delete updates.$set.longitude; }However, given that
immutablefields are enforced by Mongoose, this additional handling may be unnecessary.🧰 Tools
🪛 Biome
[error] 405-405: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 406-406: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 410-410: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 411-411: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 416-416: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 417-417: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 425-425: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 426-426: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 427-427: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 428-428: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/device-registry/models/Site.js(3 hunks)
🧰 Additional context used
🪛 Biome
src/device-registry/models/Site.js
[error] 377-377: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 405-405: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 406-406: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 410-410: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 411-411: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 416-416: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 417-417: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 425-425: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 426-426: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 427-427: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 428-428: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (2)
src/device-registry/models/Site.js (2)
130-130: Proper use of immutable for the latitude field
Setting latitude as immutable correctly prevents modifications after creation, aligning with the PR objectives to enhance the security of GPS coordinates.
139-139: Proper use of immutable for the longitude field
Marking longitude as immutable ensures it cannot be altered after the document is created, supporting the goal of preventing unauthorized modifications.
src/device-registry/models/Site.js
Outdated
| siteSchema.pre( | ||
| ["updateOne", "findOneAndUpdate", "updateMany", "update", "save"], | ||
| function(next) { | ||
| // Handle updates | ||
| if (this.getUpdate) { | ||
| const updates = this.getUpdate(); | ||
| if (updates) { | ||
| // Remove latitude and longitude from updates | ||
| if (updates.latitude) delete updates.latitude; | ||
| if (updates.longitude) delete updates.longitude; | ||
|
|
||
| // Check for $set operator | ||
| if (updates.$set) { | ||
| if (updates.$set.latitude) delete updates.$set.latitude; | ||
| if (updates.$set.longitude) delete updates.$set.longitude; | ||
| } | ||
|
|
||
| // Check for $push operator | ||
| if (updates.$push) { | ||
| if (updates.$push.latitude) delete updates.$push.latitude; | ||
| if (updates.$push.longitude) delete updates.$push.longitude; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return next(); | ||
| }); | ||
| // Handle save operation | ||
| if (this.isNew || this.isModified()) { | ||
| // Remove specific fields on save | ||
| if (this.isModified("latitude")) delete this.latitude; | ||
| if (this.isModified("longitude")) delete this.longitude; | ||
| if (this.isModified("_id")) delete this._id; | ||
| if (this.isModified("generated_name")) delete this.generated_name; | ||
|
|
||
| // Update site_codes array | ||
| this.site_codes = [ | ||
| this._id, | ||
| this.name, | ||
| this.generated_name, | ||
| this.lat_long, | ||
| ]; | ||
| if (this.search_name) this.site_codes.push(this.search_name); | ||
| if (this.location_name) this.site_codes.push(this.location_name); | ||
| if (this.formatted_name) this.site_codes.push(this.formatted_name); | ||
|
|
||
| // Check for duplicate values in the grids array | ||
| const duplicateValues = this.grids.filter( | ||
| (value, index, self) => self.indexOf(value) !== index | ||
| ); | ||
| if (duplicateValues.length > 0) { | ||
| return next(new Error("Duplicate values found in grids array.")); | ||
| } | ||
| } | ||
|
|
||
| siteSchema.pre("update", function(next) { | ||
| if (this.isModified("latitude")) { | ||
| delete this.latitude; | ||
| next(); | ||
| } | ||
| if (this.isModified("longitude")) { | ||
| delete this.longitude; | ||
| } | ||
| if (this.isModified("_id")) { | ||
| delete this._id; | ||
| } | ||
| if (this.isModified("generated_name")) { | ||
| delete this.generated_name; | ||
| } | ||
| return next(); | ||
| }); | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Redundant pre-hook logic due to field immutability
Since latitude and longitude are marked as immutable: true, Mongoose will prevent any updates to these fields automatically. The pre-hook logic to remove these fields from updates is redundant.
Additionally, using the delete operator can impact performance. Consider removing this pre-hook or simplifying it to avoid unnecessary operations.
You might remove or adjust the pre-hook as follows:
-siteSchema.pre(
- ["updateOne", "findOneAndUpdate", "updateMany", "update", "save"],
- function(next) {
- // Handle updates
- if (this.getUpdate) {
- const updates = this.getUpdate();
- if (updates) {
- // Remove latitude and longitude from updates
- if (updates.latitude) delete updates.latitude;
- if (updates.longitude) delete updates.longitude;
- // Additional logic...
- }
- }
- next();
- }
-);
+// The immutability of latitude and longitude is enforced by Mongoose.
+// Additional pre-hook logic may not be necessary.Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome
[error] 405-405: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 406-406: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 410-410: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 411-411: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 416-416: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 417-417: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 425-425: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 426-426: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 427-427: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 428-428: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
src/device-registry/models/Site.js
Outdated
| siteSchema.post( | ||
| ["save", "updateOne", "findOneAndUpdate", "updateMany", "update"], | ||
| async function(doc, next) { | ||
| // Handle post-save logic | ||
| if (this.op === "save") { | ||
| // You can add any additional logic needed after saving a document here | ||
| // For example, logging, notifications, etc. | ||
| } | ||
|
|
||
| siteSchema.pre("save", function(next) { | ||
| if (this.isModified("latitude")) { | ||
| delete this.latitude; | ||
| } | ||
| if (this.isModified("longitude")) { | ||
| delete this.longitude; | ||
| } | ||
| if (this.isModified("_id")) { | ||
| delete this._id; | ||
| } | ||
| if (this.isModified("generated_name")) { | ||
| delete this.generated_name; | ||
| } | ||
| // Handle post-update logic | ||
| if (this.getUpdate) { | ||
| const updates = this.getUpdate(); | ||
| if (updates && updates.$set) { | ||
| // Check if latitude or longitude are being modified | ||
| if (updates.$set.latitude || updates.$set.longitude) { | ||
| return next( | ||
| new HttpError( | ||
| "Cannot modify latitude or longitude after creation", | ||
| httpStatus.BAD_REQUEST, | ||
| { | ||
| message: "Cannot modify latitude or longitude after creation", | ||
| } | ||
| ) | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| this.site_codes = [this._id, this.name, this.generated_name, this.lat_long]; | ||
| if (this.search_name) { | ||
| this.site_codes.push(this.search_name); | ||
| } | ||
| if (this.location_name) { | ||
| this.site_codes.push(this.location_name); | ||
| } | ||
| if (this.formatted_name) { | ||
| this.site_codes.push(this.formatted_name); | ||
| next(); | ||
| } | ||
| ); |
There was a problem hiding this comment.
Potential misuse of this.getUpdate() in post-hook
In the post-hook, this.getUpdate() is used, but in post-hooks, this refers to the document, and getUpdate() may not be available. This could lead to unexpected behavior.
Consider moving this logic to a pre-hook where this.getUpdate() is accessible, or adjust the code to correctly access the updated fields in the post-hook.
Apply this change to fix the issue:
-siteSchema.post(
+siteSchema.pre(
["updateOne", "findOneAndUpdate", "updateMany", "update"],
- async function(doc, next) {
+ function(next) {
// Handle update logic
const updates = this.getUpdate();
if (updates && updates.$set) {
// Check if latitude or longitude are being modified
if (updates.$set.latitude || updates.$set.longitude) {
return next(
new HttpError(
"Cannot modify latitude or longitude after creation",
httpStatus.BAD_REQUEST,
{
message: "Cannot modify latitude or longitude after creation",
}
)
);
}
}
next();
}
);📝 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.
| siteSchema.post( | |
| ["save", "updateOne", "findOneAndUpdate", "updateMany", "update"], | |
| async function(doc, next) { | |
| // Handle post-save logic | |
| if (this.op === "save") { | |
| // You can add any additional logic needed after saving a document here | |
| // For example, logging, notifications, etc. | |
| } | |
| siteSchema.pre("save", function(next) { | |
| if (this.isModified("latitude")) { | |
| delete this.latitude; | |
| } | |
| if (this.isModified("longitude")) { | |
| delete this.longitude; | |
| } | |
| if (this.isModified("_id")) { | |
| delete this._id; | |
| } | |
| if (this.isModified("generated_name")) { | |
| delete this.generated_name; | |
| } | |
| // Handle post-update logic | |
| if (this.getUpdate) { | |
| const updates = this.getUpdate(); | |
| if (updates && updates.$set) { | |
| // Check if latitude or longitude are being modified | |
| if (updates.$set.latitude || updates.$set.longitude) { | |
| return next( | |
| new HttpError( | |
| "Cannot modify latitude or longitude after creation", | |
| httpStatus.BAD_REQUEST, | |
| { | |
| message: "Cannot modify latitude or longitude after creation", | |
| } | |
| ) | |
| ); | |
| } | |
| } | |
| } | |
| this.site_codes = [this._id, this.name, this.generated_name, this.lat_long]; | |
| if (this.search_name) { | |
| this.site_codes.push(this.search_name); | |
| } | |
| if (this.location_name) { | |
| this.site_codes.push(this.location_name); | |
| } | |
| if (this.formatted_name) { | |
| this.site_codes.push(this.formatted_name); | |
| next(); | |
| } | |
| ); | |
| siteSchema.pre( | |
| ["updateOne", "findOneAndUpdate", "updateMany", "update"], | |
| function(next) { | |
| // Handle update logic | |
| const updates = this.getUpdate(); | |
| if (updates && updates.$set) { | |
| // Check if latitude or longitude are being modified | |
| if (updates.$set.latitude || updates.$set.longitude) { | |
| return next( | |
| new HttpError( | |
| "Cannot modify latitude or longitude after creation", | |
| httpStatus.BAD_REQUEST, | |
| { | |
| message: "Cannot modify latitude or longitude after creation", | |
| } | |
| ) | |
| ); | |
| } | |
| } | |
| next(); | |
| } | |
| ); |
🧰 Tools
🪛 Biome
[error] 377-377: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
|
Device registry changes in this PR available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/device-registry/models/Site.js (2)
402-419: Optimize save operation handlingThe save operation handling can be simplified by leveraging Mongoose's immutable option and using undefined assignments instead of delete operations for better performance.
// Handle save operation if (this.isNew || this.isModified()) { - // Remove specific fields on save - if (this.isModified("latitude")) delete this.latitude; - if (this.isModified("longitude")) delete this.longitude; - if (this.isModified("_id")) delete this._id; - if (this.isModified("generated_name")) delete this.generated_name; + // Set fields to undefined instead of using delete + if (this.isModified("latitude")) this.latitude = undefined; + if (this.isModified("longitude")) this.longitude = undefined; + if (this.isModified("_id")) this._id = undefined; + if (this.isModified("generated_name")) this.generated_name = undefined; // Update site_codes array this.site_codes = [ this._id, this.name, this.generated_name, this.lat_long, ]; if (this.search_name) this.site_codes.push(this.search_name); if (this.location_name) this.site_codes.push(this.location_name); if (this.formatted_name) this.site_codes.push(this.formatted_name); }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 402-402: src/device-registry/models/Site.js#L402
Added line #L402 was not covered by tests
[warning] 404-407: src/device-registry/models/Site.js#L404-L407
Added lines #L404 - L407 were not covered by tests
[warning] 410-410: src/device-registry/models/Site.js#L410
Added line #L410 was not covered by tests
[warning] 416-418: src/device-registry/models/Site.js#L416-L418
Added lines #L416 - L418 were not covered by tests🪛 Biome
[error] 404-404: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 405-405: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 406-406: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 407-407: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
369-429: Add test coverage for coordinate modification preventionThe coordinate modification prevention logic lacks test coverage. Please add tests for:
- Direct field updates
- Updates using $set operator
- Updates using $push operator
- Save operation modifications
Would you like me to help create comprehensive test cases for these scenarios?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 369-371: src/device-registry/models/Site.js#L369-L371
Added lines #L369 - L371 were not covered by tests
[warning] 373-374: src/device-registry/models/Site.js#L373-L374
Added lines #L373 - L374 were not covered by tests
[warning] 377-379: src/device-registry/models/Site.js#L377-L379
Added lines #L377 - L379 were not covered by tests
[warning] 389-390: src/device-registry/models/Site.js#L389-L390
Added lines #L389 - L390 were not covered by tests
[warning] 394-396: src/device-registry/models/Site.js#L394-L396
Added lines #L394 - L396 were not covered by tests
[warning] 402-402: src/device-registry/models/Site.js#L402
Added line #L402 was not covered by tests
[warning] 404-407: src/device-registry/models/Site.js#L404-L407
Added lines #L404 - L407 were not covered by tests
[warning] 410-410: src/device-registry/models/Site.js#L410
Added line #L410 was not covered by tests
[warning] 416-418: src/device-registry/models/Site.js#L416-L418
Added lines #L416 - L418 were not covered by tests
[warning] 421-422: src/device-registry/models/Site.js#L421-L422
Added lines #L421 - L422 were not covered by tests
[warning] 424-425: src/device-registry/models/Site.js#L424-L425
Added lines #L424 - L425 were not covered by tests
[warning] 429-429: src/device-registry/models/Site.js#L429
Added line #L429 was not covered by tests🪛 Biome
[error] 373-373: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 374-374: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 389-389: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 390-390: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 395-395: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 396-396: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 404-404: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 405-405: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 406-406: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 407-407: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/device-registry/models/Site.js(3 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/device-registry/models/Site.js
[warning] 369-371: src/device-registry/models/Site.js#L369-L371
Added lines #L369 - L371 were not covered by tests
[warning] 373-374: src/device-registry/models/Site.js#L373-L374
Added lines #L373 - L374 were not covered by tests
[warning] 377-379: src/device-registry/models/Site.js#L377-L379
Added lines #L377 - L379 were not covered by tests
[warning] 389-390: src/device-registry/models/Site.js#L389-L390
Added lines #L389 - L390 were not covered by tests
[warning] 394-396: src/device-registry/models/Site.js#L394-L396
Added lines #L394 - L396 were not covered by tests
[warning] 402-402: src/device-registry/models/Site.js#L402
Added line #L402 was not covered by tests
[warning] 404-407: src/device-registry/models/Site.js#L404-L407
Added lines #L404 - L407 were not covered by tests
[warning] 410-410: src/device-registry/models/Site.js#L410
Added line #L410 was not covered by tests
[warning] 416-418: src/device-registry/models/Site.js#L416-L418
Added lines #L416 - L418 were not covered by tests
[warning] 421-422: src/device-registry/models/Site.js#L421-L422
Added lines #L421 - L422 were not covered by tests
[warning] 424-425: src/device-registry/models/Site.js#L424-L425
Added lines #L424 - L425 were not covered by tests
[warning] 429-429: src/device-registry/models/Site.js#L429
Added line #L429 was not covered by tests
🪛 Biome
src/device-registry/models/Site.js
[error] 373-373: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 374-374: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 389-389: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 390-390: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 395-395: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 396-396: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 404-404: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 405-405: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 406-406: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 407-407: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (1)
src/device-registry/models/Site.js (1)
130-130: LGTM! Good use of Mongoose's immutable option.
The addition of immutable: true to latitude and longitude fields is an excellent first line of defense against coordinate modifications. This leverages Mongoose's built-in functionality for field immutability.
Also applies to: 139-139
src/device-registry/models/Site.js
Outdated
| siteSchema.pre( | ||
| ["updateOne", "findOneAndUpdate", "updateMany", "update", "save"], | ||
| function(next) { | ||
| // Handle updates | ||
| if (this.getUpdate) { | ||
| const updates = this.getUpdate(); | ||
| if (updates) { | ||
| // Remove latitude and longitude from updates | ||
| if (updates.latitude) delete updates.latitude; | ||
| if (updates.longitude) delete updates.longitude; | ||
|
|
||
| // Check for $set operator | ||
| if (updates.$set) { | ||
| if (updates.$set.latitude || updates.$set.longitude) { | ||
| return next( | ||
| new HttpError( | ||
| "Cannot modify latitude or longitude after creation", | ||
| httpStatus.BAD_REQUEST, | ||
| { | ||
| message: "Cannot modify latitude or longitude after creation", | ||
| } | ||
| ) | ||
| ); | ||
| } | ||
| if (updates.$set.latitude) delete updates.$set.latitude; | ||
| if (updates.$set.longitude) delete updates.$set.longitude; | ||
| } | ||
|
|
||
| // Check for $push operator | ||
| if (updates.$push) { | ||
| if (updates.$push.latitude) delete updates.$push.latitude; | ||
| if (updates.$push.longitude) delete updates.$push.longitude; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify update prevention logic
The current implementation has redundant checks since the fields are already marked as immutable. Consider simplifying the pre-hook logic:
siteSchema.pre(
["updateOne", "findOneAndUpdate", "updateMany", "update", "save"],
function(next) {
if (this.getUpdate) {
const updates = this.getUpdate();
if (updates) {
- // Remove latitude and longitude from updates
- if (updates.latitude) delete updates.latitude;
- if (updates.longitude) delete updates.longitude;
// Check for $set operator
if (updates.$set && (updates.$set.latitude || updates.$set.longitude)) {
return next(
new HttpError(
"Cannot modify latitude or longitude after creation",
httpStatus.BAD_REQUEST,
{
message: "Cannot modify latitude or longitude after creation",
}
)
);
}
- // These deletes are redundant due to immutable: true
- if (updates.$set) {
- if (updates.$set.latitude) delete updates.$set.latitude;
- if (updates.$set.longitude) delete updates.$set.longitude;
- }
- // Check for $push operator
- if (updates.$push) {
- if (updates.$push.latitude) delete updates.$push.latitude;
- if (updates.$push.longitude) delete updates.$push.longitude;
- }
}
}
next();
}
);📝 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.
| siteSchema.pre( | |
| ["updateOne", "findOneAndUpdate", "updateMany", "update", "save"], | |
| function(next) { | |
| // Handle updates | |
| if (this.getUpdate) { | |
| const updates = this.getUpdate(); | |
| if (updates) { | |
| // Remove latitude and longitude from updates | |
| if (updates.latitude) delete updates.latitude; | |
| if (updates.longitude) delete updates.longitude; | |
| // Check for $set operator | |
| if (updates.$set) { | |
| if (updates.$set.latitude || updates.$set.longitude) { | |
| return next( | |
| new HttpError( | |
| "Cannot modify latitude or longitude after creation", | |
| httpStatus.BAD_REQUEST, | |
| { | |
| message: "Cannot modify latitude or longitude after creation", | |
| } | |
| ) | |
| ); | |
| } | |
| if (updates.$set.latitude) delete updates.$set.latitude; | |
| if (updates.$set.longitude) delete updates.$set.longitude; | |
| } | |
| // Check for $push operator | |
| if (updates.$push) { | |
| if (updates.$push.latitude) delete updates.$push.latitude; | |
| if (updates.$push.longitude) delete updates.$push.longitude; | |
| } | |
| } | |
| } | |
| siteSchema.pre( | |
| ["updateOne", "findOneAndUpdate", "updateMany", "update", "save"], | |
| function(next) { | |
| // Handle updates | |
| if (this.getUpdate) { | |
| const updates = this.getUpdate(); | |
| if (updates) { | |
| // Check for $set operator | |
| if (updates.$set && (updates.$set.latitude || updates.$set.longitude)) { | |
| return next( | |
| new HttpError( | |
| "Cannot modify latitude or longitude after creation", | |
| httpStatus.BAD_REQUEST, | |
| { | |
| message: "Cannot modify latitude or longitude after creation", | |
| } | |
| ) | |
| ); | |
| } | |
| } | |
| } | |
| next(); | |
| } | |
| ); |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 369-371: src/device-registry/models/Site.js#L369-L371
Added lines #L369 - L371 were not covered by tests
[warning] 373-374: src/device-registry/models/Site.js#L373-L374
Added lines #L373 - L374 were not covered by tests
[warning] 377-379: src/device-registry/models/Site.js#L377-L379
Added lines #L377 - L379 were not covered by tests
[warning] 389-390: src/device-registry/models/Site.js#L389-L390
Added lines #L389 - L390 were not covered by tests
[warning] 394-396: src/device-registry/models/Site.js#L394-L396
Added lines #L394 - L396 were not covered by tests
🪛 Biome
[error] 373-373: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 374-374: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 389-389: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 390-390: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 395-395: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 396-396: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
| const duplicateValues = this.grids.filter( | ||
| (value, index, self) => self.indexOf(value) !== index | ||
| ); | ||
| if (duplicateValues.length > 0) { | ||
| return next(new Error("Duplicate values found in grids array.")); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Missing test coverage for grids array duplicate validation
The duplicate check logic for the grids array in Site.js is not covered by any test cases, as confirmed by searching through the test files. This validation is critical as it prevents duplicate grid assignments to sites which could lead to data inconsistencies.
Please add test cases to cover:
- Validation success when no duplicates exist in the grids array
- Validation failure when duplicates are present in the grids array
- Error message verification for duplicate grid entries
The test should be added to src/device-registry/models/test/ut_site.js and follow the existing test patterns.
🔗 Analysis chain
Add test coverage for grid duplicate check
The duplicate check logic for the grids array is not covered by tests. This is a critical validation that needs test coverage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing test coverage of grids array duplicate validation
# Search for test files related to Site model
fd -e test.js -e spec.js site
# Search for test cases covering grids array validation
rg -l "grids.*duplicate" -g "*.test.js" -g "*.spec.js"
Length of output: 237
Script:
#!/bin/bash
# Let's try a more comprehensive search approach
# First, find all test files in the repository
echo "=== Finding test files ==="
fd ".*\.(test|spec)\.(js|ts|jsx|tsx)$"
# Then search for Site model tests specifically
echo -e "\n=== Searching for Site model tests ==="
rg -l "Site" -g "*.(test|spec).{js,ts,jsx,tsx}"
# Search for grid-related tests with more context
echo -e "\n=== Searching for grid-related tests ==="
rg "grid" -i -g "*.(test|spec).{js,ts,jsx,tsx}" -C 2
# Look for validation tests in general
echo -e "\n=== Searching for validation tests ==="
rg "validate|validation" -i -g "*.(test|spec).{js,ts,jsx,tsx}" -C 2
Length of output: 1004
Script:
#!/bin/bash
# Let's try a different approach focusing on the specific file location
# First, let's find the exact location of the Site model test file
echo "=== Looking for Site model and its test files ==="
fd -t f "Site" src/device-registry
# Now let's look for any test files in the device-registry directory
echo -e "\n=== Finding test files in device-registry ==="
fd -t f "(test|spec)" src/device-registry
# Search for grid-related tests in the device-registry directory
echo -e "\n=== Searching for grid-related content in device-registry ==="
rg "grid" -i src/device-registry -C 2
# Look for validation tests in the device-registry directory
echo -e "\n=== Searching for validation tests in device-registry ==="
rg "validate|validation" -i src/device-registry -C 2
Length of output: 433066
Script:
#!/bin/bash
# Let's search for test files specifically related to Site model and grid validation
# First, find test files for Site model
echo "=== Looking for Site model test files ==="
fd -t f "site.*\.(test|spec)\.(js|ts)" src/device-registry/models/test
# Then search for grid-related tests in Site model tests
echo -e "\n=== Searching for grid validation tests in Site model tests ==="
rg "grids.*duplicate|duplicate.*grids" -C 3 src/device-registry/models/test
# Look for any test files that might test Site model's grid validation
echo -e "\n=== Searching for grid validation tests in all test files ==="
rg "Site.*grid.*validation|grid.*validation.*Site" -C 3 src/device-registry
Length of output: 603
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 421-422: src/device-registry/models/Site.js#L421-L422
Added lines #L421 - L422 were not covered by tests
[warning] 424-425: src/device-registry/models/Site.js#L424-L425
Added lines #L424 - L425 were not covered by tests
|
Device registry changes in this PR available for preview here |
Description
more checks to prevent modification of GPS coordinates
Changes Made
Testing
Affected Services
Endpoints Ready for Testing
API Documentation Updated?
Summary by CodeRabbit