Skip to content

feat(cli): update controller template to enable filter for findById endpoint #4114

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 14, 2019
Merged

feat(cli): update controller template to enable filter for findById endpoint #4114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions docs/site/Controller-generator.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,12 +202,19 @@ export class TodoController {
responses: {
'200': {
description: 'Todo model instance',
content: {'application/json': {schema: getModelSchemaRef(Todo)}},
content: {
'application/json': {
schema: getModelSchemaRef(Todo, {includeRelations: true}),
},
},
},
},
})
async findById(@param.path.number('id') id: number): Promise<Todo> {
return this.todoRepository.findById(id);
async findById(
@param.path.number('id') id: number,
@param.query.object('filter', getFilterSchemaFor(Todo)) filter?: Filter<Todo>
): Promise<Todo> {
return this.todoRepository.findById(id, filter);
}

@patch('/todos/{id}', {
Expand Down
18 changes: 14 additions & 4 deletions docs/site/Sequence.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,15 +231,25 @@ from the path object.
responses: {
'200': {
description: 'Note model instance',
content: {'application/json': {schema: getModelSchemaRef(Note)}},
content: {
'application/json': {
schema: getModelSchemaRef(Note, {includeRelations: true}),
},
},
},
},
})
async findById(@param.path.string('id') id: string): Promise<Note> {
return this.noteRepository.findById(id);
async findById(
@param.path.string('id') id: string,
@param.query.object('filter', getFilterSchemaFor(Note)) filter?: Filter<Note>
): Promise<Note> {
return this.noteRepository.findById(id, filter);
}
```

(Notice: the filter for `findById()` method only supports the `include` clause
for now.)

You can also specify a parameter which is an object value encoded as a JSON
string or in multiple nested keys. For a JSON string, a sample value would be
`location={"lang": 23.414, "lat": -98.1515}`. For the same `location` object, it
Expand Down Expand Up @@ -378,7 +388,7 @@ details.
```

During development and testing, it may be useful to see all error details in the
HTTP responsed returned by the server. This behavior can be enabled by enabling
HTTP response returned by the server. This behavior can be enabled by enabling
the `debug` flag in error-handler configuration as shown in the code example
below. See strong-error-handler
[docs](https://github.com/strongloop/strong-error-handler#options) for a list of
Expand Down
18 changes: 14 additions & 4 deletions packages/cli/generators/controller/templates/src/controllers/controller-rest-template.ts.ejs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,10 @@ export class <%= className %>Controller {
description: 'Array of <%= modelName %> model instances',
content: {
'application/json': {
schema: {type: 'array', items: getModelSchemaRef(<%= modelName %>)},
schema: {
type: 'array',
items: getModelSchemaRef(<%= modelName %>, {includeRelations: true}),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

},
},
},
},
Expand Down Expand Up @@ -108,12 +111,19 @@ export class <%= className %>Controller {
responses: {
'200': {
description: '<%= modelName %> model instance',
content: {'application/json': {schema: getModelSchemaRef(<%= modelName %>)}},
content: {
'application/json': {
schema: getModelSchemaRef(<%= modelName %>, {includeRelations: true}),
},
},
},
},
})
async findById(@param.path.<%= idType %>('id') id: <%= idType %>): Promise<<%= modelName %>> {
return this.<%= repositoryNameCamel %>.findById(id);
async findById(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

based on your question in comment, the response schema should also be updated to

content: {
  `application/json`: {
      schema: getModelSchemaRef(<%= modelName %>, includeRelations),
    }
}

@param.path.<%= idType %>('id') id: <%= idType %>,
@param.query.object('filter', getFilterSchemaFor(<%= modelName %>)) filter?: Filter<<%= modelName %>>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we support all filter properties in findById? For example, I find it weird to combine findById with where or limit/skip filter properties, because findById always returns a single record matching the given id only. Should we perhaps describe only filter.include and filter.fields properties in the schema?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See also #3336 Filter on findById is ignoring the where clause.

I am concerned that if we describe filter parameter of findById method as accepting all filter properties, then our users will try to set those (unsupported) properties and get surprised/confused.

Copy link
Contributor Author

@agnes512 agnes512 Nov 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bajtos I think findById only works for inclusion atm.

And I agree that the filter for findById should work for inclusion and field as you described in #1721. Order, limit, where, etc. don't quite make sense to me.

I'd like to land this PR first, then we can limit it in #1721 to fix #3336.

): Promise<<%= modelName %>> {
return this.<%= repositoryNameCamel %>.findById(id, filter);
}

@patch('<%= httpPathName %>/{id}', {
Expand Down
36 changes: 28 additions & 8 deletions packages/cli/snapshots/integration/generators/controller.integration.snapshots.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,10 @@ export class ProductReviewController {
description: 'Array of ProductReview model instances',
content: {
'application/json': {
schema: {type: 'array', items: getModelSchemaRef(ProductReview)},
schema: {
type: 'array',
items: getModelSchemaRef(ProductReview, {includeRelations: true}),
},
},
},
},
Expand Down Expand Up @@ -118,12 +121,19 @@ export class ProductReviewController {
responses: {
'200': {
description: 'ProductReview model instance',
content: {'application/json': {schema: getModelSchemaRef(ProductReview)}},
content: {
'application/json': {
schema: getModelSchemaRef(ProductReview, {includeRelations: true}),
},
},
},
},
})
async findById(@param.path.number('id') id: number): Promise<ProductReview> {
return this.barRepository.findById(id);
async findById(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto https://github.com/strongloop/loopback-next/pull/4114/files#r346005700

@param.path.number('id') id: number,
@param.query.object('filter', getFilterSchemaFor(ProductReview)) filter?: Filter<ProductReview>
): Promise<ProductReview> {
return this.barRepository.findById(id, filter);
}

@patch('/product-reviews/{id}', {
Expand Down Expand Up @@ -249,7 +259,10 @@ export class ProductReviewController {
description: 'Array of ProductReview model instances',
content: {
'application/json': {
schema: {type: 'array', items: getModelSchemaRef(ProductReview)},
schema: {
type: 'array',
items: getModelSchemaRef(ProductReview, {includeRelations: true}),
},
},
},
},
Expand Down Expand Up @@ -287,12 +300,19 @@ export class ProductReviewController {
responses: {
'200': {
description: 'ProductReview model instance',
content: {'application/json': {schema: getModelSchemaRef(ProductReview)}},
content: {
'application/json': {
schema: getModelSchemaRef(ProductReview, {includeRelations: true}),
},
},
},
},
})
async findById(@param.path.number('id') id: number): Promise<ProductReview> {
return this.barRepository.findById(id);
async findById(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto https://github.com/strongloop/loopback-next/pull/4114/files#r346005700

@param.path.number('id') id: number,
@param.query.object('filter', getFilterSchemaFor(ProductReview)) filter?: Filter<ProductReview>
): Promise<ProductReview> {
return this.barRepository.findById(id, filter);
}

@patch('/product-reviews/{id}', {
Expand Down