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

shimks · 2018-05-09T21:33:20Z

closes [CLI] Prompt for plural of model for REST Controller generation #887

UX:

# after model/repository setup
Select the HTTP path naming convention of this CRUD repository
  - default ('/todo' and '/todos')
  - custom

If custom is selected:

What is the singular base HTTP path of this CRUD repository? (`/todo`)

What is the singular base HTTP path of this CRUD repository? /customer-order
What is the plural base HTTP path of this CRUD repository? (/customer-orders)

Checklist

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
API Documentation in code was updated
Documentation in /docs/site was updated
Affected artifact templates in packages/cli were updated
Affected example projects in examples/* were updated

shimks · 2018-05-09T21:36:39Z

packages/cli/generators/controller/index.js

+            when: this.artifactInfo.modelName === undefined,
+            default: response =>
+              utils.kebabCase(utils.pluralize(response.modelName)),
+            validate: utils.validateUrlSlug,


The prompt would look something like this:

What is the name of the model to use with this CRUD repository? Customer What is the plural form of the model? (customers)

When given a bad input (foo#bar):

What is the plural form of the model? (customers) Invalid url slug. Suggested slug: foo-bar

Once we rephrase the question to ask for URL path, should we include the leading / in the default value?

If we do so, I think we should still accept and normalize values without a leading slash. For example:

User enters "foo-bar" -> we use /foo-bar

User enters "foo/" -> we use /foo

shimks

I've decided to not add in any flags for plural form. Flags for setting model and repository don't exist, and I don't think we want users to specify setting a specific plural name for any potential models they may have; the flag should only be able to be set if a flag for the model (that they want to set the plural name on) can also be set.

bajtos

Please revert the changes of /todo to /todos in the docs and example repose - see the comments below.

bajtos · 2018-05-10T11:30:22Z

docs/site/todo-tutorial-controller.md

  ) {}

-  @post('/todo')
+  @post('/todos')


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".

bajtos · 2018-05-10T11:33:34Z

docs/site/todo-tutorial-controller.md

  }

-  @get('/todo/{id}')
+  @get('/todos/{id}')


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.

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?

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?

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

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?

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[]

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.

bajtos · 2018-05-10T11:37:50Z

docs/site/Controller-generator.md

+to select or specify:

 - The model to use for the CRUD function definitions
+- The plural form of model to use for REST paths


In my experience, the whole thing with plural model forms was rather confusing in LB 3.x. The connection from a plural model form to an HTTP path mapping is not obvious, plus there are examples where a plural form is confusing to people with a non-English primary language (e.g. a Person model is exposed at /people).

Also in LB4, it is possible to have a controller that's not tied to a single model (or any model at all), in which case the plural form makes even less sense.

I am proposing to be explicit about our intents in LB 4.x. When we need to decide how to map model's CRUD methods to a REST API, then ask the user about the base HTTP path to use. The fact that we use a plural form as the default path should be just our implementation detail.

bajtos · 2018-05-10T11:45:13Z

packages/cli/generators/controller/index.js

+            type: 'input',
+            name: 'modelNamePlural',
+            message:
+              'What is the custom plural form of the model (for building REST URL)',


As I commented above, please rephrase this question to make it clear that we are asking for the URL base path.

bajtos · 2018-05-10T11:46:01Z

packages/cli/generators/controller/index.js

+              'What is the custom plural form of the model (for building REST URL)',
+            when: this.artifactInfo.modelName === undefined,
+            default: response =>
+              utils.kebabCase(utils.pluralize(response.modelName)),


Nice, +1 for using kebabCase.

Here is a wider context:

URLs like /ProductReviews are an anti-pattern in the REST world. The convention is to use kebab case, e.g. /product-reviews.

In LB 3.x, we didn't want to break all existing applications, therefore we never changed the legacy behavior. There is a flag to enable kebab case though, see rest.
normalizeHttpPath.

I'd like us to leverage the opportunity to break backwards compatibility that LB 4.0 offers us, and fix the default REST APIs to finally use kebab-case instead of CamelCase in URL paths.

The impact on this pull request should be pretty minimal.

Actually, since you added a dependency on url-slug, I think we should use that one instead of kebabCase. What do you think?

bajtos · 2018-05-10T11:47:26Z

packages/cli/generators/controller/index.js

+            when: this.artifactInfo.modelName === undefined,
+            default: response =>
+              utils.kebabCase(utils.pluralize(response.modelName)),
+            validate: utils.validateUrlSlug,


Once we rephrase the question to ask for URL path, should we include the leading / in the default value?

If we do so, I think we should still accept and normalize values without a leading slash. For example:

User enters "foo-bar" -> we use /foo-bar

User enters "foo/" -> we use /foo

bajtos · 2018-05-10T11:49:13Z

packages/cli/generators/controller/templates/src/controllers/controller-rest-template.ts.ejs

  ) {}

-  @post('/<%= modelNameCamel %>')
+  @post('/<%= modelNamePluralKebab %>')


Our variable names should reflect the intent (what is the URL path), not the implementation details (a plural+kebab version of the name). Let's rename this variable to something like modelUrlPath please.

bajtos · 2018-05-10T11:50:42Z

packages/cli/generators/controller/index.js

+              'What is the custom plural form of the model (for building REST URL)',
+            when: this.artifactInfo.modelName === undefined,
+            default: response =>
+              utils.kebabCase(utils.pluralize(response.modelName)),


Actually, since you added a dependency on url-slug, I think we should use that one instead of kebabCase. What do you think?

bajtos · 2018-05-10T11:52:00Z

packages/cli/test/integration/generators/controller.integration.js

+            generator,
+            Object.assign(
+              {
+                modelName: 'Foo',


Please use a multi-word name to verify how it is converted to an URL. It's also better to use real-world names instead of generic Foo.

A proposed model name: ProductReview.

shimks · 2018-05-11T19:10:52Z

packages/cli/test/unit/utils.unit.js

    }
  });
+
+  describe('validateUrlSlug', () => {


With the way validateUrlSlug is currently coded, the function accepts camel casings along with other words separated by url-safe separators (such as _). Should we potentially look to enforce kebab-casing?

jannyHou · 2018-05-11T19:16:00Z

docs/site/Controller-generator.md

+  @patch('/todos')
  async updateAll(
    @param.query.string('where') where: Where,
    @reqeustBody() obj: Todo,


https://github.com/strongloop/loopback-next/blob/master/packages/repository/src/repositories/legacy-juggler-bridge.ts#L169
Seems like updateAll only takes in a single instance of Todo

jannyHou · 2018-05-11T19:38:37Z

packages/cli/generators/controller/templates/src/controllers/controller-rest-template.ts.ejs

+  @patch('<%= httpPathPlural %>')
+  async updateAll(
+    @param.query.string('where') where: Where,
+    @requestBody() obj: <%= modelName %>


objs: <%= modelName %>[]

Posting what I posted above:
https://github.com/strongloop/loopback-next/blob/master/packages/repository/src/repositories/legacy-juggler-bridge.ts#L169
Seems like updateAll only takes in a single instance of Todo

IIRC, updateAll works more like patchAll, it accepts a single key/value object containing changes to apply to all model instances matching the "where" filter. The correct type definition would be Partial<ProductReview> (replace ProductReview with the actual modelName).

oh sorry, my bad, @bajtos is correct ^
👍

bajtos

LGTM, please consider my comments below before merging.

bajtos · 2018-05-15T10:09:49Z

packages/cli/lib/utils.js

+ * Adds a backslash to the start of the word if not already present
+ * @param {string} httpPath
+ */
+exports.appendBackslash = httpPath => httpPath.replace(/^\/?/, '/');


"append" usually means to add content at the end of the string. I am proposing to use "prepend" instead, i.e. prependBackslash.

bajtos · 2018-05-15T10:17:54Z

packages/cli/generators/controller/index.js

+            type: 'list',
+            name: 'httpPathNameChoices',
+            message:
+              'Select the HTTP path naming convention of this CRUD repository',


IMO, this prompt is redundant. Since the next two prompts (httpPathSingular and httpPathPlural) are already offering default values, users can tell us to use the default convention by simply hitting Enter key twice to accept both proposed defaults.

Thoughts?

Although I agree, I personally like that users would have to only hit Enter once in most cases with this approach. I also think the prompt is well organized in sense that first-time users don't have to stop to think about what singular HTTP path name is supposed to mean.

Not really against your suggestion though. Thoughts @strongloop/lb-next-dev ?

first-time users don't have to stop to think about what singular HTTP path name is supposed to mean.

This is a good point that I haven't considered. Let's keep the initial prompt for the naming convention then.

bajtos · 2018-05-15T10:19:46Z

packages/cli/generators/controller/templates/src/controllers/controller-rest-template.ts.ejs

+  @patch('<%= httpPathPlural %>')
+  async updateAll(
+    @param.query.string('where') where: Where,
+    @requestBody() obj: <%= modelName %>


IIRC, updateAll works more like patchAll, it accepts a single key/value object containing changes to apply to all model instances matching the "where" filter. The correct type definition would be Partial<ProductReview> (replace ProductReview with the actual modelName).

virkt25 · 2018-05-15T17:15:29Z

Great design! I really like the simple vs. customization approach allowing the user to power through the prompts. I'm not a fan of the / in the custom default value since a user might be inclined to think they have to enter it as such ... even though I know we add it if it's not there. Just looks weird but it's probably just me.

Great work :)

raymondfeng · 2018-05-16T16:11:26Z

Sorry, I found the prompts and mixed usage of singular and plural forms for base paths for the same controller very confusing.

Here is an example of generated controller class:

export class TestController {
  constructor(
    @repository(CustomerRepository)
    public customerRepository : CustomerRepository,
  ) {}

  @post('/customer')
  async create(@requestBody() obj: Customer)
    : Promise<Customer> {
    return await this.customerRepository.create(obj);
  }

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

  @get('/customers')
  async find(@param.query.string('filter') filter: Filter)
    : Promise<Customer[]> {
    return await this.customerRepository.find(filter);
  }

  @patch('/customers')
  async updateAll(
    @param.query.string('where') where: Where,
    @requestBody() obj: Customer
  ): Promise<number> {
    return await this.customerRepository.updateAll(where, obj);
  }

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

  @get('/customer/{id}')
  async findById(@param.path.number('id') id: string): Promise<Customer> {
    return await this.customerRepository.findById(id);
  }

  @patch('/customer/{id}')
  async updateById(
    @param.path.number('id') id: string,
    @requestBody() obj: Customer
  ): Promise<boolean> {
    return await this.customerRepository.updateById(id, obj);
  }

  @del('/customer/{id}')
  async deleteById(@param.path.number('id') id: string): Promise<boolean> {
    return await this.customerRepository.deleteById(id);
  }
}

I would NEVER expect to use different forms of base paths for the same controller:

GET /customers
POST /customer -> BAD
GET /customer/{id} -> BAD
PATCH /customer/{id} -> BAD
DELETE /customer/{id} -> BAD

I would expect the base path to be same for all methods under the same controller. Once we have the @path decorator, I would like it to be used at the class level.
We should only EITHER choose singular form OR plural form
The prompt should be simplified as follows:

Base path for the controller:

raymondfeng

I think the PR is heading to a wrong direction.

shimks · 2018-05-16T16:20:16Z

@raymondfeng I think it all depends on stylistic choices we want to advocate to users. From what I've seen on the net, people are split on the design here; some people think it's good to have the endpoints make sense linguistically (e.g. getting an ID of a resource should be singular) while others expect the identifier to be the same for the operations done with that resource.

I see both sides of the argument and don't have a strong opinion for either and wouldn't be opposed to reverting the singular/plural distinction. @bajtos, do you have a strong opinion on having the REST path be linguistically correct?

shimks · 2018-05-16T16:22:38Z

Now that I think about the @path point, I think it makes sense to have the base path name be the same for all of the controller's operations.

jannyHou · 2018-05-16T16:25:13Z

The discussion is folded: #1314 (comment)

cc @raymondfeng for your comment, see the related discussion ^^

raymondfeng · 2018-05-16T16:26:10Z

I think it all depends on stylistic choices we want to advocate to users. From what I've seen on the net, people are split on the design here; some people think it's good to have the endpoints make sense linguistically (e.g. getting an ID of a resource should be singular) while others expect the identifier to be the same for the operations done with that resource.

I never see any REST APIs use mixed forms of singular and plural base paths for the same resource (implemented by a controller). Please provide a link if you see so.

The purpose of the prompt should be just getting a consistent base path for the controller. We derive a default value from the model name for confirmation/customization.

raymondfeng · 2018-05-16T16:30:42Z

Sorry, I’m a bit late in the game. But having to use different base paths for the same controller/resource is really confusing. Think about the basePath as the id/name of a resource. We won’t have two different resource names for the same controller depending on which operation is called.

raymondfeng · 2018-05-16T16:41:22Z

Adding a few links for references:

shimks · 2018-05-16T16:48:43Z

You've convinced me. I'll be changing the PR to have all the operations available under the same base path

bajtos · 2018-05-17T07:24:19Z

I never see any REST APIs use mixed forms of singular and plural base paths for the same resource (implemented by a controller). Please provide a link if you see so.

At some point in the past, GitHub used to have different URLs for accessing a collection of resources and an individual resource. For example:

https://github.com/strongloop/loopback-next/pulls
https://github.com/strongloop/loopback-next/pull/1336

It looks like they have moved away from that pattern since then, their v3 API uses the same plural form now - see https://developer.github.com/v3/pulls/

In that light, I am fine with using the same base path for all controller methods, as proposed by Raymond. Sorry for derailing this pull request and wasting our time on a wrong direction 🙈

I have one concern though which may be orthogonal to the discussion here, but I'd still like to raise it: with a single base path for all controller method, we have a shared namespace for model ids and remote method names.

Consider the case where models are identified by string names in the REST API, for example we may have the following model-instance paths:

GET /api/products/pen
GET /api/products/pencil

In addition to instance-specific paths, we have also remote methods:

GET /api/products/count

Now what happens when an API client creates a resource with id count? We cannot serve both findById('count') and count() methods at the same endpoint.

With two paths (singular/plural), it's easy to create the convention that all remote methods are exposed on the plural path (GET /api/products/count) and the path segment after the singular form is always a resource IDs (GET /api/product/pen or POST /api/order/123/updateTotals).

I don't see how to solve this challenge when there is only a single base path for all resource-related methods. We could require resources to avoid using IDs that match remote method names, but I find that rather impractical, considering the list of reserved IDs can change as the code evolves.

Thoughts?

raymondfeng · 2018-05-17T15:19:32Z

The possibility of conflict between ids and our built-in operation names such as count, findOne is very low. At least we have never received any complaint from LB3 users. Most ids are numbers or uuids.

For the edge case, I see a few options:

Escape our built paths as something like $count.
Make it possible for developers to customize the path of a method. (I think it's already doable)

@bajtos If you are keen on this, please open an issue to track it.

bajtos · 2018-05-18T15:53:08Z

The possibility of conflict between ids and our built-in operation names such as count, findOne is very low. At least we have never received any complaint from LB3 users. Most ids are numbers or uuids.

Yeah, I agree. I guess I just don't like the possibility of a naming conflict that the current solution offers, I prefer to design my code/apps to make such conflicts impossible instead of only low-probability.

Not a big deal, we can wait until we have a real world scenario when this matters.

bajtos · 2018-05-24T07:20:02Z

docs/site/Controller-generator.md

 - 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


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

Yup, I'll open a PR to fix this

shimks requested review from bajtos and raymondfeng as code owners May 9, 2018 21:33

shimks added the needs-priority label May 9, 2018

shimks commented May 9, 2018

View reviewed changes

shimks force-pushed the cli/plural-models branch from 3920f88 to 650aaf0 Compare May 9, 2018 21:39

shimks commented May 9, 2018

View reviewed changes

bajtos suggested changes May 10, 2018

View reviewed changes

bajtos reviewed May 10, 2018

View reviewed changes

bajtos suggested changes May 10, 2018

View reviewed changes

shimks force-pushed the cli/plural-models branch 4 times, most recently from dbe1eec to 2d4b2c6 Compare May 11, 2018 19:06

shimks commented May 11, 2018

View reviewed changes

jannyHou reviewed May 11, 2018

View reviewed changes

shimks force-pushed the cli/plural-models branch 3 times, most recently from 8f8cf80 to 2d7156a Compare May 14, 2018 17:53

bajtos approved these changes May 15, 2018

View reviewed changes

b-admike approved these changes May 15, 2018

View reviewed changes

raymondfeng requested changes May 16, 2018

View reviewed changes

shimks force-pushed the cli/plural-models branch 2 times, most recently from 16b19d4 to 34202ff Compare May 22, 2018 15:48

virkt25 approved these changes May 22, 2018

View reviewed changes

feat(cli): add CLI prompt for controller's http path name

9ff713f

shimks force-pushed the cli/plural-models branch from 34202ff to 9ff713f Compare May 22, 2018 19:06

shimks merged commit 0f9c438 into master May 22, 2018

shimks deleted the cli/plural-models branch May 22, 2018 19:31

bajtos reviewed May 24, 2018

View reviewed changes

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

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

Uh oh!

Conversation

shimks commented May 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shimks left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jannyHou May 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shimks May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shimks May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shimks May 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

shimks commented May 9, 2018 •

edited

Loading

shimks left a comment •

edited

Loading

jannyHou May 10, 2018 •

edited

Loading

shimks May 11, 2018 •

edited

Loading

shimks May 11, 2018 •

edited

Loading

shimks May 15, 2018 •

edited

Loading