-
Notifications
You must be signed in to change notification settings - Fork 57
feat(rest): Created rest package #200
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 49cd3a4. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 5 targetsSent with 💌 from NxCloud. |
packages/query-rest/src/decorators/filterable-field.decorator.ts
Outdated
Show resolved
Hide resolved
# Conflicts: # examples/project.json # package.json # yarn.lock
☁️ Nx Cloud ReportCI is running/has finished running commands for commit e2353ec. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution
✅ Successfully ran 2 targetsSent with 💌 from NxCloud. |
# 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
@TriPSs What's going on in this PR? Kinda curious as to on what you're working in this branch. The name kinda does not ring a bell. I mean what do you mean by "rest"? |
The current packages are focused on building a GraphQL API, this will add a package to create/support REST (Get, Post, Put, Delete) API (More traditional), I'm currently still building this out in inside a other project who will almost go live, so once that is done i'm back porting it here and will implement some tests so it can be released. |
OK, so basically it will be a new package next to the others we have already. But I am worried if this will introduce extra functionality which just might complicate the source code unnecessarily. Will skim through in the next week. |
Well it's something I wanted/needed so i'm still going to add it, it's open for others to then also use if they don't want GraphQL. Also since it's completely islolated in it's own package (just like GraphQL is) and uses the other services (just like the GraphQL one does) I do not see how this complicates the code base, as people only tend to look at the things they need/use (if they look at all). |
Well the idea is to be exactly like the GraphQL one (but with minor differences as not everything in GraphQL is available in REST), with full swagger/open api support so frontend types/libs can still be automatically generated :) Once the internal version is live I will update this PR with the latest version, improve some naming, cleanup, add tests and merge it in for others to use. |
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughAdds a new query-rest package with REST CRUD infrastructure (decorators, resolvers, interceptors, providers, paging/connection types), config and tests. Introduces a basic NestJS REST example app (DTOs, entities, modules), e2e test and OpenAPI generator/spec. Updates root configs (Jest mapping, dependency), and GitHub automation (Dependabot, stale bot). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant Ctl as REST Controller (Auto-generated)
participant IntA as AuthorizerInterceptor
participant IntH as HookInterceptor
participant S as QueryService
participant DB as DB/Repo
Note over Ctl: GET /todo-item-dtos?limit&offset
C->>Ctl: HTTP GET
activate Ctl
Ctl->>IntA: apply
IntA->>Ctl: attach request.authorizer
Ctl->>IntH: apply
IntH->>Ctl: attach request.hooks
Ctl->>S: queryMany(query, authorizeFilter)
S->>DB: find with paging/filter
DB-->>S: rows, count?
S-->>Ctl: nodes (+pageInfo, totalCount?)
Ctl-->>C: 200 JSON (ConnectionType)
deactivate Ctl
sequenceDiagram
autonumber
participant C as Client
participant Ctl as REST Controller (Auto-generated)
participant IntA as AuthorizerInterceptor
participant IntH as HookInterceptor(BEFORE_CREATE_ONE)
participant S as QueryService
participant DB as DB/Repo
Note over Ctl: POST /todo-item-dtos
C->>Ctl: HTTP POST (CreateDTO)
activate Ctl
Ctl->>IntA: apply
IntA->>Ctl: attach authorizer
Ctl->>IntH: apply
IntH->>Ctl: attach hooks
Ctl->>S: createOne(input)
S->>DB: insert
DB-->>S: created row
S-->>Ctl: DTO
Ctl-->>C: 201 JSON (DTO)
deactivate Ctl
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 33
{ | ||
"name": "id", | ||
"required": true, | ||
"in": "path", | ||
"schema": { | ||
"type": "string" | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Path id type mismatches the DTOs.
DTO ids are numbers, but path params are strings. Switch to number (or integer) for consistency and validation.
- "schema": { "type": "string" }
+ "schema": { "type": "number" }
Apply to sub-task, todo-item, and tag item paths.
Also applies to: 171-177, 331-337
🤖 Prompt for AI Agents
In examples/basic-rest/open-api.json around lines 10 to 17 (and similarly at
171-177 and 331-337), the path parameter "id" is declared as a string while the
DTOs use numeric ids; update the "schema" for these path parameters to use type
"integer" (or "number" if decimals are expected) and add "format": "int64" (or
"int32" as appropriate) to match the DTOs, ensuring required remains true and
validation will accept numeric ids for sub-task, todo-item, and tag endpoints.
"requestBody": { | ||
"required": true, | ||
"content": { | ||
"application/json": { | ||
"schema": { | ||
"$ref": "#/components/schemas/CreateTagDTO" | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tag update uses create schema.
PUT updates should not accept CreateTagDTO. Add a TagUpdateDTO (optional name) and reference it here.
"requestBody": {
"required": true,
"content": {
"application/json": {
"schema": {
- "$ref": "#/components/schemas/CreateTagDTO"
+ "$ref": "#/components/schemas/TagUpdateDTO"
}
}
}
},
Add schema:
+"TagUpdateDTO": {
+ "type": "object",
+ "properties": {
+ "name": { "type": "string", "nullable": true }
+ }
+},
📝 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.
"requestBody": { | |
"required": true, | |
"content": { | |
"application/json": { | |
"schema": { | |
"$ref": "#/components/schemas/CreateTagDTO" | |
} | |
} | |
} | |
"requestBody": { | |
"required": true, | |
"content": { | |
"application/json": { | |
"schema": { | |
"$ref": "#/components/schemas/TagUpdateDTO" | |
} | |
} | |
} | |
}, |
"requestBody": { | |
"required": true, | |
"content": { | |
"application/json": { | |
"schema": { | |
"$ref": "#/components/schemas/CreateTagDTO" | |
} | |
} | |
} | |
"TagUpdateDTO": { | |
"type": "object", | |
"properties": { | |
"name": { "type": "string", "nullable": true } | |
} | |
}, |
🤖 Prompt for AI Agents
In examples/basic-rest/open-api.json around lines 393 to 401, the PUT
requestBody currently references CreateTagDTO which is incorrect for updates;
add a new components/schemas/TagUpdateDTO with the same properties as
CreateTagDTO but make fields optional (e.g., name optional) and replace the
requestBody schema $ref to "#/components/schemas/TagUpdateDTO"; ensure the new
schema is added under components.schemas and referenced by the PUT operation.
@FilterableField() | ||
todoItemId!: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todoItemId type should match TodoItemEntity.id (number).
Keeps the REST surface consistent with the entity and other DTOs.
@FilterableField()
- todoItemId!: string
+ todoItemId!: number
📝 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.
@FilterableField() | |
todoItemId!: string | |
@FilterableField() | |
todoItemId!: number |
🤖 Prompt for AI Agents
In examples/basic-rest/src/sub-task/dto/sub-task.dto.ts around lines 22 to 23,
the todoItemId property is declared as a string but should be a number to match
TodoItemEntity.id and other DTOs; change the property type from string to number
(and adjust any decorators or validators if they assume string) so the DTO's
todoItemId uses number throughout the REST surface to remain consistent with the
entity.
@Field() | ||
@IsNotEmpty() | ||
todoItemId!: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todoItemId should be numeric and validated.
Entity TodoItemEntity.id is a number; keep DTOs consistent and coerce/validate.
@Field()
@IsNotEmpty()
- todoItemId!: string
+ @Type(() => Number)
+ @IsNumber()
+ todoItemId!: number
📝 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.
@Field() | |
@IsNotEmpty() | |
todoItemId!: string | |
@Field() | |
@IsNotEmpty() | |
@Type(() => Number) | |
@IsNumber() | |
todoItemId!: number |
🤖 Prompt for AI Agents
In examples/basic-rest/src/sub-task/dto/subtask-input.dto.ts around lines 20 to
22, the todoItemId is declared as a string but the entity TodoItemEntity.id is
numeric; change the DTO to accept a numeric id and validate/transform it by
making the property a number, adding class-validator checks (e.g., @IsInt or
@IsNumber) and using class-transformer's @Type(() => Number) to coerce incoming
values to number (ensure the validation pipe is enabled so non-numeric input is
rejected).
@ManyToMany((): ObjectType<TodoItemEntity> => TodoItemEntity, (td) => td.tags) | ||
todoItems!: TodoItemEntity[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the ManyToMany decorator usage.
The ObjectType<TodoItemEntity>
usage is incorrect. TypeORM's @ManyToMany
decorator expects a function returning the entity class directly.
-@ManyToMany((): ObjectType<TodoItemEntity> => TodoItemEntity, (td) => td.tags)
+@ManyToMany(() => TodoItemEntity, (td) => td.tags)
📝 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.
@ManyToMany((): ObjectType<TodoItemEntity> => TodoItemEntity, (td) => td.tags) | |
todoItems!: TodoItemEntity[] | |
@ManyToMany(() => TodoItemEntity, (td) => td.tags) | |
todoItems!: TodoItemEntity[] |
🤖 Prompt for AI Agents
In examples/basic-rest/src/tag/tag.entity.ts around lines 19 to 20, the
@ManyToMany decorator is using an incorrect ObjectType<TodoItemEntity> wrapper;
change it to pass the entity class directly by replacing the first argument with
a function that returns the entity class (i.e. () => TodoItemEntity) and keep
the inverse side (td => td.tags); ensure TodoItemEntity is imported so the
decorator reads like @ManyToMany(() => TodoItemEntity, td => td.tags) and
nothing else needs to change.
export type NonePagingType = Paging | ||
export type OffsetPagingType = Paging | ||
|
||
// eslint-disable-next-line @typescript-eslint/no-duplicate-type-constituents | ||
export type PagingTypes = OffsetPagingType | NonePagingType | ||
export type InferPagingTypeFromStrategy<PS extends PagingStrategies> = PS extends PagingStrategies.OFFSET | ||
? OffsetPagingType | ||
: PS extends PagingStrategies.NONE | ||
? NonePagingType | ||
: never |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Union of identical aliases erases type discrimination; add lightweight brands.
-import { Paging } from '@ptc-org/nestjs-query-core'
+import { Paging } from '@ptc-org/nestjs-query-core'
@@
-export type NonePagingType = Paging
-export type OffsetPagingType = Paging
-
-// eslint-disable-next-line @typescript-eslint/no-duplicate-type-constituents
-export type PagingTypes = OffsetPagingType | NonePagingType
+type Brand<T, B extends string> = T & { readonly __pagingBrand?: B }
+export type NonePagingType = Brand<Paging, 'none'>
+export type OffsetPagingType = Brand<Paging, 'offset'>
+export type PagingTypes = OffsetPagingType | NonePagingType
export type InferPagingTypeFromStrategy<PS extends PagingStrategies> = PS extends PagingStrategies.OFFSET
? OffsetPagingType
: PS extends PagingStrategies.NONE
? NonePagingType
: never
📝 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 type NonePagingType = Paging | |
export type OffsetPagingType = Paging | |
// eslint-disable-next-line @typescript-eslint/no-duplicate-type-constituents | |
export type PagingTypes = OffsetPagingType | NonePagingType | |
export type InferPagingTypeFromStrategy<PS extends PagingStrategies> = PS extends PagingStrategies.OFFSET | |
? OffsetPagingType | |
: PS extends PagingStrategies.NONE | |
? NonePagingType | |
: never | |
type Brand<T, B extends string> = T & { readonly __pagingBrand?: B } | |
export type NonePagingType = Brand<Paging, 'none'> | |
export type OffsetPagingType = Brand<Paging, 'offset'> | |
export type PagingTypes = OffsetPagingType | NonePagingType | |
export type InferPagingTypeFromStrategy<PS extends PagingStrategies> = PS extends PagingStrategies.OFFSET | |
? OffsetPagingType | |
: PS extends PagingStrategies.NONE | |
? NonePagingType | |
: never |
🤖 Prompt for AI Agents
In packages/query-rest/src/types/query/paging/interfaces.ts around lines 5 to
14, the two paging aliases are identical which removes type discrimination; make
each alias a lightweight branded/nominal type so the union preserves
distinction. Replace NonePagingType and OffsetPagingType with intersections that
add a unique phantom property or symbol (e.g. Paging & { __pagingKind: 'none' }
and Paging & { __pagingKind: 'offset' }) and update PagingTypes and
InferPagingTypeFromStrategy to use those branded types so conditional types
correctly discriminate on PagingStrategies.
export const DEFAULT_QUERY_OPTS = { | ||
defaultResultSize: 10, | ||
maxResultsSize: 50, | ||
defaultSort: [], | ||
defaultFilter: {} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Freeze defaults to prevent cross-request/global mutation
defaultSort: []
and defaultFilter: {}
are mutable and exported; accidental mutation will leak across consumers/requests.
Apply this diff:
-export const DEFAULT_QUERY_OPTS = {
- defaultResultSize: 10,
- maxResultsSize: 50,
- defaultSort: [],
- defaultFilter: {}
-}
+export const DEFAULT_QUERY_OPTS = Object.freeze({
+ defaultResultSize: 10,
+ maxResultsSize: 50,
+ // Freeze nested structures to prevent accidental mutation
+ defaultSort: Object.freeze([] as const),
+ defaultFilter: Object.freeze({} as const)
+} as const)
📝 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 const DEFAULT_QUERY_OPTS = { | |
defaultResultSize: 10, | |
maxResultsSize: 50, | |
defaultSort: [], | |
defaultFilter: {} | |
} | |
export const DEFAULT_QUERY_OPTS = Object.freeze({ | |
defaultResultSize: 10, | |
maxResultsSize: 50, | |
// Freeze nested structures to prevent accidental mutation | |
defaultSort: Object.freeze([] as const), | |
defaultFilter: Object.freeze({} as const) | |
} as const) |
🤖 Prompt for AI Agents
In packages/query-rest/src/types/query/query-args/constants.ts around lines 1 to
6, the exported DEFAULT_QUERY_OPTS contains mutable defaults (defaultSort: []
and defaultFilter: {}) that can be mutated across requests; replace these with
frozen values and freeze the container to prevent accidental mutation (e.g. use
Object.freeze([]) for defaultSort, Object.freeze({}) for defaultFilter and
Object.freeze(...) on the DEFAULT_QUERY_OPTS object so none of its properties
can be altered at runtime).
public buildQuery(): RestQuery<DTO> { | ||
return { | ||
query: this.query, | ||
paging: { | ||
limit: this.limit || opts.maxResultsSize, | ||
offset: this.offset | ||
}, | ||
filter: this.filter, | ||
sorting: this.sorting, | ||
relations: [] | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fallback for maxResultsSize needs validation
The maxResultsSize
fallback in buildQuery
could allow unbounded queries if both limit
and maxResultsSize
are undefined.
public buildQuery(): RestQuery<DTO> {
+ const effectiveLimit = this.limit || opts.maxResultsSize
+ if (effectiveLimit === undefined || effectiveLimit <= 0) {
+ throw new Error('A valid limit must be specified for queries')
+ }
return {
query: this.query,
paging: {
- limit: this.limit || opts.maxResultsSize,
+ limit: effectiveLimit,
offset: this.offset
},
filter: this.filter,
sorting: this.sorting,
relations: []
}
}
📝 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 buildQuery(): RestQuery<DTO> { | |
return { | |
query: this.query, | |
paging: { | |
limit: this.limit || opts.maxResultsSize, | |
offset: this.offset | |
}, | |
filter: this.filter, | |
sorting: this.sorting, | |
relations: [] | |
} | |
} | |
public buildQuery(): RestQuery<DTO> { | |
const effectiveLimit = this.limit || opts.maxResultsSize | |
if (effectiveLimit === undefined || effectiveLimit <= 0) { | |
throw new Error('A valid limit must be specified for queries') | |
} | |
return { | |
query: this.query, | |
paging: { | |
limit: effectiveLimit, | |
offset: this.offset | |
}, | |
filter: this.filter, | |
sorting: this.sorting, | |
relations: [] | |
} | |
} |
/** @internal */ | ||
export function IsUndefined(validationOptions?: ValidationOptions) { | ||
// eslint-disable-next-line @typescript-eslint/ban-types | ||
return (obj: Object, property: string) => | ||
ValidateIf((o: Record<string, unknown>) => o[property] !== undefined, validationOptions)(obj, property) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming and behavior mismatch; clarify contract or rename.
This decorator doesn’t validate “is undefined”; it gates other validators to run when value is not undefined. That’s easy to misuse. Options:
- Keep behavior but rename (e.g.,
ValidateIfDefined
), or - Implement a true validator that fails when value is defined.
Given this is a new public surface, align name/behavior before release to avoid breaking changes later.
"compilerOptions": { | ||
"module": "commonjs", | ||
"outDir": "../../dist/out-tsc", | ||
"declaration": true, | ||
"types": [ | ||
"node" | ||
] | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add composite: true
to enable project references build.
With packages/query-rest/tsconfig.json
referencing this project, TypeScript requires the referenced project to be composite. Without it, tsc -b
will fail.
Apply this diff:
"compilerOptions": {
"module": "commonjs",
"outDir": "../../dist/out-tsc",
"declaration": true,
+ "composite": true,
"types": [
"node"
]
},
📝 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.
"compilerOptions": { | |
"module": "commonjs", | |
"outDir": "../../dist/out-tsc", | |
"declaration": true, | |
"types": [ | |
"node" | |
] | |
}, | |
"compilerOptions": { | |
"module": "commonjs", | |
"outDir": "../../dist/out-tsc", | |
"declaration": true, | |
"composite": true, | |
"types": [ | |
"node" | |
] | |
}, |
🤖 Prompt for AI Agents
In packages/query-rest/tsconfig.lib.json around lines 3 to 10, the
compilerOptions block is missing "composite": true which is required for project
references; update the file by adding "composite": true to the compilerOptions
object so this project can be referenced and built with tsc -b (ensure the JSON
remains valid and commas are adjusted as needed).
TODO
@FilterableField
None
paging typeMaybe create separate package with the shared code between
rest
andgraphql
packages (Like the interceptors)?Summary by CodeRabbit
New Features
Tests
Documentation
Chores