Conversation
# Conflicts: # examples/project.json # package.json # yarn.lock
# Conflicts: # package.json # yarn.lock
…er in `@Field` decorator
# Conflicts: # yarn.lock
…to use it - Added `FindOneArgsType` for standardizing ID parameter handling. - Updated delete, update, and read resolvers to use `FindOneArgsType`. - Introduced new `id-field.decorator` for marking ID fields. - Removed redundant `BadRequestException` in read resolver.
…clude "Args" suffix
# Conflicts: # yarn.lock
…piSchema decorator
- Correct `FindOneArgsType` parameter in `read.resolver.ts` - Enhance `field.decorator.ts` to properly retrieve metadata - Add `IsNumber` and `IsString` validators for specific types - Improve handling of array options and validation
# Conflicts: # examples/project.json # package.json # yarn.lock
- Added `.npmrc` to the `.gitignore` file for excluding configuration from version control.
- Replaced `resolver.provider` with `controller.provider` implementation. - Updated import paths and typings throughout the module. - Added `ParamArgsType` and `QueryParamArgsType` in `param-args.type`. - Refactored `OffsetPaging` to use the updated `Field` decorator. - Removed unused decorators and methods related to resolvers. - Updated dependencies and added `csv-stringify`.
- Bump versions of `@nestjs` packages and other dependencies in `package.json`. - Update `tslib` in `packages/query-rest`. - Upgrade `@mikro-orm` dependencies in `packages/query-mikro-orm`.
…leFieldOptionsToField` - Enhanced `filterableFieldOptionsToField` to accept default empty object for `advancedOptions`.
…y decorator - Corrected type annotations in `ManyToMany` decorator to resolve type issues in `tag.entity`.
|
@coderabbitai review |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run-many --target=e2e --all |
❌ Failed | 2m 3s | View ↗ |
nx run-many --target=lint --all |
❌ Failed | 9s | View ↗ |
nx run-many --target=test --all |
✅ Succeeded | 1s | View ↗ |
nx run-many --target=build --all |
✅ Succeeded | 3s | View ↗ |
nx run workspace:version |
✅ Succeeded | 1s | View ↗ |
☁️ Nx Cloud last updated this comment at 2026-02-12 16:58:01 UTC
@ptc-org/nestjs-query-core
@ptc-org/nestjs-query-graphql
@ptc-org/nestjs-query-mikro-orm
@ptc-org/nestjs-query-mongoose
@ptc-org/nestjs-query-rest
@ptc-org/nestjs-query-sequelize
@ptc-org/nestjs-query-typegoose
@ptc-org/nestjs-query-typeorm
commit: |
📝 WalkthroughWalkthroughAdds a new package Changes
Sequence DiagramsequenceDiagram
participant Client
participant NestApp as NestJS App
participant AuthInt as Authorizer Interceptor
participant HookInt as Hook Interceptor
participant Controller
participant Authorizer
participant Hooks
participant Service
participant DB
Client->>NestApp: HTTP Request (route)
NestApp->>AuthInt: run AuthorizerInterceptor
AuthInt->>Authorizer: resolve/inject authorizer
Authorizer->>AuthInt: authorizer instance
AuthInt->>NestApp: attach authorizer to request
NestApp->>HookInt: run HookInterceptor
HookInt->>Hooks: retrieve hooks for DTO/operation
HookInt->>NestApp: attach hooks to request
NestApp->>Controller: invoke endpoint handler
Controller->>Hooks: execute before-hooks (transform input/query)
Hooks->>Controller: return transformed input/query
Controller->>Service: call service (create/read/update/delete/export) with authorize filter
Service->>Authorizer: ask for authorize filter
Authorizer->>Service: return filter
Service->>DB: execute query
DB->>Service: return results
Service->>Controller: return DTO(s)
Controller->>NestApp: send HTTP response
NestApp->>Client: HTTP Response (JSON/CSV)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Replaced direct `id` reference with `params.getId()` for consistency. - Ensures proper handling of parameter extraction in delete controller.
- Replaced direct `id` reference with `params.getId()` for consistency. - Improves parameter extraction handling in read controller.
…corator`
- Updated `IsEnum` decorator to include `{ each: isArray }` for array handling support.
- Ensures proper validation of enum types for array fields.
…and connections - Adjusted spacing in `IsEnum` decorator to enhance code formatting. - Renamed variable `isArray` to `isReturnTypeArray` for clarity in `controller-methods.decorator`. - Reorganized imports for better structure and readability in offset connection files.
…crud.controller` - Updated `extractDeleteResolverOpts` to ensure `delete` argument defaults to an empty object. - Prevents potential runtime errors when `delete` is undefined.
- Updated `@mikro-orm/better-sqlite` and `@mikro-orm/core` to version 6.6.6 in `package.json`. - Refreshed `yarn.lock` to reflect dependency changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In `@examples/basic-rest/e2e/fixtures.ts`:
- Around line 22-23: SubTaskEntity.todoItemId is declared as string but must
match TodoItemEntity.id (number); change the type of SubTaskEntity.todoItemId to
number and update its `@Column` type accordingly, then update all DTOs
(SubTaskDTO, SubTaskInputDTO, SubTaskUpdateDTO) and any places that construct
sub-task objects (e.g., fixtures, tests) to use number for todoItemId so TypeORM
types align and no runtime coercion occurs.
In `@examples/basic-rest/open-api.json`:
- Around line 393-401: The PUT operation for /tag-dtos/{id} incorrectly uses
CreateTagDTO for the requestBody (CreateTagDTO) which forces full-create fields
on updates; change the schema to a proper partial update DTO (e.g.,
TagUpdateDTO) or add TagUpdateDTO to components that allows optional/partial
fields, then reference that new schema in the PUT requestBody instead of
CreateTagDTO; follow the existing pattern used by SubTaskUpdateDTO and
TodoItemUpdateDTO for naming and optional property semantics so PUT
/tag-dtos/{id} uses TagUpdateDTO.
In `@examples/basic-rest/src/sub-task/dto/subtask-update.dto.ts`:
- Around line 5-9: The `@Field` decorator on the title property is missing
nullable: true which causes OpenAPI to mark title as required; update the title
property’s GraphQL decorator to `@Field`({ nullable: true }) while keeping the
existing `@IsOptional`(), `@IsNotEmpty`(), and `@IsString`() validators so the
optional title?: string in the DTO (e.g., in SubtaskUpdateDto or the title
property) is correctly represented as nullable/optional in the generated schema.
In `@examples/basic-rest/src/sub-task/sub-task.entity.ts`:
- Around line 28-29: The todoItemId property is typed as string but must match
TodoItemEntity.id (which is a number via `@PrimaryGeneratedColumn`); change the
declaration of todoItemId from string to number (todoItemId!: number) and update
the Column metadata if needed (e.g., Column({ type: 'int', nullable: false,
name: 'todo_item_id' })) so the TypeScript type and DB column type match the
referenced primary key.
In `@examples/basic-rest/src/todo-item/dto/todo-item.dto.ts`:
- Around line 13-16: The FilterableField decorator usage on the completed
property is intended to map the schema name to "isCompleted" but currently
schemaName metadata is recorded as "completed"; fix the bug in
filterable-field.decorator.ts so the decorator writes the provided name option
into the metadata key (schemaName) rather than defaulting to the property name.
Update the decorator implementation (the function that defines metadata for
FilterableField) to check for an explicit name option and set
Reflect.defineMetadata('schemaName', options.name, target, propertyKey) (or
equivalent metadata key used) when provided, so the mapping for the completed
property to "isCompleted" works as intended.
In `@packages/query-rest/package.json`:
- Around line 40-48: Remove the unused peerDependencies entries for
"@nestjs/graphql" and "ts-morph" from the peerDependencies block in
package.json; locate the peerDependencies object (the keys "@nestjs/common",
"@nestjs/core", "@nestjs/graphql", "@nestjs/swagger", "class-transformer",
"class-validator", "ts-morph") and delete the "@nestjs/graphql" and "ts-morph"
entries so only actually used peers remain (e.g., "@nestjs/common",
"@nestjs/core", "@nestjs/swagger", "class-transformer", "class-validator").
In `@packages/query-rest/src/controllers/crud.controller.ts`:
- Around line 45-56: The CRUDController interface currently extends
CreateController, ReadControllerFromOpts, and UpdateController but omits
DeleteController and ExportController, causing methods like deleteOne and
exportMany to be present at runtime (via Deletable/Exportable composition) but
missing from the type; update the CRUDController declaration to also extend
DeleteController<DTO, QS> and ExportController<DTO, QS> (keeping the existing
generics R and QS) so the type includes delete and export operations and aligns
with the runtime composition.
In `@packages/query-rest/src/controllers/delete.controller.ts`:
- Around line 25-27: The Deletable mixin ignores opts.disabled so delete
endpoints are always registered; update the Deletable function to mirror
Creatable/Readable by returning BaseClass as never when opts.disabled is truthy
(check opts.disabled at the top of the returned function), i.e. inside the
Deletable closure before defining the subclass check if (opts.disabled) { return
BaseClass as never } so DeleteController/Deletable honors
DeleteResolverOpts.disabled and avoids registering the endpoint.
In `@packages/query-rest/src/controllers/export.controller.ts`:
- Around line 110-115: Remove the unsupported defaultEncoding option from the
stringifyCsv call in export.controller; locate the stringifyCsv invocation that
wraps plainToInstance(ExportDTOClass, items, { excludeExtraneousValues: true })
and delete the defaultEncoding: 'utf8' entry so the options object only contains
valid csv-stringify settings (header, delimiter, quoted_string, etc.).
In `@packages/query-rest/src/controllers/read.controller.ts`:
- Around line 139-141: The method queryMany incorrectly uses the
`@ParamHookArgs`() decorator for the params argument; replace `@ParamHookArgs`()
with `@QueryHookArgs`() so queryMany reads filtering parameters from the query
string (not route params). Update the parameter decorator on queryMany(params:
QOP) to use `@QueryHookArgs`() consistent with exportMany and the
QueryParamArgsType semantics, ensuring NestQuery is used to extract query
parameters.
In `@packages/query-rest/src/decorators/filterable-field.decorator.ts`:
- Around line 87-93: The metadata currently sets schemaName to
propertyName.toString() inside reflector.append, ignoring advancedOptions.name;
update the call in filterable-field.decorator (reflector.append) to set
schemaName to advancedOptions?.name || propertyName.toString() while keeping
propertyName as propertyName.toString(), so downstream consumers like
getFilterableFields will see the provided API/schema name when
advancedOptions.name is set.
- Line 99: The call passes a function that returns returnTypeFunc
(double-wrapping) causing Field to receive a function-returning-function; change
the call to pass returnTypeFunc directly when it's defined and only wrap it in a
zero-arg function when it's not — i.e., call Field with either returnTypeFunc
(if truthy) or (() => returnTypeFunc) appropriately; update the expression
around applyDecorators(Field(...),
filterableFieldOptionsToField(advancedOptions)) so Field receives a function
that returns a type (or undefined) rather than a function that returns a
function; adjust the code where applyDecorators, Field, returnTypeFunc,
filterableFieldOptionsToField, advancedOptions, target, propertyName, descriptor
are used to implement this conditional pass-through.
In `@packages/query-rest/src/decorators/hook-args.decorator.ts`:
- Around line 11-12: HooksTransformer is missing the `@Injectable`() decorator so
`@Inject`(REQUEST) is ignored when Nest instantiates the pipe outside DI; add
`@Injectable()` (imported from `@nestjs/common`) to the HooksTransformer class
declaration so Nest will resolve and inject REQUEST correctly (so this.request
is available for runMutationHooks and runQueryHooks when the pipe is used via
NestParam / NestQuery / NestBody).
In `@packages/query-rest/src/index.ts`:
- Around line 1-7: The barrel file is missing exports for the REST controllers,
preventing consumers from importing core symbols like CRUDController and
ReadController from the package root; update the index barrel (export list in
packages/query-rest/src/index.ts) to export the controllers by adding an export
for './controllers' so that CRUDController, ReadController and any other
controller types are re-exported from the package entrypoint.
In `@packages/query-rest/src/interceptors/hook.interceptor.ts`:
- Around line 28-33: The interceptor's intercept method sets request.hooks but
the downstream HooksTransformer pipe (used by hook-args.decorator.ts) can't get
REQUEST via DI when the pipe is instantiated as a class reference; fix this by
changing the decorator to instantiate the pipe with the hooks from the current
request instead of relying on DI (e.g., in hook-args.decorator.ts, read
context.switchToHttp().getRequest().hooks and pass them into new
HooksTransformer(...)), and keep request.hooks assignment in intercept
(intercept method / HooksTransformer are the symbols to locate).
In `@packages/query-rest/src/types/param-args.type.ts`:
- Around line 19-38: The getId method in the generated FindOneArgs class (inside
ParamArgsType) uses `this.id || this[properties[0]]`, which treats numeric 0 as
falsy; update the fallback to use nullish coalescing so only null/undefined fall
through: inside the getId method of class FindOneArgs (returned by
ParamArgsType), replace the `||` fallback with `??` to return `this.id` when it
is 0 and only use `this[properties[0]]` when `this.id` is null or undefined.
In `@packages/query-rest/src/types/query/filter.type.ts`:
- Around line 46-60: The getter filter builds filters by iterating fields but
uses a truthiness check (if (this[field.schemaName])) which drops valid falsy
values like 0, false, or "". Change the condition in the Filter<T> builder
inside the public get filter() method to explicitly test for presence (e.g.,
Object.prototype.hasOwnProperty.call(this, field.schemaName) or
this[field.schemaName] !== undefined) so falsy-but-valid values are included,
leaving the rest of the logic (populate filter[field.schemaName] and fallback to
super.filter) unchanged.
- Around line 30-85: The circular-reference guard is inserted too late: move the
early cache population so recursive calls can short-circuit. Specifically, after
creating QueryFilter (and before the fields.forEach decoration loop), ensure
TClassCache = TClassCache ?? new Map(); TClassCache.set(typeName, QueryFilter);
internalCache.set(TClass, TClassCache) so that internalCache contains the
partially-constructed QueryFilter for (TClass, typeName) prior to running the
field decoration code; keep the existing reflector.memoize usage and leave the
rest of the filter logic unchanged.
🟡 Minor comments (35)
packages/query-rest/src/types/query/filter.type.ts-59-59 (1)
59-59:⚠️ Potential issue | 🟡 Minor
super.filtermay beundefinedifBaseClasslacks afiltergetter.When no fields match, the getter falls through to
super.filter. If the suppliedBaseClassdoesn't define afilterproperty, this returnsundefined, which will surprise callers expecting aFilter<T>(or at minimum{}). Consider falling back to an empty object.Proposed fix
- return super.filter + return super.filter ?? ({} as Filter<T>)packages/query-rest/README.md-21-22 (1)
21-22:⚠️ Potential issue | 🟡 MinorTypo: "The get started" → "To get started".
📝 Proposed fix
-The get started with the `@ptc-org/nestjs-query-rest` package checkout +To get started with the `@ptc-org/nestjs-query-rest` package checkoutpackages/query-rest/README.md-12-13 (1)
12-13:⚠️ Potential issue | 🟡 MinorFix typos in the description.
- "code first" → "code-first" (hyphenated compound modifier)
- "built on top of of" → "built on top of" (duplicated "of")
📝 Proposed fix
-This package provides a code first implementation of rest CRUD endpoints. It is built on top of -of [nestjs](https://nestjs.com/). +This package provides a code-first implementation of REST CRUD endpoints. It is built on top +of [nestjs](https://nestjs.com/)..github/stale.yml-1-15 (1)
1-15:⚠️ Potential issue | 🟡 MinorMisleading
staleLabelvalue.The label
"auto closed"is applied when an issue is marked as stale, not when it is actually closed. An issue marked stale may receive activity and avoid closure, yet it would still carry the "auto closed" label. Consider renaming it to something like"stale"to accurately reflect the state.packages/query-rest/src/types/update-one-input.type.ts-7-12 (1)
7-12:⚠️ Potential issue | 🟡 MinorStale JSDoc — references "create one" and "graphql schema" instead of "update one" and REST.
This was likely copied from
CreateOneInputType. Update the doc to accurately describe this type.Proposed fix
/** - * The abstract input type for create one operations. + * The abstract input type for update one operations. * - * `@param` fieldName - The name of the field to be exposed in the graphql schema * `@param` UpdateClass - The InputType to be used. */packages/query-rest/src/decorators/authorizer.decorator.ts-12-19 (1)
12-19:⚠️ Potential issue | 🟡 MinorFix
returnfrom avoid-typed function (Biome lint error).The arrow function on line 12 declares a
voidreturn type, but lines 16 and 18 return the result ofreflector.set(...). While this works at runtime, it violates the type contract and is flagged by Biome (noVoidTypeReturn). Separate the call from the return:Proposed fix
return (DTOClass: Class<DTO>): void => { if (!('authorize' in optsOrAuthorizerOrClass)) { // If the user provided a class, provide the custom authorizer and create a default authorizer that injects the custom authorizer customAuthorizerReflector.set(DTOClass, optsOrAuthorizerOrClass) - return reflector.set(DTOClass, createDefaultAuthorizer(DTOClass)) + reflector.set(DTOClass, createDefaultAuthorizer(DTOClass)) + return } - return reflector.set(DTOClass, createDefaultAuthorizer(DTOClass, optsOrAuthorizerOrClass)) + reflector.set(DTOClass, createDefaultAuthorizer(DTOClass, optsOrAuthorizerOrClass)) }packages/query-rest/src/decorators/id-field.decorator.ts-17-31 (1)
17-31:⚠️ Potential issue | 🟡 MinorJSDoc is copy-pasted from
FilterableFieldand doesn't describeIDField.The doc says "Fields that should be filterable through a FilterType" and shows a
FilterableField/GraphQL example, but this decorator marks ID fields. Update the doc to describeIDFieldsemantics and theidOnlyoption.packages/query-rest/src/types/param-args.type.ts-19-19 (1)
19-19:⚠️ Potential issue | 🟡 MinorMissing
eslint-disablefor@typescript-eslint/no-redeclare.Static analysis flags both
ParamArgsType(line 19) andQueryParamArgsType(line 40) as redeclared. Other files using the same dual-declaration pattern (e.g.,mutation-args.type.tsline 11) include// eslint-disable-next-line@typescript-eslint/no-redeclare-- intentional. Add the same suppression here for consistency and to silence the linter.Suggested fix
+// eslint-disable-next-line `@typescript-eslint/no-redeclare` -- intentional export function ParamArgsType(DTOClass: Class<unknown>): Class<ParamArgsType> {+// eslint-disable-next-line `@typescript-eslint/no-redeclare` -- intentional export function QueryParamArgsType(DTOClass: Class<unknown>): Class<QueryParamArgsType> {Also applies to: 40-40
packages/query-rest/src/decorators/field.decorator.ts-32-61 (1)
32-61:⚠️ Potential issue | 🟡 MinorJSDoc references GraphQL imports instead of REST.
The example in the doc comment references
@nestjs/graphqltypes (ObjectType,ID,GraphQLISODateTime,FilterableFieldfrom@ptc-org/nestjs-query-graphql). Since this is the REST package, the documentation should show REST-specific usage with@ptc-org/nestjs-query-restimports and Swagger/REST decorators.packages/query-rest/src/types/param-args.type.ts-20-21 (1)
20-21:⚠️ Potential issue | 🟡 MinorMissing null guard on
getIDFieldsresult before calling.map().
getIDFieldshas return typeMetaValue<IDFieldDescriptor[]>(i.e.,IDFieldDescriptor[] | undefined). If it ever returnsundefined, line 21 will throw at runtime. Add a?? []fallback, consistent with howmutation-args.type.ts(line 13) handles it.Suggested fix
- const dtoIDFields = getIDFields(DTOClass) + const dtoIDFields = getIDFields(DTOClass) ?? []Same issue applies to line 41 in
QueryParamArgsType.packages/query-rest/src/types/create-one-input.type.ts-7-12 (1)
7-12:⚠️ Potential issue | 🟡 MinorJSDoc references a non-existent
fieldNameparam and "graphql schema".This appears to be a copy-paste artifact from the GraphQL package. The
@param fieldNamedoesn't correspond to any parameter, and "graphql schema" doesn't apply to the REST package.Suggested fix
/** - * The abstract input type for create one operations. - * - * `@param` fieldName - The name of the field to be exposed in the graphql schema - * `@param` InputClass - The InputType to be used. + * Factory that produces an input type class for create-one operations. + * + * `@param` InputClass - The InputType to be used. */packages/query-rest/src/providers/hook.provider.ts-14-32 (1)
14-32:⚠️ Potential issue | 🟡 MinorReturn type
Provider[] | undefinedis misleading —reducealways returns an array.The
reduceis initialized with[], so it can never returnundefined. However,getHookProvidersspreads the result directly (...createHookProvider(...)), which would throw at runtime ifundefinedwere ever returned. The type annotation creates a false sense thatundefinedis a valid return value.Suggested fix
-function createHookProvider(hookType: HookTypes, ...DTOClass: Class<unknown>[]): Provider[] | undefined { +function createHookProvider(hookType: HookTypes, ...DTOClass: Class<unknown>[]): Provider[] {packages/query-rest/src/decorators/id-field.decorator.ts-44-44 (1)
44-44:⚠️ Potential issue | 🟡 Minor
delete options?.idOnlymutates the caller's options object.If a consumer passes a reusable/shared options object, this silently strips
idOnlyfrom it. Create a shallow copy before deleting, or destructureidOnlyout before forwarding.Suggested fix
+ const { idOnly: _idOnly, ...fieldOptions } = options ?? {} + if (descriptor) { - return Field(options)(target, propertyName, descriptor) + return Field(fieldOptions)(target, propertyName, descriptor) } - return Field(options)(target, propertyName) + return Field(fieldOptions)(target, propertyName)And remove line 44 (
delete options?.idOnly).packages/query-rest/src/decorators/filterable-field.decorator.ts-9-14 (1)
9-14:⚠️ Potential issue | 🟡 Minor
allowedComparisonstype is a fixed tuple['=', '!=']instead of a flexible array.This restricts the value to exactly the two-element tuple
['=', '!=']. It should likely be an array of allowed comparison operators, e.g.('=' | '!=' | '>' | '<' | '>=' | '<=' | 'like' | ...)[]or at least astring[]to be useful.packages/query-rest/src/decorators/filterable-field.decorator.ts-31-59 (1)
31-59:⚠️ Potential issue | 🟡 MinorJSDoc example references GraphQL package, not the REST package.
The doc comment imports from
@ptc-org/nestjs-query-graphqland uses@nestjs/graphqldecorators (ObjectType,ID,GraphQLISODateTime). Since this is the REST package, the example should reference@ptc-org/nestjs-query-restand use REST-appropriate decorators.packages/query-rest/src/controllers/update.controller.ts-40-43 (1)
40-43:⚠️ Potential issue | 🟡 MinorJSDoc says "graphql endpoints" — should be "REST endpoints".
Copy-paste artifact from the GraphQL package.
📝 Proposed fix
/** * `@internal` - * Mixin to add `update` graphql endpoints. + * Mixin to add `update` REST endpoints. */packages/query-rest/src/decorators/resolver-method.decorator.ts-68-75 (1)
68-75:⚠️ Potential issue | 🟡 MinorJSDoc says "disabled" but the function checks
enabled.The doc comment states "Returns true if any of the ResolverRelationMethodOpts are disabled" but the function
isEnabledcheckso.enabled.📝 Proposed fix
/** * `@internal` - * Returns true if any of the [[ResolverRelationMethodOpts]] are disabled. + * Returns true if any of the [[ResolverRelationMethodOpts]] are enabled. * `@param` opts - The array of [[ResolverRelationMethodOpts]] to check. */packages/query-rest/src/controllers/create.controller.ts-45-48 (1)
45-48:⚠️ Potential issue | 🟡 MinorJSDoc comments reference GraphQL — should reference REST.
Line 47 says "Mixin to add
creategraphql endpoints" and Lines 110-113 mention "graphql query endpoints", "createOneTodoItem and createManyTodoItems". These should reference REST endpoints, and thecreateManymention is inaccurate since onlycreateOneis implemented.Also applies to: 107-121
packages/query-rest/src/decorators/hook-args.decorator.ts-48-61 (1)
48-61:⚠️ Potential issue | 🟡 Minor
runQueryHooksdoesn't guard against missingbuildQuerybefore calling it.At Line 50,
data.buildQuery()is called unconditionally. If the transformed value doesn't implementBuildableQueryType(e.g.,transformValuereturns a plain object without the method), this will throw at runtime. Consider adding a guard or an informative error message.packages/query-rest/src/controllers/read.controller.ts-58-61 (1)
58-61:⚠️ Potential issue | 🟡 MinorJSDoc says "graphql endpoints" — should say "REST endpoints".
The comment at Line 60 reads
Mixin to add 'read' graphql endpoints, but this is the REST package.Proposed fix
/** * `@internal` - * Mixin to add `read` graphql endpoints. + * Mixin to add `read` REST endpoints. */packages/query-rest/src/controllers/delete.controller.ts-21-24 (1)
21-24:⚠️ Potential issue | 🟡 MinorJSDoc says "graphql endpoints" — should say "REST endpoints".
Same pattern as the other controller files.
Proposed fix
/** * `@internal` - * Mixin to add `delete` graphql endpoints. + * Mixin to add `delete` REST endpoints. */packages/query-rest/src/controllers/controller.interface.ts-8-25 (1)
8-25:⚠️ Potential issue | 🟡 MinorJSDoc references "graphql" throughout — should reference REST.
Multiple comments refer to "graphql query or mutation" and "graphql endpoints" (Lines 9, 11, 18, 22). This is the REST package — these should say "REST endpoint" or simply "endpoint".
Proposed fix
export type NamedEndpoint = { - /** Specify to override the name of the graphql query or mutation * */ + /** Specify to override the endpoint path * */ path?: string - /** Specify a description for the graphql query or mutation* */ + /** Specify a description for the endpoint * */ description?: string operationOptions?: ApiOperationOptions } export interface ControllerOpts extends QueryResolverMethodOpts, DTONamesOpts { /** - * Options for single record graphql endpoints + * Options for single record endpoints */ one?: QueryResolverMethodOpts & NamedEndpoint /** - * Options for multiple record graphql endpoints + * Options for multiple record endpoints */ many?: QueryResolverMethodOpts & NamedEndpoint }packages/query-rest/src/controllers/read.controller.ts-110-122 (1)
110-122:⚠️ Potential issue | 🟡 MinorReturn type mismatch between interface and implementation.
The
ReadControllerinterface (Line 55) declaresfindByIdas returningPromise<DTO | undefined>, but the implementation at Line 117 returnsPromise<DTO>. TypicallyQueryService.getByIdthrowsNotFoundExceptionwhen the record is missing rather than returningundefined, so the implementation is correct — but the interface should match.Proposed fix in the interface
- findById(params: ParamArgsType, authorizeFilter?: Filter<DTO>): Promise<DTO | undefined> + findById(params: ParamArgsType, authorizeFilter?: Filter<DTO>): Promise<DTO>packages/query-rest/src/decorators/hook.decorator.ts-38-46 (1)
38-46:⚠️ Potential issue | 🟡 MinorRemove explicit
returnfrom void-typed function.The arrow function's signature implies
void, butreturn Reflect.defineMetadata(...)explicitly returns a value. WhileReflect.defineMetadataitself returnsvoid, the explicitreturnis unnecessary and triggers the Biome lint error (noVoidTypeReturn).Proposed fix
return (...data: HookDecoratorArg<H>[]) => // eslint-disable-next-line `@typescript-eslint/ban-types` (target: Function): void => { - return Reflect.defineMetadata( + Reflect.defineMetadata( key, data.map((d) => getHook(d)), target ) }packages/query-rest/src/decorators/param-args.decorator.ts-7-19 (1)
7-19:⚠️ Potential issue | 🟡 MinorIncorrect return type and duplicated
transformValuelogic.Two issues:
Misleading return type (Line 8):
transformdeclaresPromise<MutationArgsType<T> | Query<T>>but actually returnsT(the plain transformed value). TheQueryimport (Line 2) andMutationArgsTypeimport (Line 5) appear unused in the implementation—they're only referenced in this inaccurate type signature. This likely was copy-pasted fromHooksTransformer.Duplicated code:
transformValueis identical to the one inhook-args.decorator.ts. Consider extracting it into a shared utility.,
Proposed fix for the return type
-import { Class, Query } from '@ptc-org/nestjs-query-core' +import { Class } from '@ptc-org/nestjs-query-core' import { plainToInstance } from 'class-transformer' -import { MutationArgsType, ParamArgsType } from '../types' +import { ParamArgsType } from '../types' class ParamTransformer<T> implements PipeTransform { - public async transform(value: T, metadata: ArgumentMetadata): Promise<MutationArgsType<T> | Query<T>> { + public async transform(value: T, metadata: ArgumentMetadata): Promise<T> { return this.transformValue(value, metadata.metatype) }packages/query-rest/src/decorators/controller-methods.decorator.ts-37-42 (1)
37-42:⚠️ Potential issue | 🟡 MinorType guard doesn't distinguish
nullfrom object options.
typeof null === 'object'in JavaScript. If a caller passesnullas the first argument (e.g.,Get(null, { path: '/foo' })), line 38 would assignoptions = null, then line 48 would throw when accessingnull.path. While the TypeScript overloads should prevent this at compile-time, a defensive check would be safer:🛡️ Suggested defensive fix
- if (typeof returnTypeFuncOrOptions === 'object') { + if (returnTypeFuncOrOptions !== null && typeof returnTypeFuncOrOptions === 'object') {examples/basic-rest/e2e/tag.endpoint.spec.ts-34-34 (1)
34-34:⚠️ Potential issue | 🟡 MinorConsolidate duplicate
afterAllhooks.There are two
afterAllhooks at the samedescribelevel (Lines 34 and 56). Only the last one is guaranteed to run in some test runners, but regardless this should be a single hook. The static analysis tool (Biome) also flags this.Proposed fix
Remove line 34 and merge the refresh call into the existing
afterAll:- afterAll(() => refresh(app.get(Connection))) - const tags = [afterAll(async () => { + await refresh(app.get(Connection)) await app.close() })Also applies to: 56-58
examples/basic-rest/e2e/tag.endpoint.spec.ts-12-12 (1)
12-12:⚠️ Potential issue | 🟡 MinorTest description says "TagResolver" — should be "TagController" or similar.
This is a REST endpoint test, not a GraphQL resolver test. The describe string appears to be a copy-paste artifact.
Proposed fix
-describe('TagResolver (basic rest - e2e)', () => { +describe('TagEndpoint (basic rest - e2e)', () => {examples/basic-rest/src/sub-task/dto/sub-task.dto.ts-22-23 (1)
22-23:⚠️ Potential issue | 🟡 Minor
todoItemIdshould benumberto match the referenced entity's primary key type.
TodoItemEntity.idisnumber(via@PrimaryGeneratedColumn()), so this field should also benumberfor type consistency. This was also noted in the entity definition.Proposed fix
`@FilterableField`() - todoItemId!: string + todoItemId!: numberpackages/query-rest/src/types/query/query-args/none-paging-query-args.type.ts-2-2 (1)
2-2:⚠️ Potential issue | 🟡 MinorInconsistent import pattern — align with relative imports used elsewhere in the package.
While
RestQueryis properly exported from the@ptc-org/nestjs-query-restpackage entry point, the codebase shows mixed import patterns:buildable-query.type.tsuses a relative import (import { RestQuery } from '../rest-query.type'), while multiplequery-argsfiles use self-referential imports from@ptc-org/nestjs-query-rest. For consistency and to avoid potential issues during development when the package hasn't been compiled, use relative imports throughout.Proposed fix
-import { RestQuery } from '@ptc-org/nestjs-query-rest' +import { RestQuery } from '../../rest-query.type'examples/basic-rest/open-api.json-150-161 (1)
150-161:⚠️ Potential issue | 🟡 MinorPOST operations return
200instead of201.REST convention is to return
201 Createdfor successful resource creation. All three POST endpoints (/sub-task-dtos,/todo-item-dtos,/tag-dtos) return200. Consider updating the controller to produce201status codes.Also applies to: 310-322, 470-482
packages/query-rest/src/connection/offset/offset-connection.type.ts-44-47 (1)
44-47:⚠️ Potential issue | 🟡 Minor
totalCountis optional but@Fieldlacksnullable: true.The property is declared as
totalCount?: numbermeaning it can beundefined, but the@Fielddecorator doesn't indicate nullability. This may cause the OpenAPI spec to marktotalCountas required (which the generatedopen-api.jsonindeed does — see therequiredarrays in connection schemas). If a count query isn't provided, this will beundefinedand may confuse API consumers expecting a value.Proposed fix
`@Field`({ - description: 'Total amount of records.' + description: 'Total amount of records.', + nullable: true }) public totalCount?: numberpackages/query-rest/src/types/query/query-args/interfaces.ts-8-9 (1)
8-9:⚠️ Potential issue | 🟡 MinorMinor typo in JSDoc comment.
"an filter" → "a filter".
📝 Suggested fix
- * to build an filter + * to build a filterpackages/query-rest/src/controllers/export.controller.ts-23-26 (1)
23-26:⚠️ Potential issue | 🟡 MinorIncorrect JSDoc: "finding one record" should reference export.
This comment was copied from the read controller. The
ExportDTOClassis used for exporting many records, not finding one.📝 Suggested fix
/** - * DTO to return with finding one record + * DTO class to use when exporting records */ ExportDTOClass?: Class<DTO>packages/query-rest/src/controllers/crud.controller.ts-107-123 (1)
107-123:⚠️ Potential issue | 🟡 MinorJSDoc references GraphQL instead of REST.
The docblock still mentions
@ptc-org/nestjs-query-graphql,@Resolver(), andCRUDResolver— all from the GraphQL package. This should be updated to reflect the REST package.📝 Suggested fix
/** - * Factory to create a resolver that includes all CRUD methods from [[CreateResolver]], [[ReadResolver]], - * [[UpdateResolver]], and [[DeleteResolver]]. + * Factory to create a controller that includes all CRUD methods from [[CreateController]], [[ReadController]], + * [[UpdateController]], [[DeleteController]], and [[ExportController]]. * * ```ts - * import { CRUDResolver } from '@ptc-org/nestjs-query-graphql'; - * import { Resolver } from '@nestjs/graphql'; + * import { CRUDController } from '@ptc-org/nestjs-query-rest'; + * import { Controller } from '@nestjs/common'; * import { TodoItemDTO } from './dto/todo-item.dto'; * import { TodoItemService } from './todo-item.service'; * - * `@Resolver`() - * export class TodoItemResolver extends CRUDResolver(TodoItemDTO) { + * `@Controller`('todo-items') + * export class TodoItemController extends CRUDController(TodoItemDTO) { * constructor(readonly service: TodoItemService) { * super(service); * } * } * ``` - * `@param` DTOClass - The DTO Class that the resolver is for. All methods will use types derived from this class. - * `@param` opts - Options to customize the resolver. + * `@param` DTOClass - The DTO Class that the controller is for. All methods will use types derived from this class. + * `@param` opts - Options to customize the controller. */
| const blockedTag = await tagsRepo.save({ name: 'Blocked' }) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "todoItemId" examples/basic-rest/ --type=ts -B2 -A2Repository: TriPSs/nestjs-query
Length of output: 1378
🏁 Script executed:
rg -n "class TodoItemEntity" examples/basic-rest/ --type=ts -A 20Repository: TriPSs/nestjs-query
Length of output: 1625
Fix the type mismatch between SubTaskEntity.todoItemId and TodoItemEntity.id.
SubTaskEntity.todoItemId is typed as string (sub-task.entity.ts line 29), but TodoItemEntity.id is typed as number (@PrimaryGeneratedColumn() at todo-item.entity.ts line 18). The DTOs (SubTaskDTO, SubTaskInputDTO, SubTaskUpdateDTO) also declare todoItemId as string. This type mismatch will cause TypeORM to coerce types at runtime, leading to potential runtime errors or silent failures. Align SubTaskEntity.todoItemId to number to match the primary key type.
🤖 Prompt for AI Agents
In `@examples/basic-rest/e2e/fixtures.ts` around lines 22 - 23,
SubTaskEntity.todoItemId is declared as string but must match TodoItemEntity.id
(number); change the type of SubTaskEntity.todoItemId to number and update its
`@Column` type accordingly, then update all DTOs (SubTaskDTO, SubTaskInputDTO,
SubTaskUpdateDTO) and any places that construct sub-task objects (e.g.,
fixtures, tests) to use number for todoItemId so TypeORM types align and no
runtime coercion occurs.
| "requestBody": { | ||
| "required": true, | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/CreateTagDTO" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
PUT /tag-dtos/{id} references CreateTagDTO instead of an update DTO.
The update operation for tags uses CreateTagDTO as its request body schema (Line 398), whereas SubTask and TodoItem have dedicated update DTOs (SubTaskUpdateDTO, TodoItemUpdateDTO). This means an update requires the same fields as creation, which is typically undesirable (updates should allow partial payloads). Either introduce a TagUpdateDTO or confirm this is intentional.
🤖 Prompt for AI Agents
In `@examples/basic-rest/open-api.json` around lines 393 - 401, The PUT operation
for /tag-dtos/{id} incorrectly uses CreateTagDTO for the requestBody
(CreateTagDTO) which forces full-create fields on updates; change the schema to
a proper partial update DTO (e.g., TagUpdateDTO) or add TagUpdateDTO to
components that allows optional/partial fields, then reference that new schema
in the PUT requestBody instead of CreateTagDTO; follow the existing pattern used
by SubTaskUpdateDTO and TodoItemUpdateDTO for naming and optional property
semantics so PUT /tag-dtos/{id} uses TagUpdateDTO.
| @Field() | ||
| @IsOptional() | ||
| @IsNotEmpty() | ||
| @IsString() | ||
| title?: string |
There was a problem hiding this comment.
title field missing nullable: true — OpenAPI will incorrectly mark it as required.
All other fields in this update DTO use @Field({ nullable: true }), but title uses @Field() without options. Since the Field decorator defaults to required: true when nullable is not set, the generated OpenAPI spec will mark title as a required field, contradicting the ? optional type annotation and the @IsOptional() decorator.
Proposed fix
- `@Field`()
+ `@Field`({ nullable: true })
`@IsOptional`()
`@IsNotEmpty`()
`@IsString`()
title?: string🤖 Prompt for AI Agents
In `@examples/basic-rest/src/sub-task/dto/subtask-update.dto.ts` around lines 5 -
9, The `@Field` decorator on the title property is missing nullable: true which
causes OpenAPI to mark title as required; update the title property’s GraphQL
decorator to `@Field`({ nullable: true }) while keeping the existing
`@IsOptional`(), `@IsNotEmpty`(), and `@IsString`() validators so the optional title?:
string in the DTO (e.g., in SubtaskUpdateDto or the title property) is correctly
represented as nullable/optional in the generated schema.
| @Column({ nullable: false, name: 'todo_item_id' }) | ||
| todoItemId!: string |
There was a problem hiding this comment.
Type mismatch: todoItemId should be number, not string.
TodoItemEntity.id is declared as number (via @PrimaryGeneratedColumn() in todo-item.entity.ts), so the foreign key column todoItemId must match that type. Using string here will cause type-safety issues and potentially runtime query mismatches.
🐛 Proposed fix
`@Column`({ nullable: false, name: 'todo_item_id' })
- todoItemId!: string
+ todoItemId!: number🤖 Prompt for AI Agents
In `@examples/basic-rest/src/sub-task/sub-task.entity.ts` around lines 28 - 29,
The todoItemId property is typed as string but must match TodoItemEntity.id
(which is a number via `@PrimaryGeneratedColumn`); change the declaration of
todoItemId from string to number (todoItemId!: number) and update the Column
metadata if needed (e.g., Column({ type: 'int', nullable: false, name:
'todo_item_id' })) so the TypeScript type and DB column type match the
referenced primary key.
| @FilterableField({ | ||
| name: 'isCompleted' | ||
| }) | ||
| completed!: boolean |
There was a problem hiding this comment.
Note: The name: 'isCompleted' mapping depends on the schemaName fix in filterable-field.decorator.ts.
The schemaName metadata for completed will be recorded as "completed" instead of "isCompleted" due to the bug flagged in the decorator. Once that is fixed, this mapping should work as intended.
🤖 Prompt for AI Agents
In `@examples/basic-rest/src/todo-item/dto/todo-item.dto.ts` around lines 13 - 16,
The FilterableField decorator usage on the completed property is intended to map
the schema name to "isCompleted" but currently schemaName metadata is recorded
as "completed"; fix the bug in filterable-field.decorator.ts so the decorator
writes the provided name option into the metadata key (schemaName) rather than
defaulting to the property name. Update the decorator implementation (the
function that defines metadata for FilterableField) to check for an explicit
name option and set Reflect.defineMetadata('schemaName', options.name, target,
propertyKey) (or equivalent metadata key used) when provided, so the mapping for
the completed property to "isCompleted" works as intended.
| export { AuthorizationContext, Authorizer, AuthorizerOptions, CustomAuthorizer, OperationGroup } from './auth' | ||
| export * from './connection' | ||
| export * from './decorators' | ||
| export * from './hooks' | ||
| export * from './interceptors' | ||
| export { NestjsQueryRestModule } from './module' | ||
| export * from './types' |
There was a problem hiding this comment.
Missing ./controllers export — likely an oversight.
The package barrel exports ./connection, ./decorators, ./hooks, ./interceptors, and ./types, but omits ./controllers. Since this package's core value is providing auto-generated REST controllers (CRUDController, ReadController, etc.), consumers can't access them from the package root.
Proposed fix
export { AuthorizationContext, Authorizer, AuthorizerOptions, CustomAuthorizer, OperationGroup } from './auth'
export * from './connection'
+export * from './controllers'
export * from './decorators'
export * from './hooks'
export * from './interceptors'
export { NestjsQueryRestModule } from './module'
export * from './types'📝 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.
| export { AuthorizationContext, Authorizer, AuthorizerOptions, CustomAuthorizer, OperationGroup } from './auth' | |
| export * from './connection' | |
| export * from './decorators' | |
| export * from './hooks' | |
| export * from './interceptors' | |
| export { NestjsQueryRestModule } from './module' | |
| export * from './types' | |
| export { AuthorizationContext, Authorizer, AuthorizerOptions, CustomAuthorizer, OperationGroup } from './auth' | |
| export * from './connection' | |
| export * from './controllers' | |
| export * from './decorators' | |
| export * from './hooks' | |
| export * from './interceptors' | |
| export { NestjsQueryRestModule } from './module' | |
| export * from './types' |
🤖 Prompt for AI Agents
In `@packages/query-rest/src/index.ts` around lines 1 - 7, The barrel file is
missing exports for the REST controllers, preventing consumers from importing
core symbols like CRUDController and ReadController from the package root;
update the index barrel (export list in packages/query-rest/src/index.ts) to
export the controllers by adding an export for './controllers' so that
CRUDController, ReadController and any other controller types are re-exported
from the package entrypoint.
| public intercept(context: ExecutionContext, next: CallHandler) { | ||
| const request = context.switchToHttp().getRequest() | ||
| request.hooks = this.hooks | ||
|
|
||
| return next.handle() | ||
| } |
There was a problem hiding this comment.
Hooks are attached to the request, but the downstream pipe cannot access them.
This interceptor correctly sets request.hooks = this.hooks (Line 30), but the HooksTransformer pipe in hook-args.decorator.ts tries to read them via @Inject(REQUEST), which won't resolve in a pipe instantiated as a class reference. See the related comment on hook-args.decorator.ts — the interceptor-to-pipe handoff for hook context is broken.
🤖 Prompt for AI Agents
In `@packages/query-rest/src/interceptors/hook.interceptor.ts` around lines 28 -
33, The interceptor's intercept method sets request.hooks but the downstream
HooksTransformer pipe (used by hook-args.decorator.ts) can't get REQUEST via DI
when the pipe is instantiated as a class reference; fix this by changing the
decorator to instantiate the pipe with the hooks from the current request
instead of relying on DI (e.g., in hook-args.decorator.ts, read
context.switchToHttp().getRequest().hooks and pass them into new
HooksTransformer(...)), and keep request.hooks assignment in intercept
(intercept method / HooksTransformer are the symbols to locate).
| export function ParamArgsType(DTOClass: Class<unknown>): Class<ParamArgsType> { | ||
| const dtoIDFields = getIDFields(DTOClass) | ||
| const properties = dtoIDFields.map(({ propertyName }) => propertyName) | ||
|
|
||
| if (properties.length === 0) { | ||
| // @ts-expect-error we want to return empty class | ||
| return class FindOneArgsType {} | ||
| } | ||
|
|
||
| class FindOneArgs extends PickType(DTOClass, properties as never) implements ParamArgsType { | ||
| id: string | number | ||
|
|
||
| getId() { | ||
| // Return the default "id", unless it's not set then return the first ID field | ||
| return this.id || this[properties[0]] | ||
| } | ||
| } | ||
|
|
||
| return FindOneArgs | ||
| } |
There was a problem hiding this comment.
this.id || this[properties[0]] on Line 33 is falsy for id = 0.
Using || means a numeric ID of 0 would be treated as falsy, falling through to this[properties[0]]. Since id is typed string | number, use nullish coalescing (??) to only fall through on null/undefined.
Suggested fix
getId() {
// Return the default "id", unless it's not set then return the first ID field
- return this.id || this[properties[0]]
+ return this.id ?? this[properties[0]]
}📝 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.
| export function ParamArgsType(DTOClass: Class<unknown>): Class<ParamArgsType> { | |
| const dtoIDFields = getIDFields(DTOClass) | |
| const properties = dtoIDFields.map(({ propertyName }) => propertyName) | |
| if (properties.length === 0) { | |
| // @ts-expect-error we want to return empty class | |
| return class FindOneArgsType {} | |
| } | |
| class FindOneArgs extends PickType(DTOClass, properties as never) implements ParamArgsType { | |
| id: string | number | |
| getId() { | |
| // Return the default "id", unless it's not set then return the first ID field | |
| return this.id || this[properties[0]] | |
| } | |
| } | |
| return FindOneArgs | |
| } | |
| export function ParamArgsType(DTOClass: Class<unknown>): Class<ParamArgsType> { | |
| const dtoIDFields = getIDFields(DTOClass) | |
| const properties = dtoIDFields.map(({ propertyName }) => propertyName) | |
| if (properties.length === 0) { | |
| // `@ts-expect-error` we want to return empty class | |
| return class FindOneArgsType {} | |
| } | |
| class FindOneArgs extends PickType(DTOClass, properties as never) implements ParamArgsType { | |
| id: string | number | |
| getId() { | |
| // Return the default "id", unless it's not set then return the first ID field | |
| return this.id ?? this[properties[0]] | |
| } | |
| } | |
| return FindOneArgs | |
| } |
🧰 Tools
🪛 ESLint
[error] 19-19: 'ParamArgsType' is already defined.
(@typescript-eslint/no-redeclare)
🤖 Prompt for AI Agents
In `@packages/query-rest/src/types/param-args.type.ts` around lines 19 - 38, The
getId method in the generated FindOneArgs class (inside ParamArgsType) uses
`this.id || this[properties[0]]`, which treats numeric 0 as falsy; update the
fallback to use nullish coalescing so only null/undefined fall through: inside
the getId method of class FindOneArgs (returned by ParamArgsType), replace the
`||` fallback with `??` to return `this.id` when it is 0 and only use
`this[properties[0]]` when `this.id` is null or undefined.
| return reflector.memoize(TClass, typeName, () => { | ||
| const fields = getFilterableFields(TClass) | ||
|
|
||
| // if the filter is already in the cache, exist early and return it | ||
| // otherwise add it to the cache early so we don't get into an infinite loop | ||
| let TClassCache = internalCache.get(TClass) | ||
|
|
||
| if (TClassCache && TClassCache.has(typeName)) { | ||
| return TClassCache.get(typeName) as FilterConstructor<T> | ||
| } | ||
|
|
||
| const hasRequiredFilters = fields.some((f) => f.advancedOptions?.filterRequired === true) | ||
|
|
||
| class QueryFilter extends BaseClass { | ||
| static hasRequiredFilters: boolean = hasRequiredFilters | ||
|
|
||
| public get filter(): Filter<T> { | ||
| const filters = fields.reduce((filter, field) => { | ||
| if (this[field.schemaName]) { | ||
| filter[field.schemaName] = { eq: this[field.schemaName] } | ||
| } | ||
|
|
||
| return filter | ||
| }, {} as Filter<T>) | ||
|
|
||
| if (Object.keys(filters).length > 0) { | ||
| return filters | ||
| } | ||
|
|
||
| return super.filter | ||
| } | ||
| } | ||
|
|
||
| fields.forEach(({ schemaName, advancedOptions }) => { | ||
| applyDecorators( | ||
| Field( | ||
| filterableFieldOptionsToField({ | ||
| ...advancedOptions, | ||
| nullable: | ||
| typeof advancedOptions.filterRequired !== 'undefined' ? !advancedOptions.filterRequired : advancedOptions.nullable, | ||
| required: Boolean( | ||
| typeof advancedOptions.filterRequired !== 'undefined' ? advancedOptions.filterRequired : advancedOptions.required | ||
| ) | ||
| } as FilterableFieldOptions) | ||
| ), | ||
| ...(advancedOptions.filterDecorators || []) | ||
| )(QueryFilter.prototype, schemaName) | ||
| }) | ||
|
|
||
| TClassCache = TClassCache ?? new Map() | ||
|
|
||
| TClassCache.set(typeName, QueryFilter) | ||
| internalCache.set(TClass, TClassCache) | ||
|
|
||
| return QueryFilter as never as FilterConstructor<T> | ||
| }) |
There was a problem hiding this comment.
Circular-reference guard is populated too late — infinite recursion is still possible.
The internalCache entry is written at lines 79-82, after the field-decoration loop (lines 63-77). If decorating a field triggers a recursive getOrCreateFilterType call for the same TClass/typeName (e.g. User → Post → User), neither reflector.memoize (value not yet stored) nor internalCache (not yet populated) will short-circuit the call, causing unbounded recursion.
Move the cache insertion to before the field-processing loop so re-entrant calls can exit early with the partially-constructed class:
Proposed fix
const hasRequiredFilters = fields.some((f) => f.advancedOptions?.filterRequired === true)
class QueryFilter extends BaseClass {
static hasRequiredFilters: boolean = hasRequiredFilters
public get filter(): Filter<T> {
const filters = fields.reduce((filter, field) => {
if (this[field.schemaName]) {
filter[field.schemaName] = { eq: this[field.schemaName] }
}
return filter
}, {} as Filter<T>)
if (Object.keys(filters).length > 0) {
return filters
}
return super.filter
}
}
+ // Register in cache *before* decorating fields so that
+ // circular references can resolve to this (incomplete) class.
+ TClassCache = TClassCache ?? new Map()
+ TClassCache.set(typeName, QueryFilter)
+ internalCache.set(TClass, TClassCache)
+
fields.forEach(({ schemaName, advancedOptions }) => {
applyDecorators(
Field(
filterableFieldOptionsToField({
...advancedOptions,
nullable:
typeof advancedOptions.filterRequired !== 'undefined' ? !advancedOptions.filterRequired : advancedOptions.nullable,
required: Boolean(
typeof advancedOptions.filterRequired !== 'undefined' ? advancedOptions.filterRequired : advancedOptions.required
)
} as FilterableFieldOptions)
),
...(advancedOptions.filterDecorators || [])
)(QueryFilter.prototype, schemaName)
})
- TClassCache = TClassCache ?? new Map()
-
- TClassCache.set(typeName, QueryFilter)
- internalCache.set(TClass, TClassCache)
-
return QueryFilter as never as FilterConstructor<T>🤖 Prompt for AI Agents
In `@packages/query-rest/src/types/query/filter.type.ts` around lines 30 - 85, The
circular-reference guard is inserted too late: move the early cache population
so recursive calls can short-circuit. Specifically, after creating QueryFilter
(and before the fields.forEach decoration loop), ensure TClassCache =
TClassCache ?? new Map(); TClassCache.set(typeName, QueryFilter);
internalCache.set(TClass, TClassCache) so that internalCache contains the
partially-constructed QueryFilter for (TClass, typeName) prior to running the
field decoration code; keep the existing reflector.memoize usage and leave the
rest of the filter logic unchanged.
| public get filter(): Filter<T> { | ||
| const filters = fields.reduce((filter, field) => { | ||
| if (this[field.schemaName]) { | ||
| filter[field.schemaName] = { eq: this[field.schemaName] } | ||
| } | ||
|
|
||
| return filter | ||
| }, {} as Filter<T>) | ||
|
|
||
| if (Object.keys(filters).length > 0) { | ||
| return filters | ||
| } | ||
|
|
||
| return super.filter | ||
| } |
There was a problem hiding this comment.
Truthiness check drops valid falsy filter values (0, false, "").
if (this[field.schemaName]) on line 48 silently ignores legitimate filter values such as 0, false, and empty string. For a filter builder this is a correctness problem — e.g. filtering records where isActive eq false or count eq 0 would never work.
Proposed fix — check for explicit presence instead
public get filter(): Filter<T> {
const filters = fields.reduce((filter, field) => {
- if (this[field.schemaName]) {
+ if (this[field.schemaName] !== undefined && this[field.schemaName] !== null) {
filter[field.schemaName] = { eq: this[field.schemaName] }
}
return filter
}, {} as Filter<T>)🤖 Prompt for AI Agents
In `@packages/query-rest/src/types/query/filter.type.ts` around lines 46 - 60, The
getter filter builds filters by iterating fields but uses a truthiness check (if
(this[field.schemaName])) which drops valid falsy values like 0, false, or "".
Change the condition in the Filter<T> builder inside the public get filter()
method to explicitly test for presence (e.g.,
Object.prototype.hasOwnProperty.call(this, field.schemaName) or
this[field.schemaName] !== undefined) so falsy-but-valid values are included,
leaving the rest of the logic (populate filter[field.schemaName] and fallback to
super.filter) unchanged.
…OffsetPaging` - Applied `@IsInt()` decorator to `limit` and `offset` fields for integer validation. - Ensures stricter input validation for offset paging parameters.
- Moved `IsNotEmpty` and `IsOptional` decorators for cleaner, logical grouping in `field.decorator`. - Prevents duplicate logic by restructuring `skipRequired` condition block.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/query-rest/src/decorators/field.decorator.ts`:
- Around line 32-61: The JSDoc example is copied from the GraphQL package and
should be replaced with a REST-appropriate example: update the docblock for the
FilterableField decorator (referenced as FilterableField and TodoItemDTO in this
file) to remove GraphQL imports and types (`@ptc-org/nestjs-query-graphql`,
`@nestjs/graphql`, `ObjectType`, `ID`, `GraphQLISODateTime`) and instead show a
simple REST DTO usage (e.g., import of FilterableField, a plain class
TodoItemDTO with typed properties decorated by FilterableField and standard
class-transformer/validator or plain TypeScript types) so the example
demonstrates how to mark DTO fields as filterable for the REST controller.
- Around line 117-119: The forceArray Transform currently wraps null/undefined
into [null]/[undefined]; update the Transform pushed into decorators (the one
using Transform in field.decorator.ts tied to advancedOptions?.forceArray) so it
first checks for null or undefined and returns the original value in that case,
otherwise returns the existing array or wraps the value in an array; locate the
Transform call added to the decorators array and change its mapping logic to
skip wrapping when value == null.
🧹 Nitpick comments (2)
packages/query-rest/src/types/query/offset-paging.type.ts (1)
18-26: Consider adding a default value foroffset.
limitdefaults to25, butoffsethas no default. If a client omits both,offsetwill beundefinedrather than0. Depending on how downstream query logic handles this, it could lead to unexpected behavior. Addingdefault: 0would make the contract consistent.`@Field`({ type: Number, description: 'The offset to start returning results from.', nullable: true, required: false, - minimum: 0 + minimum: 0, + default: 0 })packages/query-rest/src/decorators/field.decorator.ts (1)
129-135: Inconsistent nullish/falsy checks for constraint options.
minLengthandmaxLengthuse truthiness checks, so a value of0would be silently skipped. In contrast,minimumandmaximum(lines 137-143) correctly use!== undefined. Align these for consistency.Proposed fix
- if (options.minLength) { + if (options.minLength !== undefined) { decorators.push(MinLength(options.minLength)) } - if (options.maxLength) { + if (options.maxLength !== undefined) { decorators.push(MaxLength(options.maxLength)) }
| /** | ||
| * Decorator for Fields that should be filterable through a [[FilterType]] | ||
| * | ||
| * @example | ||
| * | ||
| * In the following DTO `id`, `title` and `completed` are filterable. | ||
| * | ||
| * ```ts | ||
| * import { FilterableField } from '@ptc-org/nestjs-query-graphql'; | ||
| * import { ObjectType, ID, GraphQLISODateTime, Field } from '@nestjs/graphql'; | ||
| * | ||
| * @ObjectType('TodoItem') | ||
| * export class TodoItemDTO { | ||
| * @FilterableField(() => ID) | ||
| * id!: string; | ||
| * | ||
| * @FilterableField() | ||
| * title!: string; | ||
| * | ||
| * @FilterableField() | ||
| * completed!: boolean; | ||
| * | ||
| * @Field(() => GraphQLISODateTime) | ||
| * created!: Date; | ||
| * | ||
| * @Field(() => GraphQLISODateTime) | ||
| * updated!: Date; | ||
| * } | ||
| * ``` | ||
| */ |
There was a problem hiding this comment.
JSDoc is copy-pasted from the GraphQL package.
The docblock references @ptc-org/nestjs-query-graphql, @nestjs/graphql, ObjectType, ID, and GraphQLISODateTime — none of which apply to this REST package. Update the example to show REST-appropriate usage (e.g., using @Field with a DTO class for a REST controller).
🤖 Prompt for AI Agents
In `@packages/query-rest/src/decorators/field.decorator.ts` around lines 32 - 61,
The JSDoc example is copied from the GraphQL package and should be replaced with
a REST-appropriate example: update the docblock for the FilterableField
decorator (referenced as FilterableField and TodoItemDTO in this file) to remove
GraphQL imports and types (`@ptc-org/nestjs-query-graphql`, `@nestjs/graphql`,
`ObjectType`, `ID`, `GraphQLISODateTime`) and instead show a simple REST DTO
usage (e.g., import of FilterableField, a plain class TodoItemDTO with typed
properties decorated by FilterableField and standard class-transformer/validator
or plain TypeScript types) so the example demonstrates how to mark DTO fields as
filterable for the REST controller.
| if (isArray && advancedOptions?.forceArray) { | ||
| decorators.push(Transform(({ value }) => (Array.isArray(value) ? value : [value]))) | ||
| } |
There was a problem hiding this comment.
forceArray Transform wraps null/undefined into a single-element array.
When the field is optional and the value is null or undefined, Transform will produce [null] or [undefined], which is likely unintended and would break downstream validation or persistence.
Proposed fix
- decorators.push(Transform(({ value }) => (Array.isArray(value) ? value : [value])))
+ decorators.push(Transform(({ value }) => (Array.isArray(value) ? value : value != null ? [value] : value)))🤖 Prompt for AI Agents
In `@packages/query-rest/src/decorators/field.decorator.ts` around lines 117 -
119, The forceArray Transform currently wraps null/undefined into
[null]/[undefined]; update the Transform pushed into decorators (the one using
Transform in field.decorator.ts tied to advancedOptions?.forceArray) so it first
checks for null or undefined and returns the original value in that case,
otherwise returns the existing array or wraps the value in an array; locate the
Transform call added to the decorators array and change its mapping logic to
skip wrapping when value == null.
1 participant
Summary by CodeRabbit
New Features
Chores