Skip to content

feat(cli): add prompt for plural model name #1314

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
shimks merged 1 commit into from
May 22, 2018
Merged

feat(cli): add prompt for plural model name #1314

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
22 changes: 13 additions & 9 deletions docs/site/Controller-generator.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ to select:

- The model to use for the CRUD function definitions
- The repository for this model that provides datasource connectivity
- The REST path naming convention
- Default: singular and plural forms of the model name in dash-delimited style
are used
Copy link
Member

@bajtos bajtos May 24, 2018

Choose a reason for hiding this comment

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

Since we are using plural form everywhere, this no longer describes the actual implementation. Could you please fix it @shimks?

Copy link
Contributor Author

@shimks shimks May 24, 2018

Choose a reason for hiding this comment

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

Yup, I'll open a PR to fix this

- Custom: users are prompted for custom names of their REST paths

{% include warning.html content= " If you do not have a model and repository to
select, then you will receive an error! " lang=page.lang
Expand Down Expand Up @@ -95,48 +99,48 @@ export class TodoController {
public todoRepository: TodoRepository,
) {}

@post('/todo')
@post('/todos')
async create(@requestBody() obj: Todo): Promise<Todo> {
return await this.todoRepository.create(obj);
}

@get('/todo/count')
@get('/todos/count')
async count(@param.query.string('where') where: Where): Promise<number> {
return await this.todoRepository.count(where);
}

@get('/todo')
@get('/todos')
async find(@param.query.string('filter') filter: Filter): Promise<Todo[]> {
return await this.todoRepository.find(filter);
}

@patch('/todo')
@patch('/todos')
async updateAll(
@param.query.string('where') where: Where,
@reqeustBody() obj: Todo,
@requestBody() obj: Todo,
): Promise<number> {
return await this.todoRepository.updateAll(where, obj);
}

@del('/todo')
@del('/todos')
async deleteAll(@param.query.string('where') where: Where): Promise<number> {
return await this.todoRepository.deleteAll(where);
}

@get('/todo/{id}')
@get('/todos/{id}')
async findById(@param.path.number('id') id: number): Promise<Todo> {
return await this.todoRepository.findById(id);
}

@patch('/todo/{id}')
@patch('/todos/{id}')
async updateById(
@param.path.number('id') id: number,
@requestBody() obj: Todo,
): Promise<boolean> {
return await this.todoRepository.updateById(id, obj);
}

@del('/todo/{id}')
@del('/todos/{id}')
async deleteById(@param.path.number('id') id: number): Promise<boolean> {
return await this.todoRepository.deleteById(id);
}
Expand Down
4 changes: 2 additions & 2 deletions docs/site/Defining-the-API-using-code-first-approach.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,12 +137,12 @@ import {post, get, param, requestBody} from '@loopback/openapi-v3';
export class TodoController {
constructor() {}

@post('/todo') // same as @operation('post', '/todo');
@post('/todos') // same as @operation('post', '/todos');
async createTodo(@requestBody() todo: Todo) {
// data creating logic goes here
}

@get('/todo/{id}')
@get('/todos/{id}')
async findTodoById(
@param.path.number('id') id: number,
@param.query.boolean('items') items?: boolean,
Expand Down
20 changes: 10 additions & 10 deletions docs/site/todo-tutorial-controller.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ summary: LoopBack 4 Todo Application Tutorial - Add a Controller

In LoopBack 4, controllers handle the request-response lifecycle for your API.
Each function on a controller can be addressed individually to handle an
incoming request (like a POST request to `/todo`), perform business logic and
incoming request (like a POST request to `/todos`), perform business logic and
then return a response.

In this respect, controllers are the regions _in which most of your business
Expand Down Expand Up @@ -74,7 +74,7 @@ export class TodoController {
@repository(TodoRepository) protected todoRepo: TodoRepository,
) {}

@post('/todo')
@post('/todos')
Copy link
Member

@bajtos bajtos May 10, 2018

Choose a reason for hiding this comment

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

I think the singular name in the path was intentional here. POST /todo means "create a new TODO instance". POST /todos can be interpreted as "create many TODO instances".

async createTodo(@requestBody() todo: Todo) {
if (!todo.title) {
throw new HttpErrors.BadRequest('title is required');
Expand All @@ -87,7 +87,7 @@ export class TodoController {
In this example, we're using two new decorators to provide LoopBack with
metadata about the route, verb and the format of the incoming request body:

- `@post('/todo')` creates metadata for `@loopback/rest` so that it can redirect
- `@post('/todos')` creates metadata for `@loopback/rest` so that it can redirect
requests to this function when the path and verb match.
- `@requestBody()` associates the OpenAPI schema for a Todo with the body of the
request so that LoopBack can validate the format of an incoming request
Expand Down Expand Up @@ -124,25 +124,25 @@ export class TodoController {
@repository(TodoRepository) protected todoRepo: TodoRepository,
) {}

@post('/todo')
@post('/todos')
async createTodo(@requestBody() todo: Todo) {
if (!todo.title) {
throw new HttpErrors.BadRequest('title is required');
}
return await this.todoRepo.create(todo);
}

@get('/todo/{id}')
@get('/todos/{id}')
Copy link
Member

@bajtos bajtos May 10, 2018

Choose a reason for hiding this comment

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

Similarly here. In REST, it's common and natural to have two path namespaces for a single model - /todo to work with a single model instance, /todos to work with the collection.

Such design has an important property: it avoids naming clashes between instance ids and remote method names - for example, it's possible to have todo with id findOne (available at GET /todo/findOne) and an remote method called findOne (exposed at GET /todos/findOne). This is something we cannot do in LB 3.x.

Copy link
Contributor Author

@shimks shimks May 10, 2018

Choose a reason for hiding this comment

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

With that in mind, I'm wondering whether we should modify our template to 'correct' the designs of our available controller methods. Putting this into example, if given path url was todo, should findById be tied to /todo/{id} while find is tied to /todos?

Copy link
Member

@bajtos bajtos May 10, 2018

Choose a reason for hiding this comment

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

That would be perfect!

My only concern is about the UX and implementation complexity of such solution. I think we will have to prompt for both paths (/todo and /todos), explain the user the differences, and allow them to use the same path for both types of endpoints.

Thoughts?

Copy link
Contributor

@jannyHou jannyHou May 10, 2018

Choose a reason for hiding this comment

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

for example, it's possible to have todo with id findOne (available at GET /todo/findOne) and an remote method called findOne (exposed at GET /todos/findOne). This is something we cannot do in LB 3.x.

👍

My only concern is about the UX and implementation complexity of such solution

I think now we don't append namespace prefix to the path of controller method, e.g. @post('/todo') or @post('/todos'), user need to provide the completed path, maybe we can enhance the REST decorators to allow specifying the namespace prefix:

  • plural
  • singula
  • customized

Copy link
Contributor Author

@shimks shimks May 10, 2018

Choose a reason for hiding this comment

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

The UX of this approach does seem difficult to grasp. Here are all of the possible options that may be available to the users:
When given Todo model,

  • Use todo and todos
  • Use just todo
  • Use just todos
  • Custom http path
    • Use only the custom path given
    • Prompt for custom singular path
      • Use plural form of given answer
      • Custom plural form

As you can see, there are tons of options that can be available to the users and if we were to go with this approach, we should only pick a couple of these to avoid overloading users with choices.

@strongloop/lb-next-dev What do you think?

Copy link
Contributor

@jannyHou jannyHou May 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

my suggestion in #1314 (comment) is totally a different story :) As for this PR I would like to just focus on adding the prompt.
For the singular/plural namespace, my take would be

  • use singular for the item takes in param with type Todo
  • use plural for those take in param with type Todo[]

Copy link
Contributor

@b-admike b-admike May 11, 2018

Choose a reason for hiding this comment

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

For reference, this is how we handle this in LB3. I also like the idea of following REST principles in our templates.

For the singular/plural namespace, my take would be

  • use singular for the item takes in param with type Todo
  • use plural for those take in param with type Todo[]

if this can be easily done, then that would make our UX easier and then we are left with a custom http path prompt. Maybe we can compromise between UX and flexibility.

async findTodoById(@param.path.number('id') id: number): Promise<Todo> {
return await this.todoRepo.findById(id);
}

@get('/todo')
@get('/todos')
async findTodos(): Promise<Todo[]> {
return await this.todoRepo.find();
}

@put('/todo/{id}')
@put('/todos/{id}')
async replaceTodo(
@param.path.number('id') id: number,
@requestBody() todo: Todo,
Expand All @@ -153,7 +153,7 @@ export class TodoController {
return await this.todoRepo.replaceById(id, todo);
}

@patch('/todo/{id}')
@patch('/todos/{id}')
async updateTodo(
@param.path.number('id') id: number,
@requestBody() todo: Todo,
Expand All @@ -162,7 +162,7 @@ export class TodoController {
return await this.todoRepo.updateById(id, todo);
}

@del('/todo/{id}')
@del('/todos/{id}')
async deleteTodo(@param.path.number('id') id: number): Promise<boolean> {
return await this.todoRepo.deleteById(id);
}
Expand All @@ -171,7 +171,7 @@ export class TodoController {

Some additional things to note about this example:

- Routes like `@get('/todo/{id}')` can be paired with the `@param.path`
- Routes like `@get('/todos/{id}')` can be paired with the `@param.path`
decorators to inject those values at request time into the handler function.
- LoopBack's `@param` decorator also contains a namespace full of other
"subdecorators" like `@param.path`, `@param.query`, and `@param.header` that
Expand Down
6 changes: 3 additions & 3 deletions docs/site/todo-tutorial-putting-it-together.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,10 +112,10 @@ your API and make requests!

Here are some requests you can try:

- `POST /todo` with a body of `{ "title": "get the milk" }`
- `GET /todo/{id}` using the ID you received from your `POST`, and see if you
- `POST /todos` with a body of `{ "title": "get the milk" }`
- `GET /todos/{id}` using the ID you received from your `POST`, and see if you
get your Todo object back.
- `PATCH /todo/{id}` with a body of `{ "desc": "need milk for cereal" }`
- `PATCH /todos/{id}` with a body of `{ "desc": "need milk for cereal" }`

That's it! You've just created your first LoopBack 4 application!

Expand Down
12 changes: 6 additions & 6 deletions examples/todo/src/controllers/todo.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {
export class TodoController {
constructor(@repository(TodoRepository) protected todoRepo: TodoRepository) {}

@post('/todo')
@post('/todos')
async createTodo(@requestBody() todo: Todo) {
// TODO(bajtos) This should be handled by the framework
// See https://github.com/strongloop/loopback-next/issues/118
Expand All @@ -30,20 +30,20 @@ export class TodoController {
return await this.todoRepo.create(todo);
}

@get('/todo/{id}')
@get('/todos/{id}')
async findTodoById(
@param.path.number('id') id: number,
@param.query.boolean('items') items?: boolean,
): Promise<Todo> {
return await this.todoRepo.findById(id);
}

@get('/todo')
@get('/todos')
async findTodos(): Promise<Todo[]> {
return await this.todoRepo.find();
}

@put('/todo/{id}')
@put('/todos/{id}')
async replaceTodo(
@param.path.number('id') id: number,
@requestBody() todo: Todo,
Expand All @@ -57,7 +57,7 @@ export class TodoController {
return await this.todoRepo.replaceById(id, todo);
}

@patch('/todo/{id}')
@patch('/todos/{id}')
async updateTodo(
@param.path.number('id') id: number,
@requestBody() todo: Todo,
Expand All @@ -71,7 +71,7 @@ export class TodoController {
return await this.todoRepo.updateById(id, todo);
}

@del('/todo/{id}')
@del('/todos/{id}')
async deleteTodo(@param.path.number('id') id: number): Promise<boolean> {
return await this.todoRepo.deleteById(id);
}
Expand Down
10 changes: 5 additions & 5 deletions examples/todo/test/acceptance/application.acceptance.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ describe('Application', () => {
it('creates a todo', async () => {
const todo = givenTodo();
const response = await client
.post('/todo')
.post('/todos')
.send(todo)
.expect(200);
expect(response.body).to.containEql(todo);
Expand All @@ -44,7 +44,7 @@ describe('Application', () => {
it('gets a todo by ID', async () => {
const todo = await givenTodoInstance();
await client
.get(`/todo/${todo.id}`)
.get(`/todos/${todo.id}`)
.send()
.expect(200, todo);
});
Expand All @@ -57,7 +57,7 @@ describe('Application', () => {
isComplete: true,
});
await client
.put(`/todo/${todo.id}`)
.put(`/todos/${todo.id}`)
.send(updatedTodo)
.expect(200);
const result = await todoRepo.findById(todo.id);
Expand All @@ -71,7 +71,7 @@ describe('Application', () => {
isComplete: true,
});
await client
.patch(`/todo/${todo.id}`)
.patch(`/todos/${todo.id}`)
.send(updatedTodo)
.expect(200);
const result = await todoRepo.findById(todo.id);
Expand All @@ -81,7 +81,7 @@ describe('Application', () => {
it('deletes the todo', async () => {
const todo = await givenTodoInstance();
await client
.del(`/todo/${todo.id}`)
.del(`/todos/${todo.id}`)
.send()
.expect(200);
try {
Expand Down
15 changes: 12 additions & 3 deletions packages/cli/generators/controller/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,18 @@ module.exports = class ControllerGenerator extends ArtifactGenerator {
when: this.artifactInfo.idType === undefined,
default: 'number',
},
{
type: 'input',
name: 'httpPathName',
message: 'What is the base HTTP path name of the CRUD operations?',
when: this.artifactInfo.httpPathName === undefined,
default: answers =>
utils.prependBackslash(
utils.pluralize(utils.urlSlug(answers.modelName)),
),
validate: utils.validateUrlSlug,
filter: utils.prependBackslash,
},
]).then(props => {
debug(`props: ${inspect(props)}`);
Object.assign(this.artifactInfo, props);
Expand All @@ -159,9 +171,6 @@ module.exports = class ControllerGenerator extends ArtifactGenerator {
this.artifactInfo.repositoryNameCamel = utils.camelCase(
this.artifactInfo.repositoryName,
);
this.artifactInfo.modelNameCamel = utils.camelCase(
this.artifactInfo.modelName,
);
return props;
});
})
Expand Down
41 changes: 22 additions & 19 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 @@ -12,53 +12,56 @@ import {<%= modelName %>} from '../models';
import {<%= repositoryName %>} from '../repositories';

export class <%= name %>Controller {

constructor(
@repository(<%= repositoryName %>)
public <%= repositoryNameCamel %> : <%= repositoryName %>,
) {}

@post('/<%= modelNameCamel %>')
@post('<%= httpPathName %>')
async create(@requestBody() obj: <%= modelName %>)
: Promise<<%= modelName %>> {
return await this.<%= repositoryNameCamel %>.create(obj);
}

@get('/<%= modelNameCamel %>/count')
async count(@param.query.string('where') where: Where) : Promise<number> {
@get('<%= httpPathName %>/count')
async count(@param.query.string('where') where: Where): Promise<number> {
return await this.<%= repositoryNameCamel %>.count(where);
}

@get('/<%= modelNameCamel %>')
@get('<%= httpPathName %>')
async find(@param.query.string('filter') filter: Filter)
: Promise<<%= modelName %>[]> {
return await this.<%= repositoryNameCamel %>.find(filter);
return await this.<%= repositoryNameCamel %>.find(filter);
}

@patch('/<%= modelNameCamel %>')
async updateAll(@param.query.string('where') where: Where,
@requestBody() obj: <%= modelName %>) : Promise<number> {
return await this.<%= repositoryNameCamel %>.updateAll(where, obj);
@patch('<%= httpPathName %>')
async updateAll(
@param.query.string('where') where: Where,
@requestBody() obj: <%= modelName %>
): Promise<number> {
return await this.<%= repositoryNameCamel %>.updateAll(where, obj);
}

@del('/<%= modelNameCamel %>')
async deleteAll(@param.query.string('where') where: Where) : Promise<number> {
@del('<%= httpPathName %>')
async deleteAll(@param.query.string('where') where: Where): Promise<number> {
return await this.<%= repositoryNameCamel %>.deleteAll(where);
}

@get('/<%= modelNameCamel %>/{id}')
async findById(@param.path.number('id') id: <%= idType %>) : Promise<<%= modelName %>> {
@get('<%= httpPathName %>/{id}')
async findById(@param.path.number('id') id: <%= idType %>): Promise<<%= modelName %>> {
return await this.<%= repositoryNameCamel %>.findById(id);
}

@patch('/<%= modelNameCamel %>/{id}')
async updateById(@param.path.number('id') id: <%= idType %>, @requestBody()
obj: <%= modelName %>) : Promise<boolean> {
@patch('<%= httpPathName %>/{id}')
async updateById(
@param.path.number('id') id: <%= idType %>,
@requestBody() obj: <%= modelName %>
): Promise<boolean> {
return await this.<%= repositoryNameCamel %>.updateById(id, obj);
}

@del('/<%= modelNameCamel %>/{id}')
async deleteById(@param.path.number('id') id: <%= idType %>) : Promise<boolean> {
@del('<%= httpPathName %>/{id}')
async deleteById(@param.path.number('id') id: <%= idType %>): Promise<boolean> {
return await this.<%= repositoryNameCamel %>.deleteById(id);
}
}
Loading