Closed joshgachnang closed 3 weeks ago
**Action:** Run all tests (18.x, 6.0) |
**Failed stage:** [yarn lint](https://github.com/FlourishHealth/ferns-api/actions/runs/11133729646/job/30940439917) [❌] |
**Failed test name:** "" |
**Failure summary:**
The action failed due to a linting error detected by ESLint:src/populate.ts , at line 49, column 24, there is a Prettier formatting error.'__proto__' || firstKey === 'constructor' || firstKey === 'prototype' .--fix option with Prettier. |
Relevant error logs:```yaml 1: ##[group]Operating System 2: Ubuntu ... 286: electionTimeout: { called: Long([32m'0'[39m), successful: Long([32m'0'[39m) }, 287: freezeTimeout: { called: Long([32m'0'[39m), successful: Long([32m'0'[39m) }, 288: numStepDownsCausedByHigherTerm: Long([32m'0'[39m), 289: numCatchUps: Long([32m'0'[39m), 290: numCatchUpsSucceeded: Long([32m'0'[39m), 291: numCatchUpsAlreadyCaughtUp: Long([32m'0'[39m), 292: numCatchUpsSkipped: Long([32m'0'[39m), 293: numCatchUpsTimedOut: Long([32m'0'[39m), 294: numCatchUpsFailedWithError: Long([32m'0'[39m), 295: numCatchUpsFailedWithNewTerm: Long([32m'0'[39m), 296: numCatchUpsFailedWithReplSetAbortPrimaryCatchUpCmd: Long([32m'0'[39m), ... 547: [32m'Bytes released to the OS take up virtual address space but no physical memory.\n'[39m 548: } 549: }, 550: tenantMigrations: { 551: currentMigrationsDonating: Long([32m'0'[39m), 552: currentMigrationsReceiving: Long([32m'0'[39m), 553: totalSuccessfulMigrationsDonated: Long([32m'0'[39m), 554: totalSuccessfulMigrationsReceived: Long([32m'0'[39m), 555: totalFailedMigrationsDonated: Long([32m'0'[39m), 556: totalFailedMigrationsReceived: Long([32m'0'[39m) ... 663: [32m'eviction gave up due to needing to remove a record from the history store but checkpoint is running'[39m: [33m0[39m, 664: [32m'eviction passes of a file'[39m: [33m0[39m, 665: [32m'eviction server candidate queue empty when topping up'[39m: [33m0[39m, 666: [32m'eviction server candidate queue not empty when topping up'[39m: [33m0[39m, 667: [32m'eviction server evicting pages'[39m: [33m0[39m, 668: [32m'eviction server skips dirty pages during a running checkpoint'[39m: [33m0[39m, 669: [32m'eviction server skips metadata pages with history'[39m: [33m0[39m, 670: [32m'eviction server skips pages that are written with transactions greater than the last running'[39m: [33m0[39m, 671: [32m'eviction server skips pages that previously failed eviction and likely will again'[39m: [33m0[39m, ... 689: [32m'eviction walk target pages reduced due to history store cache pressure'[39m: [33m0[39m, 690: [32m'eviction walk target strategy both clean and dirty pages'[39m: [33m0[39m, 691: [32m'eviction walk target strategy only clean pages'[39m: [33m0[39m, 692: [32m'eviction walk target strategy only dirty pages'[39m: [33m0[39m, 693: [32m'eviction walks abandoned'[39m: [33m0[39m, 694: [32m'eviction walks gave up because they restarted their walk twice'[39m: [33m0[39m, 695: [32m'eviction walks gave up because they saw too many pages and found no candidates'[39m: [33m0[39m, 696: [32m'eviction walks gave up because they saw too many pages and found too few candidates'[39m: [33m0[39m, 697: [32m'eviction walks random search fails to locate a page, results in a null position'[39m: [33m0[39m, ... 703: [32m'eviction worker thread created'[39m: [33m0[39m, 704: [32m'eviction worker thread evicting pages'[39m: [33m0[39m, 705: [32m'eviction worker thread removed'[39m: [33m0[39m, 706: [32m'eviction worker thread stable number'[39m: [33m0[39m, 707: [32m'files with active eviction walks'[39m: [33m0[39m, 708: [32m'files with new eviction walks started'[39m: [33m0[39m, 709: [32m'force re-tuning of eviction workers once in a while'[39m: [33m0[39m, 710: [32m'forced eviction - do not retry count to evict pages selected to evict during reconciliation'[39m: [33m0[39m, 711: [32m'forced eviction - history store pages failed to evict while session has history store cursor open'[39m: [33m0[39m, ... 769: [32m'pages read into cache after truncate'[39m: [33m7[39m, 770: [32m'pages read into cache after truncate in prepare state'[39m: [33m0[39m, 771: [32m'pages removed from the ordinary queue to be queued for urgent eviction'[39m: [33m0[39m, 772: [32m'pages requested from the cache'[39m: [33m188[39m, 773: [32m'pages seen by eviction walk'[39m: [33m0[39m, 774: [32m'pages seen by eviction walk that are already queued'[39m: [33m0[39m, 775: [32m'pages selected for eviction unable to be evicted'[39m: [33m0[39m, 776: [32m'pages selected for eviction unable to be evicted because of active children on an internal page'[39m: [33m0[39m, 777: [32m'pages selected for eviction unable to be evicted because of failure in reconciliation'[39m: [33m0[39m, ... 1029: session: { 1030: [32m'attempts to remove a local object and the object is in use'[39m: [33m0[39m, 1031: [32m'flush_tier operation calls'[39m: [33m0[39m, 1032: [32m'flush_tier tables skipped due to no checkpoint'[39m: [33m0[39m, 1033: [32m'flush_tier tables switched'[39m: [33m0[39m, 1034: [32m'local objects removed'[39m: [33m0[39m, 1035: [32m'open session count'[39m: [33m15[39m, 1036: [32m'session query timestamp calls'[39m: [33m0[39m, 1037: [32m'table alter failed calls'[39m: [33m0[39m, 1038: [32m'table alter successful calls'[39m: [33m0[39m, 1039: [32m'table alter triggering checkpoint calls'[39m: [33m0[39m, 1040: [32m'table alter unchanged and skipped'[39m: [33m0[39m, 1041: [32m'table compact failed calls'[39m: [33m0[39m, 1042: [32m'table compact failed calls due to cache pressure'[39m: [33m0[39m, 1043: [32m'table compact running'[39m: [33m0[39m, 1044: [32m'table compact skipped as process would not reduce file size'[39m: [33m0[39m, 1045: [32m'table compact successful calls'[39m: [33m0[39m, 1046: [32m'table compact timeout'[39m: [33m0[39m, 1047: [32m'table create failed calls'[39m: [33m0[39m, 1048: [32m'table create successful calls'[39m: [33m9[39m, 1049: [32m'table drop failed calls'[39m: [33m0[39m, 1050: [32m'table drop successful calls'[39m: [33m0[39m, 1051: [32m'table rename failed calls'[39m: [33m0[39m, 1052: [32m'table rename successful calls'[39m: [33m0[39m, 1053: [32m'table salvage failed calls'[39m: [33m0[39m, 1054: [32m'table salvage successful calls'[39m: [33m0[39m, 1055: [32m'table truncate failed calls'[39m: [33m0[39m, 1056: [32m'table truncate successful calls'[39m: [33m0[39m, 1057: [32m'table verify failed calls'[39m: [33m0[39m, ... 1159: [32m'transactions rolled back'[39m: [33m9[39m, 1160: [32m'update conflicts'[39m: [33m0[39m 1161: }, 1162: concurrentTransactions: { 1163: write: { out: [33m0[39m, available: [33m128[39m, totalTickets: [33m128[39m }, 1164: read: { out: [33m0[39m, available: [33m128[39m, totalTickets: [33m128[39m } 1165: }, 1166: [32m'snapshot-window-settings'[39m: { 1167: [32m'total number of SnapshotTooOld errors'[39m: Long([32m'0'[39m), ... 1251: [32m'$skip'[39m: Long([32m'0'[39m), 1252: [32m'$sort'[39m: Long([32m'0'[39m), 1253: [32m'$sortByCount'[39m: Long([32m'0'[39m), 1254: [32m'$unionWith'[39m: Long([32m'0'[39m), 1255: [32m'$unset'[39m: Long([32m'0'[39m), 1256: [32m'$unwind'[39m: Long([32m'0'[39m), 1257: [32m'$vectorSearch'[39m: Long([32m'0'[39m) 1258: }, 1259: changeStreams: { largeEventsFailed: Long([32m'0'[39m), largeEventsSplit: Long([32m'0'[39m) }, 1260: commands: { 1261: [32m' |
⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪ |
🧪 PR contains tests |
🔒 No security concerns identified |
⚡ Key issues to review Breaking Change Removal of function-based populatePaths option may break existing implementations Performance Concern Recursive unpopulate function may be inefficient for deeply nested structures Code Duplication Potential duplication of model conversion logic between openApi.ts and populate.ts |
Category | Suggestion | Score |
Error handling |
Add error handling for cases where the populate model is not found___ **Consider adding error handling for the case whenpopulateModel is undefined after attempting to retrieve it. This could prevent potential runtime errors if the model is not found.** [src/populate.ts [147-153]](https://github.com/FlourishHealth/ferns-api/pull/444/files#diff-fe6875cb88f2efad3be804290e45138f69636fd6fc90eeae56768702cfce1484R147-R153) ```diff let populateModel = model.schema.path(populatePath.path)?.options?.ref; const populatePathIsArray = Array.isArray(model.schema.path(populatePath.path).options.type); if (populatePathIsArray) { populateModel = model.schema.path(populatePath.path).options.type[0].ref; } if (!populateModel) { - return; + throw new Error(`Unable to find populate model for path: ${populatePath.path}`); } ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 9Why: Adding error handling for undefined `populateModel` is crucial for preventing runtime errors and ensuring the application can handle unexpected situations gracefully. This suggestion addresses a potential bug and improves code reliability. | 9 |
Code simplification |
Simplify nested property access using optional chaining and nullish coalescing___ **Consider using optional chaining and nullish coalescing operators to simplify thenested property access and provide a default value for populateModel .**
[src/populate.ts [147-151]](https://github.com/FlourishHealth/ferns-api/pull/444/files#diff-fe6875cb88f2efad3be804290e45138f69636fd6fc90eeae56768702cfce1484R147-R151)
```diff
-let populateModel = model.schema.path(populatePath.path)?.options?.ref;
-const populatePathIsArray = Array.isArray(model.schema.path(populatePath.path).options.type);
-if (populatePathIsArray) {
- populateModel = model.schema.path(populatePath.path).options.type[0].ref;
-}
+const populatePathOptions = model.schema.path(populatePath.path)?.options;
+const populateModel = populatePathOptions?.ref ??
+ (Array.isArray(populatePathOptions?.type) ? populatePathOptions.type[0].ref : undefined);
```
- [ ] **Apply this suggestion**
Suggestion importance[1-10]: 8Why: The suggestion simplifies the code by using modern JavaScript features, making it more concise and readable. This change also ensures that `populateModel` is assigned a default value if not found, enhancing robustness. | 8 |
Type safety |
Improve type safety by using more specific types for variables___ **Consider using a more specific type forpopulateModel instead of any . This could be a generic type parameter or a union of possible model types.** [src/populate.ts [147-151]](https://github.com/FlourishHealth/ferns-api/pull/444/files#diff-fe6875cb88f2efad3be804290e45138f69636fd6fc90eeae56768702cfce1484R147-R151) ```diff -let populateModel = model.schema.path(populatePath.path)?.options?.ref; +let populateModel: string | undefined = model.schema.path(populatePath.path)?.options?.ref; const populatePathIsArray = Array.isArray(model.schema.path(populatePath.path).options.type); if (populatePathIsArray) { populateModel = model.schema.path(populatePath.path).options.type[0].ref; } ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: The suggestion improves type safety by specifying a more precise type for `populateModel`. This change enhances code maintainability and reduces potential runtime errors related to type mismatches. | 7 |
Naming convention |
Use a more descriptive variable name for imported modules to improve readability___ **Consider using a more descriptive variable name form2s to improve code readability. The full name "mongoose-to-swagger" or an abbreviation like "mongooseToSwagger" would be clearer.** [src/populate.ts [4]](https://github.com/FlourishHealth/ferns-api/pull/444/files#diff-fe6875cb88f2efad3be804290e45138f69636fd6fc90eeae56768702cfce1484R4-R4) ```diff -import m2s from "mongoose-to-swagger"; +import mongooseToSwagger from "mongoose-to-swagger"; ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 5Why: While the suggestion improves code readability by using a more descriptive variable name, it is a minor enhancement and does not address any functional issues. It contributes to better maintainability and understanding of the code. | 5 |
💡 Need additional feedback ? start a PR chat
User description
Ditch the option to pass a function for populatePaths. This was causing a lot of type errors and was from the days before responseHandler was used all over. Now versioning can be handled in responseHandler. Add a utility function unpopulate to help with versioning (for clients that shouldn't have a population).
Update the signature for PopulatePaths to allow limiting the fields that are populated and an openApiComponent to allow a nice, reusable component, which makes frontend SDKs nicer.
This is a backwards incompatible change, so apps using this will need to change their populatePaths to match the new style. I tried to make it work but typing and OpenAPI wasn't possible.
PR Type
enhancement, tests
Description
populatePaths
and introducingaddPopulateToQuery
.FernsRouterOptions
to use the newPopulatePath
type, allowing more flexible population configurations.Changes walkthrough 📝
4 files
api.test.ts
Update and extend tests for populate functionality
src/api.test.ts
populatePaths
to use objects withpath
andfields
.openApi.test.ts
Add OpenAPI tests for populated fields
src/openApi.test.ts
addRoutesPopulate
for testing OpenAPI population.populate.test.ts
Add tests for unpopulate function
src/populate.test.ts
unpopulate
function.tests.ts
Extend test schemas for population testing
src/tests.ts
User
andFood
interfaces with new fields.likesSchema
for nested population testing.6 files
api.ts
Refactor population handling in API
src/api.ts
populatePaths
.addPopulateToQuery
for handling population.FernsRouterOptions
to usePopulatePath
type.expressServer.ts
Enhance error logging in server setup
src/expressServer.ts - Improved error logging by including error stack.
index.ts
Export populate module
src/index.ts - Exported new `populate` module.
openApi.ts
Refactor OpenAPI model conversion
src/openApi.ts
convertModel
function.convertModel
frompopulate
module.permissions.ts
Update permissions middleware for new population method
src/permissions.ts
populate
withaddPopulateToQuery
for query population.populate.ts
Implement population utilities and OpenAPI integration
src/populate.ts
addPopulateToQuery
andunpopulate
functions.convertModel
for OpenAPI integration.