Closed joshgachnang closed 2 months ago
โฑ๏ธ Estimated effort to review: 3 ๐ต๐ต๐ตโชโช |
๐งช PR contains tests |
๐ No security concerns identified |
โก Key issues to review Error Handling The error message in the `DateOnly` class cast method might not provide enough context about why the date is invalid. Consider enhancing the error details to help developers understand the cause of the error more clearly. Type Safety The casting logic in `DateOnly` uses `any` type for the input value, which could lead to potential type safety issues. Consider using more specific type annotations to improve code robustness and maintainability. |
**Action:** Run all tests (16.x, 6.0) |
**Failed stage:** [yarn lint](https://github.com/FlourishHealth/ferns-api/actions/runs/10236711031/job/28319104940) [โ] |
**Failure summary:**
The action failed because the linting process detected two errors in the file src/plugins.ts :no-console rule).no-console rule). |
Relevant error logs:```yaml 1: ##[group]Operating System 2: Ubuntu ... 288: electionTimeout: { called: Long([32m'0'[39m), successful: Long([32m'0'[39m) }, 289: freezeTimeout: { called: Long([32m'0'[39m), successful: Long([32m'0'[39m) }, 290: numStepDownsCausedByHigherTerm: Long([32m'0'[39m), 291: numCatchUps: Long([32m'0'[39m), 292: numCatchUpsSucceeded: Long([32m'0'[39m), 293: numCatchUpsAlreadyCaughtUp: Long([32m'0'[39m), 294: numCatchUpsSkipped: Long([32m'0'[39m), 295: numCatchUpsTimedOut: Long([32m'0'[39m), 296: numCatchUpsFailedWithError: Long([32m'0'[39m), 297: numCatchUpsFailedWithNewTerm: Long([32m'0'[39m), 298: numCatchUpsFailedWithReplSetAbortPrimaryCatchUpCmd: Long([32m'0'[39m), ... 548: [32m'Bytes released to the OS take up virtual address space but no physical memory.\n'[39m 549: } 550: }, 551: tenantMigrations: { 552: currentMigrationsDonating: Long([32m'0'[39m), 553: currentMigrationsReceiving: Long([32m'0'[39m), 554: totalSuccessfulMigrationsDonated: Long([32m'0'[39m), 555: totalSuccessfulMigrationsReceived: Long([32m'0'[39m), 556: totalFailedMigrationsDonated: Long([32m'0'[39m), 557: totalFailedMigrationsReceived: Long([32m'0'[39m) ... 664: [32m'eviction gave up due to needing to remove a record from the history store but checkpoint is running'[39m: [33m0[39m, 665: [32m'eviction passes of a file'[39m: [33m0[39m, 666: [32m'eviction server candidate queue empty when topping up'[39m: [33m0[39m, 667: [32m'eviction server candidate queue not empty when topping up'[39m: [33m0[39m, 668: [32m'eviction server evicting pages'[39m: [33m0[39m, 669: [32m'eviction server skips dirty pages during a running checkpoint'[39m: [33m0[39m, 670: [32m'eviction server skips metadata pages with history'[39m: [33m0[39m, 671: [32m'eviction server skips pages that are written with transactions greater than the last running'[39m: [33m0[39m, 672: [32m'eviction server skips pages that previously failed eviction and likely will again'[39m: [33m0[39m, ... 702: [32m'eviction worker thread created'[39m: [33m0[39m, 703: [32m'eviction worker thread evicting pages'[39m: [33m0[39m, 704: [32m'eviction worker thread removed'[39m: [33m0[39m, 705: [32m'eviction worker thread stable number'[39m: [33m0[39m, 706: [32m'files with active eviction walks'[39m: [33m0[39m, 707: [32m'files with new eviction walks started'[39m: [33m0[39m, 708: [32m'force re-tuning of eviction workers once in a while'[39m: [33m0[39m, 709: [32m'forced eviction - do not retry count to evict pages selected to evict during reconciliation'[39m: [33m0[39m, 710: [32m'forced eviction - history store pages failed to evict while session has history store cursor open'[39m: [33m0[39m, ... 767: [32m'pages read into cache after truncate'[39m: [33m7[39m, 768: [32m'pages read into cache after truncate in prepare state'[39m: [33m0[39m, 769: [32m'pages removed from the ordinary queue to be queued for urgent eviction'[39m: [33m0[39m, 770: [32m'pages requested from the cache'[39m: [33m188[39m, 771: [32m'pages seen by eviction walk'[39m: [33m0[39m, 772: [32m'pages seen by eviction walk that are already queued'[39m: [33m0[39m, 773: [32m'pages selected for eviction unable to be evicted'[39m: [33m0[39m, 774: [32m'pages selected for eviction unable to be evicted because of active children on an internal page'[39m: [33m0[39m, 775: [32m'pages selected for eviction unable to be evicted because of failure in reconciliation'[39m: [33m0[39m, ... 1027: session: { 1028: [32m'attempts to remove a local object and the object is in use'[39m: [33m0[39m, 1029: [32m'flush_tier operation calls'[39m: [33m0[39m, 1030: [32m'flush_tier tables skipped due to no checkpoint'[39m: [33m0[39m, 1031: [32m'flush_tier tables switched'[39m: [33m0[39m, 1032: [32m'local objects removed'[39m: [33m0[39m, 1033: [32m'open session count'[39m: [33m15[39m, 1034: [32m'session query timestamp calls'[39m: [33m0[39m, 1035: [32m'table alter failed calls'[39m: [33m0[39m, 1036: [32m'table alter successful calls'[39m: [33m0[39m, 1037: [32m'table alter triggering checkpoint calls'[39m: [33m0[39m, 1038: [32m'table alter unchanged and skipped'[39m: [33m0[39m, 1039: [32m'table compact failed calls'[39m: [33m0[39m, 1040: [32m'table compact failed calls due to cache pressure'[39m: [33m0[39m, 1041: [32m'table compact running'[39m: [33m0[39m, 1042: [32m'table compact skipped as process would not reduce file size'[39m: [33m0[39m, 1043: [32m'table compact successful calls'[39m: [33m0[39m, 1044: [32m'table compact timeout'[39m: [33m0[39m, 1045: [32m'table create failed calls'[39m: [33m0[39m, 1046: [32m'table create successful calls'[39m: [33m9[39m, 1047: [32m'table drop failed calls'[39m: [33m0[39m, 1048: [32m'table drop successful calls'[39m: [33m0[39m, 1049: [32m'table rename failed calls'[39m: [33m0[39m, 1050: [32m'table rename successful calls'[39m: [33m0[39m, 1051: [32m'table salvage failed calls'[39m: [33m0[39m, 1052: [32m'table salvage successful calls'[39m: [33m0[39m, 1053: [32m'table truncate failed calls'[39m: [33m0[39m, 1054: [32m'table truncate successful calls'[39m: [33m0[39m, 1055: [32m'table verify failed calls'[39m: [33m0[39m, ... 1157: [32m'transactions rolled back'[39m: [33m7[39m, 1158: [32m'update conflicts'[39m: [33m0[39m 1159: }, 1160: concurrentTransactions: { 1161: write: { out: [33m0[39m, available: [33m128[39m, totalTickets: [33m128[39m }, 1162: read: { out: [33m0[39m, available: [33m128[39m, totalTickets: [33m128[39m } 1163: }, 1164: [32m'snapshot-window-settings'[39m: { 1165: [32m'total number of SnapshotTooOld errors'[39m: Long([32m'0'[39m), ... 1249: [32m'$skip'[39m: Long([32m'0'[39m), 1250: [32m'$sort'[39m: Long([32m'0'[39m), 1251: [32m'$sortByCount'[39m: Long([32m'0'[39m), 1252: [32m'$unionWith'[39m: Long([32m'0'[39m), 1253: [32m'$unset'[39m: Long([32m'0'[39m), 1254: [32m'$unwind'[39m: Long([32m'0'[39m), 1255: [32m'$vectorSearch'[39m: Long([32m'0'[39m) 1256: }, 1257: changeStreams: { largeEventsFailed: Long([32m'0'[39m), largeEventsSplit: Long([32m'0'[39m) }, 1258: commands: { 1259: [32m' |
Category | Suggestion | Score |
Possible bug |
Handle
___
**To improve the robustness and maintainability of the | 9 |
Performance |
Cache computationally expensive operations to enhance performance___ **To enhance the performance and reduce redundancy, consider caching the result ofDateTime.fromJSDate(new Date(val)).toUTC().startOf("day") when the input val is a string or number, as this operation is computationally expensive and is repeated in the else if block.**
[src/plugins.ts [178-188]](https://github.com/FlourishHealth/ferns-api/pull/413/files#diff-5d37292cf21755714262793e7553ee317a12b157d0601e27a0ca26e096a60c00R178-R188)
```diff
-const date = DateTime.fromJSDate(new Date(val)).toUTC().startOf("day");
-if (!date.isValid) {
+const parsedDate = DateTime.fromJSDate(new Date(val)).toUTC().startOf("day");
+if (!parsedDate.isValid) {
throw new MongooseError.CastError(
"DateOnly",
val,
this.path,
new Error("Value is not a valid date")
);
}
-return date.toJSDate();
+return parsedDate.toJSDate();
```
- [ ] **Apply this suggestion**
Suggestion importance[1-10]: 8Why: Caching the result of a computationally expensive operation can significantly enhance performance and reduce redundancy. This is a valuable optimization for the `DateOnly` class. | 8 |
Enhancement |
Add back route logging for better traceability and debugging___ **Consider adding back the logging of routes during the initialization for bettertraceability and debugging. This can be particularly useful in development environments or when diagnosing issues in production.** [src/expressServer.ts [281]](https://github.com/FlourishHealth/ferns-api/pull/413/files#diff-d8c51792951866599be44999fb26695bc96b6b7435d64cab68e024b90efdeb1cR281-R281) ```diff +logger.debug("Listening on routes:"); +app._router.stack.forEach((r: any) => { + if (r.route && r.route.path) { + logger.debug(`[Route] ${r.route.path}`); + } +}); return app; ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: Adding back the route logging can be useful for debugging and traceability, especially in development environments. However, it may not be crucial for all use cases, so it is a moderate enhancement. | 7 |
Maintainability |
Use more descriptive variable names in test cases to enhance readability___ **Refactor the test case forDateOnly to use a more descriptive variable name than res for the result of the stuffModel.create method, enhancing code readability.**
[src/plugins.test.ts [208-213]](https://github.com/FlourishHealth/ferns-api/pull/413/files#diff-80dedbdb2ef4dd17540ab73a859430da8c9ce7d17a11ccffa1ea73834c320a6eR208-R213)
```diff
-const res = await stuffModel.create({
+const createdItem = await stuffModel.create({
name: "Things",
ownerId: "123",
date: "2005-10-10T17:17:17.017Z" as any,
});
-assert.strictEqual(res.date.toISOString(), "2005-10-10T00:00:00.000Z");
+assert.strictEqual(createdItem.date.toISOString(), "2005-10-10T00:00:00.000Z");
```
- [ ] **Apply this suggestion**
Suggestion importance[1-10]: 6Why: Using more descriptive variable names improves code readability and maintainability. This is a minor but valuable improvement for understanding the test cases. | 6 |
User description
Add a type that forces dates to end in 00:00:00.000Z UTC to match ferns-ui handling of date only fields.
Also remove the verbose but not very useful startup logging. It didn't include any of the FernsRouter routes and gets chatty with microservices and tests.
PR Type
enhancement, tests, dependencies
Description
DateOnly
schema type that normalizes dates to00:00:00.000Z
using Luxon.DateOnly
schema type to ensure proper date adjustment and error handling.package.json
to include theluxon
dependency for date handling.Changes walkthrough ๐
expressServer.ts
Remove verbose startup logging in express server
src/expressServer.ts
plugins.ts
Introduce `DateOnly` schema type for date normalization
src/plugins.ts
DateOnly
schema type.00:00:00.000Z
using Luxon.DateOnly
with Mongoose.plugins.test.ts
Add tests for `DateOnly` schema type
src/plugins.test.ts
DateOnly
schema type tests.00:00:00.000Z
.package.json
Add `luxon` dependency for date handling
package.json - Added `luxon` dependency.