#151 into release@4.0.0 🧊 add orm in interceptor #171
Conversation
|
Думаю надо добавить еще тесты (а то уже есть баги с findIndexById) и README обновить |
src/core/database/createDatabaseRoutes/storages/File/FileStorage.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
У Сереги примерно такая же middleware есть в PR логгера. Думаю можно в одну объединить ваши решения
There was a problem hiding this comment.
В принципе норм идея, всю инфу о реквесте и орм в request.context положить
| @@ -8,6 +10,7 @@ export interface RequestInterceptorParams { | |||
| getCookie: (name: string) => RequestInterceptorCookieValue; | |||
| getHeader: (field: string) => RequestInterceptorHeaderValue; | |||
| getHeaders: () => Record<string, RequestInterceptorHeaderValue>; | |||
| orm: Orm<Database>; | |||
There was a problem hiding this comment.
orm может быть undefined
There was a problem hiding this comment.
на самом деле не может это всегда {} пофиксил .? убрал
There was a problem hiding this comment.
но есть ньюанс, короче это бы обсудить
debabin
changed the title
src/utils/types/database.ts
Outdated
| delete: (id: StorageIndex) => void; | ||
|
|
||
| createMany: (items: Item[]) => void; | ||
| updateMany: (ids: StorageIndex[], item: Partial<Omit<Item, 'id'>>) => void; |
There was a problem hiding this comment.
Не очень понятна сигнатура функции - типа куча айдишников, но при этом один item - вряд ли когда то куча сущностей будут апдейтиться одним и тем же значением. Имхо тут логичнее бы смотрелась мапа { id => value }. Т.е. возможность обновления за один раз кучи сущностей, каждая из которых может быть обновлена своим уникальным значением
There was a problem hiding this comment.
опять же, пока брал как в других орм и как это работает, https://www.prisma.io/docs/orm/reference/prisma-client-reference#examples-14
src/utils/types/database.ts
Outdated
| } | ||
|
|
||
| export interface NestedOrm<Item = Record<string, unknown>> { | ||
| get: () => Item[]; |
There was a problem hiding this comment.
getAll мб более явное название будет?
There was a problem hiding this comment.
в итоге удалил его и сделал fillters необзательным для методов findMany и findFirst смотрел на prisma, добавил тест
|
|
||
| create: (item) => { | ||
| const collection = storage.read(key); | ||
| if (!collection) throw new Error('Collection not found'); |
There was a problem hiding this comment.
А такая ситуация же невозможна - коллекцию нереально полностью удалить
There was a problem hiding this comment.
забыл удалить, вообще человек может файл удалить или типо того, надо будет в 5 версии орм сделать норм отказоустойчивость
src/utils/types/database.ts
Outdated
| delete: (id: StorageIndex) => void; | ||
|
|
||
| createMany: (items: Item[]) => void; | ||
| updateMany: (ids: StorageIndex[], item: Partial<Omit<Item, 'id'>>) => void; |
There was a problem hiding this comment.
И еще в реальной имплементации возвращается длина, а не void. Имхо тут можно вернуть мапу { id => newValue }
There was a problem hiding this comment.
смотрел на анлоги и вообще это делается все с помощью ключей обычно, думаю замутим такое уже в 5, а сейчас сделаем просто length
src/core/rest/createRestRoutes/helpers/prepareRestRequestConfigs/prepareRestRequestConfigs.ts
Outdated
Show resolved
Hide resolved
| update: (id: StorageIndex, item: Partial<Omit<Item, 'id'>>) => void; | ||
| delete: (id: StorageIndex) => void; | ||
|
|
||
| createMany: (items: Omit<Item, 'id'>[]) => void; | ||
| updateMany: (ids: StorageIndex[], item: Partial<Omit<Item, 'id'>>) => number; |
There was a problem hiding this comment.
не кажется ли странным, что update не возвращает ничего, а updateMany возвращает что-то (в данном случае число)? Мб оба метода не должны возвращать что-либо?
There was a problem hiding this comment.
второй возращает количество записей, который поменялись в будущем по хорошему должна быть настройка для update, которая вернет еще и сущность 5.0.0 релиз ждем
| update: (id, item) => { | ||
| const collection = storage.read(key); | ||
| const currentResourceIndex = findIndexById(collection, id); | ||
|
|
||
| const currentResource = storage.read([key, currentResourceIndex]); | ||
| const updatedRecord = { ...currentResource, ...item, id }; | ||
|
|
||
| storage.write([key, currentResourceIndex], updatedRecord); | ||
|
|
||
| return updatedRecord; | ||
| }, |
There was a problem hiding this comment.
return updatedRecord;
в типизации для этого метода возвращаемое значение написано void.
По итогу планируется тут удалить return?
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces a comprehensive enhancement to the project's database and ORM (Object-Relational Mapping) capabilities. The changes focus on creating a flexible storage and data management system, with new utility functions for creating storage, implementing ORM interfaces, and extending middleware to support database context. The modifications improve type safety, introduce dynamic storage selection, and provide a more robust framework for handling data interactions across different server configurations. Changes
Sequence DiagramsequenceDiagram
participant Server as Mock Server
participant Middleware as Context Middleware
participant Storage as Storage Creator
participant ORM as ORM Creator
Server->>Middleware: Initialize with config
Middleware->>Storage: Create storage
Storage-->>Middleware: Return storage instance
Middleware->>ORM: Create ORM with storage
ORM-->>Middleware: Return ORM instance
Middleware->>Server: Attach context with ORM
Poem
🪧 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 (
|
| log, | ||
| orm: request.context?.orm |
There was a problem hiding this comment.
orm всегда будет существовать, не? Т.е. нет смысла в ?.
| createMany(items) { | ||
| items.forEach((item) => this.create(item)); | ||
| }, | ||
| updateMany(ids, item) { | ||
| ids.forEach((id) => this.update(id, item)); | ||
| return ids.length; | ||
| }, | ||
| deleteMany(ids) { | ||
| ids.forEach((id) => this.delete(id)); | ||
| }, |
There was a problem hiding this comment.
это обычные функции для сохранения контекста? Я хз насколько это реальный кейс, но можно просто пофлексить нереальными знаниями JSa и перед return orm на 98 строке делать orm[key].createMany = orm[key].createMany.bind(orm[key]) ну и так со всеми методами.
Но я хз, это не прям обязательно, просто чтобы точно защититься от того, что эту функцию вызовут в неправильном контексте
| orm = createOrm<typeof data>(storage); | ||
| }); | ||
|
|
||
| test('Should create many new resources', () => { |
There was a problem hiding this comment.
Название не соответствует тесту - тут только один ресурс создается
| test('Should update many resources', () => { | ||
| const usersLength = orm.users.updateMany([1, 2], { age: 40 }); | ||
| const users = orm.users.findMany(); | ||
|
|
||
| expect(usersLength).toBe(2); | ||
| expect(users).toStrictEqual([ | ||
| { | ||
| id: 1, | ||
| name: 'John Doe', | ||
| age: 40, | ||
| address: { city: 'Novosibirsk' }, | ||
| hobbies: ['music', 'sport'] | ||
| }, | ||
| { | ||
| id: 2, | ||
| name: 'Jane Smith', | ||
| age: 40, | ||
| address: { city: 'Tomsk' }, | ||
| hobbies: ['sport', 'games'] | ||
| }, | ||
| { id: 3, name: 'Will Smith', age: 27, address: { city: 'Moscow' }, hobbies: ['music'] } | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
во всех тестах до этого не было проверок всего вернувшегося массива, а тут есть. Вообще мб во всех тестах стоит сделать как раз проверку всего массива, т.к. такая проверка позволяет не только убедиться что сделано то, что нужно, но еще и то, что другие данные остались нетронутыми
| expect(remainingUsers.length).toBe(1); | ||
| expect(remainingUsers).toStrictEqual([ | ||
| { id: 3, name: 'Will Smith', age: 27, address: { city: 'Moscow' }, hobbies: ['music'] } | ||
| ]); |
There was a problem hiding this comment.
Вот типо такой проверки - проверили весь массив юзеров, а не только, что юзеры с id 1 и 2 пропали. А вдруг остальные были случайно мутированы? А подобные проверки исключают это
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (3)
src/core/database/createDatabaseRoutes/storages/Memory/MemoryStorage.ts (1)
Add array bounds validation to prevent unexpected behavior
The current implementation only validates that array indices are non-negative integers but doesn't check against the array's length. The suggested bounds checking would prevent potential silent failures when splicing arrays.
🔗 Analysis chain
Line range hint
38-53: Enhance array handling in delete operation.The array splice operation could be problematic if the index is out of bounds.
Add bounds checking:
if (Array.isArray(deletable) && isIndex(keys[index])) { + const arrayIndex = keys[index] as number; + if (arrayIndex >= 0 && arrayIndex < deletable.length) { - deletable.splice(keys[index] as number, 1); + deletable.splice(arrayIndex, 1); + } return; }🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find isIndex function implementation and StorageIndex type ast-grep --pattern 'function isIndex($_) { $$$ }' ast-grep --pattern 'type StorageIndex = $_' # Search for any array bounds validation rg "arrayIndex|bounds|< length|>= 0" -A 2Length of output: 533
src/core/database/createDatabaseRoutes/storages/File/FileStorage.ts (1)
Line range hint
35-47: Consider using async file operations.The
writemethod uses synchronous file operations which could block the event loop.Consider converting to async operations:
-public write(key: StorageIndex | StorageIndex[], value: unknown): void { +public async write(key: StorageIndex | StorageIndex[], value: unknown): Promise<void> { // ... existing key handling code ... writable[keys[index]] = value; - this.fileWriter.write(JSON.stringify(this.data)); + await this.fileWriter.writeAsync(JSON.stringify(this.data)); }Note: This would require updating the Storage interface and all implementations to support async operations.
package.json (1)
Line range hint
39-41: Update Node.js engine requirement to match esbuild's minimum version.The current Node.js engine requirement (
">=12") is incompatible with esbuild v0.21.4, which requires Node.js >= 14.14.0.Apply this diff to fix the compatibility issue:
"engines": { - "node": ">=12" + "node": ">=14.14.0" },
♻️ Duplicate comments (1)
src/core/database/createOrm/createOrm.ts (1)
41-50:⚠️ Potential issueBind methods to preserve context when using
thisThe methods
createMany,updateMany, anddeleteManyusethisto call other methods. Without proper binding,thismight not refer to the correct context when these methods are invoked, leading to runtime errors. To ensurethismaintains the correct reference, consider binding these methods to the ORM instance or using arrow functions.Apply this refactor to bind the methods:
- createMany(items) { + createMany: function (items) { items.forEach((item) => this.create(item)); }, - updateMany(ids, item) { + updateMany: function (ids, item) { ids.forEach((id) => this.update(id, item)); return ids.length; }, - deleteMany(ids) { + deleteMany: function (ids) { ids.forEach((id) => this.delete(id)); },Alternatively, bind the methods after definition:
orm[key].createMany = orm[key].createMany.bind(orm[key]); orm[key].updateMany = orm[key].updateMany.bind(orm[key]); orm[key].deleteMany = orm[key].deleteMany.bind(orm[key]);
🧹 Nitpick comments (8)
src/core/database/createOrm/createOrm.ts (1)
75-81: Use consistent type definitions for flattened resourcesIn the
findFirstmethod, when flattening resources and filters, ensure that consistent and specific types are used to maintain type safety and readability. This mirrors the suggestion made for thefindManymethod.Apply this change for consistency:
- const flattenedFilters = flatten<any, any>(filters); + const flattenedFilters = flatten<Record<string, unknown>, Record<string, unknown>>(filters);src/core/database/createStorage/createStorage.ts (1)
3-4: LGTM! Consider adding JSDoc comments.The type predicate function is well-implemented with proper type safety. Consider adding JSDoc comments to document the purpose and behavior.
+/** + * Type predicate to check if a variable is a string ending with '.json' + * @param variable - The variable to check + * @returns True if the variable is a string ending with '.json' + */ const isVariableJsonFile = (variable: unknown): variable is `${string}.json` => typeof variable === 'string' && variable.endsWith('.json');src/core/database/createDatabaseRoutes/createDatabaseRoutes.ts (1)
20-20: Consider using createStorage consistently.Good use of the factory function for storage creation. Consider using
createStoragefor the second storage creation as well to maintain consistency.- const storage = isVariableJsonFile(data) ? new FileStorage(data) : new MemoryStorage(data); + const storage = createStorage(data);src/utils/helpers/interceptors/callResponseInterceptors/callResponseInterceptors.ts (1)
77-78: Remove unnecessary optional chaining.Based on the codebase context, the
ormproperty will always exist in the request context, making the optional chaining (?.) unnecessary.Apply this diff to simplify the code:
- orm: request.context?.orm + orm: request.context.ormsrc/core/database/createOrm/createOrm.test.ts (4)
89-101: Test name doesn't match implementation.The test name suggests creating multiple resources, but it only creates one resource.
Rename the test to better reflect its implementation:
-test('Should create many new resources', () => { +test('Should create a new resource', () => {
138-141: Improve test assertion clarity.The current assertion compares array elements with themselves. Consider using
newUsersinstead ofusersfor clearer test intentions.Apply this diff to improve the assertion:
- expect([users[3], users[4]]).toStrictEqual([ - { ...newUsers[0], id: 4 }, - { ...newUsers[1], id: 5 } - ]); + expect([users[3], users[4]]).toStrictEqual([ + { ...newUsers[0], id: 4 }, + { ...newUsers[1], id: 5 } + ]); + // Also verify the entire users array to ensure other records weren't affected + expect(users).toStrictEqual([ + ...users.slice(0, 3), + { ...newUsers[0], id: 4 }, + { ...newUsers[1], id: 5 } + ]);
188-189: Improve variable naming.The variable name
undefinedUsercould be more descriptive.Apply this diff to improve clarity:
- const undefinedUser = orm.users.findById(999); + const nonExistentUser = orm.users.findById(999);
213-224: Add test for empty filter.The test suite is missing a test case for
findManywithout filters.Add a test case for
findManywithout filters:test('Should find many without filters', () => { const users = orm.users.findMany(); expect(users).toHaveLength(3); expect(users).toStrictEqual([ // ... expected users array ]); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.eslintrc.js(1 hunks)bin/validateMockServerConfig/graphqlConfigSchema/routeConfigSchema/routeConfigSchema.test.ts(2 hunks)bin/validateMockServerConfig/restConfigSchema/routeConfigSchema/routeConfigSchema.test.ts(2 hunks)package.json(1 hunks)src/core/database/createDatabaseRoutes/createDatabaseRoutes.ts(2 hunks)src/core/database/createDatabaseRoutes/storages/File/FileStorage.ts(3 hunks)src/core/database/createDatabaseRoutes/storages/Memory/MemoryStorage.ts(3 hunks)src/core/database/createOrm/createOrm.test.ts(1 hunks)src/core/database/createOrm/createOrm.ts(1 hunks)src/core/database/createStorage/createStorage.ts(1 hunks)src/core/database/index.ts(1 hunks)src/core/middlewares/contextMiddleware/contextMiddleware.ts(3 hunks)src/core/rest/createRestRoutes/createRestRoutes.test.ts(3 hunks)src/server/createDatabaseMockServer/createDatabaseMockServer.ts(1 hunks)src/server/createGraphQLMockServer/createGraphQLMockServer.ts(1 hunks)src/server/createMockServer/createMockServer.ts(1 hunks)src/server/createRestMockServer/createRestMockServer.ts(1 hunks)src/utils/helpers/interceptors/callRequestInterceptor/callRequestInterceptor.ts(1 hunks)src/utils/helpers/interceptors/callResponseInterceptors/callResponseInterceptors.ts(1 hunks)src/utils/types/database.ts(1 hunks)src/utils/types/interceptors.ts(3 hunks)src/utils/types/server.ts(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- bin/validateMockServerConfig/restConfigSchema/routeConfigSchema/routeConfigSchema.test.ts
- bin/validateMockServerConfig/graphqlConfigSchema/routeConfigSchema/routeConfigSchema.test.ts
🧰 Additional context used
🪛 Biome (1.9.4)
.eslintrc.js
[error] 10-10: This property value named @typescript-eslint/no-namespace is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named @typescript-eslint/no-namespace
(lint/suspicious/noDuplicateObjectKeys)
🔇 Additional comments (17)
src/utils/types/database.ts (2)
21-25: Consider making return types consistent for update methodsIn the
NestedOrminterface, theupdatemethod returnsvoid, whereasupdateManyreturns anumberindicating the count of updated records. For consistency, consider aligning the return types or providing a rationale for the difference.
38-41: Ensure accurate type inference in conditional typesIn the
Ormtype definition, the conditional type checks ifItemextends{ id: StorageIndex }. This relies onItemhaving anidproperty. IfItemdoes not extend{ id: StorageIndex }, the entire array is treated as a shallow object. This could lead to unexpected behaviors if the array items have anidproperty that is not of typeStorageIndex. Consider refining the type checks for more precise type inference.src/core/database/createOrm/createOrm.ts (1)
62-66: Enhance type safety by specifying generics instead ofanyThe use of
flatten<any, any>withanyas generic parameters can compromise type safety. To improve this, specify the generic types explicitly, such asRecord<string, unknown>, to maintain type integrity throughout your code.Apply this change to improve type specificity:
- const flattenedFilters = flatten<any, any>(filters); + const flattenedFilters = flatten<Record<string, unknown>, Record<string, unknown>>(filters);src/core/database/index.ts (1)
2-3: Exports are correctly organizedThe additional export statements for
createOrmandcreateStorageare appropriately added, enhancing the module's accessibility.src/core/database/createStorage/createStorage.ts (1)
6-8: LGTM! Consider adding tests.The factory function is well-implemented with proper type safety and dependency injection. Consider adding unit tests to verify the storage type selection logic.
Would you like me to help generate unit tests for both functions? The tests should cover:
isVariableJsonFile:
- Returns true for strings ending with
.json- Returns false for other strings and non-string types
createStorage:
- Returns FileStorage for
.jsonfiles- Returns MemoryStorage for other data types
src/utils/helpers/interceptors/callRequestInterceptor/callRequestInterceptor.ts (1)
29-30: Verify type safety of the orm property.The
ormproperty is retrieved using optional chaining, but consider:
- Adding type definitions or assertions for the
ormproperty- Adding null checks or providing a default value
Run the following script to verify the type definitions:
✅ Verification successful
Type safety verification completed successfully
The
ormproperty is properly typed asOrm<Database>with comprehensive type definitions that handle both nested and shallow database operations. The optional chaining in the implementation is appropriate for handling undefined context cases.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check type definitions for orm property # Test: Search for orm type definitions in interceptor types rg -A 5 'interface RequestInterceptorParams' src/utils/types/Length of output: 567
Script:
#!/bin/bash # Get complete interface definition and imports rg -B 5 -A 15 'interface RequestInterceptorParams' src/utils/types/interceptors.ts # Check for orm-related type imports rg "import.*orm" src/utils/types/interceptors.tsLength of output: 932
Script:
#!/bin/bash # Search for Orm and Database type definitions ast-grep --pattern 'type Orm' ast-grep --pattern 'interface Orm' ast-grep --pattern 'type Database' ast-grep --pattern 'interface Database' # Check for potential barrel files fd -e ts -x grep -l "export.*\(Orm\|Database\)" {} \;Length of output: 1233
Script:
#!/bin/bash # Check database type definitions in utils/types cat src/utils/types/database.ts # If not found there, check core database module cat src/core/database/index.tsLength of output: 1709
src/core/database/createDatabaseRoutes/createDatabaseRoutes.ts (1)
5-5: LGTM!Clean import of the
createStoragefunction.src/core/database/createDatabaseRoutes/storages/Memory/MemoryStorage.ts (1)
Line range hint
25-36: Consider adding error handling for undefined nested paths.The
writemethod assumes all intermediate objects in the path exist. This could lead to runtime errors if a nested path is undefined.Consider adding validation:
public write(key: StorageIndex | StorageIndex[], value: unknown): void { const keys = Array.isArray(key) ? key : [key]; let writable: any = this.data; let index = 0; while (index < keys.length - 1) { + if (writable[keys[index]] === undefined) { + writable[keys[index]] = {}; + } writable = writable[keys[index]]; index += 1; } writable[keys[index]] = value; }src/core/middlewares/contextMiddleware/contextMiddleware.ts (1)
27-30: Consider merging with logger middleware.As per previous discussions, this middleware could be consolidated with the logger middleware for better maintainability.
src/server/createDatabaseMockServer/createDatabaseMockServer.ts (1)
36-36: LGTM: Consistent database configuration.The database configuration is correctly passed to the context middleware, maintaining consistency with the overall architecture.
src/utils/types/interceptors.ts (1)
17-17: 🛠️ Refactor suggestionConsider making
ormproperty optional for better type safetyThe
ormproperty is currently required in both interceptor params, but it should be optional since the database configuration itself is optional in the server config. This could lead to runtime errors when database is not configured.Let's verify if database config is indeed optional:
Make the orm property optional to match the optional database config:
- orm: Orm<Database>; + orm?: Orm<Database>;Also applies to: 40-40
src/server/createRestMockServer/createRestMockServer.ts (1)
37-37: LGTM! Consistent with database configuration patternThe change to pass
restMockServerConfigtocontextMiddlewarealigns well with the conditional database routes creation pattern used later in the file.src/server/createGraphQLMockServer/createGraphQLMockServer.ts (1)
37-37: LGTM! Maintains consistency across server implementationsThe change mirrors the implementation in
createRestMockServer.ts, ensuring consistent ORM context handling across different server types.src/utils/types/server.ts (1)
4-4: Add tests for ORM integration in interceptorsThe ORM integration in interceptors needs test coverage to ensure:
- ORM context is correctly passed to interceptors when database is configured
- Interceptors work correctly without ORM when database is not configured
- Type safety of the ORM context in different scenarios
Let's check for existing tests:
src/server/createMockServer/createMockServer.ts (1)
38-38: LGTM! Consistent with ORM context integration.The addition of
mockServerConfigparameter tocontextMiddlewarealigns with the broader changes for ORM support across the codebase.src/core/rest/createRestRoutes/createRestRoutes.test.ts (1)
273-273: LGTM! Improved header value assertions.The changes make the tests more resilient by using regex patterns to match content-disposition headers, allowing for variations while still ensuring the correct filename is present.
Also applies to: 495-495, 500-502, 507-507
package.json (1)
53-53: LGTM: Addition of type-checking script.The new
typescript usingtsc --noEmitis a good addition for quick type validation without generating output files.
| @@ -7,6 +7,7 @@ module.exports = { | |||
| { | |||
| files: ['*.ts'], | |||
| rules: { | |||
| '@typescript-eslint/no-namespace': ['error', { allowDeclarations: true }], | |||
There was a problem hiding this comment.
Remove duplicate ESLint rule.
The @typescript-eslint/no-namespace rule is defined twice with the same configuration. Remove the duplicate rule at line 10 to avoid confusion.
- '@typescript-eslint/no-namespace': ['error', { allowDeclarations: true }],🧰 Tools
🪛 Biome (1.9.4)
[error] 10-10: This property value named @typescript-eslint/no-namespace is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named @typescript-eslint/no-namespace
(lint/suspicious/noDuplicateObjectKeys)
| const context: Express['request']['context'] = { orm: {} }; | ||
|
|
||
| if (database) { | ||
| const storage = createStorage(database.data); | ||
| const orm = createOrm(storage); | ||
| context.orm = orm; | ||
| } |
There was a problem hiding this comment.
Fix potential race condition with shared context.
The context object is shared across all requests, which could lead to race conditions in a concurrent environment.
Move context initialization inside the middleware function:
-const context: Express['request']['context'] = { orm: {} };
-
-if (database) {
- const storage = createStorage(database.data);
- const orm = createOrm(storage);
- context.orm = orm;
-}
server.use((request, _response, next) => {
+ const context: Express['request']['context'] = { orm: {} };
+
+ if (database) {
+ const storage = createStorage(database.data);
+ const orm = createOrm(storage);
+ context.orm = orm;
+ }📝 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.
| const context: Express['request']['context'] = { orm: {} }; | |
| if (database) { | |
| const storage = createStorage(database.data); | |
| const orm = createOrm(storage); | |
| context.orm = orm; | |
| } | |
| server.use((request, _response, next) => { | |
| const context: Express['request']['context'] = { orm: {} }; | |
| if (database) { | |
| const storage = createStorage(database.data); | |
| const orm = createOrm(storage); | |
| context.orm = orm; | |
| } |
| public constructor(fileName: string) { | ||
| const filePath = path.resolve(APP_PATH, fileName); | ||
| this.fileWriter = new FileWriter(filePath); | ||
| this.data = JSON.parse(fs.readFileSync(filePath, 'utf-8')); | ||
| } |
There was a problem hiding this comment.
Add error handling for file operations.
The constructor uses synchronous file operations without error handling, which could crash the application if the file is not accessible.
Add proper error handling:
public constructor(fileName: string) {
const filePath = path.resolve(APP_PATH, fileName);
this.fileWriter = new FileWriter(filePath);
- this.data = JSON.parse(fs.readFileSync(filePath, 'utf-8'));
+ try {
+ const fileContent = fs.readFileSync(filePath, 'utf-8');
+ this.data = JSON.parse(fileContent);
+ } catch (error) {
+ if (error instanceof Error) {
+ throw new Error(`Failed to initialize FileStorage: ${error.message}`);
+ }
+ throw error;
+ }
}📝 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.
| public constructor(fileName: string) { | |
| const filePath = path.resolve(APP_PATH, fileName); | |
| this.fileWriter = new FileWriter(filePath); | |
| this.data = JSON.parse(fs.readFileSync(filePath, 'utf-8')); | |
| } | |
| public constructor(fileName: string) { | |
| const filePath = path.resolve(APP_PATH, fileName); | |
| this.fileWriter = new FileWriter(filePath); | |
| try { | |
| const fileContent = fs.readFileSync(filePath, 'utf-8'); | |
| this.data = JSON.parse(fileContent); | |
| } catch (error) { | |
| if (error instanceof Error) { | |
| throw new Error(`Failed to initialize FileStorage: ${error.message}`); | |
| } | |
| throw error; | |
| } | |
| } |
| interface Request { | ||
| context: { | ||
| orm: Orm<Database>; | ||
| }; | ||
| } |
There was a problem hiding this comment.
Make Request.context type definition consistent with optional database config
The current type definition makes context and orm required on all requests, but this doesn't align with the optional nature of database configuration in server configs.
Update the type definition to make both context and orm optional:
interface Request {
- context: {
- orm: Orm<Database>;
- };
+ context?: {
+ orm?: Orm<Database>;
+ };
}📝 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.
| interface Request { | |
| context: { | |
| orm: Orm<Database>; | |
| }; | |
| } | |
| interface Request { | |
| context?: { | |
| orm?: Orm<Database>; | |
| }; | |
| } |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary by CodeRabbit
Based on the comprehensive set of changes, here are the release notes:
New Features
Improvements
Chores