Skip to content
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

[CLI] add lb4 relation command #1359

Closed
16 tasks
dhmlau opened this issue May 24, 2018 · 15 comments
Closed
16 tasks

[CLI] add lb4 relation command #1359

dhmlau opened this issue May 24, 2018 · 15 comments
Assignees
Labels
CLI feature parity Juggler Relations Model relations (has many, etc.)

Comments

@dhmlau
Copy link
Member

dhmlau commented May 24, 2018

Description / Steps to reproduce / Feature proposal

  • CLI command lb4 relation or the likes to wire up related models (think of LB3)
    • Creating CLI templates which expose relation related CRUD methods during controller method generation
    • Adding CLI prompts which create/decorate relational properties etc.

See also #695

LoopBack 3 has models relations definition in JSON file or in JavaScript code
We would like to define models relations in LoopBack 4 as well. Definition of relations need to auto generate the api services.

cc @b-admike


Acceptance Criteria (@virkt25)

  • Add a new lb4 relation command
  • Prompt the user to select relation type (current support is: hasMany, belongsTo in progress).
  • For hasMany ask the following prompts:
    • Model to create a relationship FROM: Provide list of available models.
    • Model to create a relationship TO: Provide list of available models (minus the one selected in the above).
    • Property name for the relation: Recommend the plural form of the TO model.
    • Enter name of custom foreign key (optional).
  • Prompt the user to see if they want to create / update a Repository (see [CLI] add lb4 repository command #1588 for prompts you should ask / need to generate a repository)
  • Prompt the user to see if they want to create a controller to expose the relation over API
  • Add the new property to the FROM model that is decorated with the hasMany decorator. Will need to import the TO model here.
  • If a custom foreign key is defined, check to see if it exists in the TO model. If not, add an optional property.
  • If the want to create / update repository: Check if a related repository exists. If not, create one which has the HasManyRepositoryFactory and it's implementation. If it exists, use AST to add the new property and code related to power the HasMany relation. See: https://github.com/strongloop/loopback-next/blob/master/examples/todo-list/src/repositories/todo-list.repository.ts for details.
  • If a new controller is wanted, create a new controller fromModel-toModel.controller.ts that exposes GET, POST, PATCH, DEL. Template can be based on https://github.com/strongloop/loopback-next/blob/master/examples/todo-list/src/controllers/todo-list-todo.controller.ts.
  • Tests + Docs + --config support (JSON Definition of Model) and --yes support (Skip relation property name, foreign key, assume they want to create/update repo, create controller).
  • Should also support the following CLI Syntax: lb4 relation fromModel hasMany toModel ... with --yes this is 🔥
  • Blog

Note For Estimation
CHALLENGE: Updating a pre-defined Model is tricky if we are manipulating the file as text ... user could've defined multiple Classes in a single file, etc. The best approach will be to use Abstract Syntax Tree (AST).

@bajtos
Copy link
Member

bajtos commented Aug 13, 2018

As for the controller generator, our current approach is to have one controller for each relation. For example, when Customer has many orders, there will be three controllers: CustomerController, OrderController and CustomerOrderController.

The reason for this split is to keep application code clean and avoid controllers with too many methods. Consider the case where a model has 3-5 relations. If we kept all relation endpoints in a single controller, the controller could easily end up with more than 20 methods.

As a nice side effect, our code generator will be also simpler to implement because adding a relation means adding a new controller file instead of editing existing controller classes.

@virkt25 virkt25 changed the title [CLI] Relations [CLI] lb4 relation command Aug 21, 2018
@virkt25 virkt25 changed the title [CLI] lb4 relation command [CLI] add lb4 relation command Aug 21, 2018
@virkt25
Copy link
Contributor

virkt25 commented Aug 28, 2018

Rejecting for now. While the task is well groomed, we need consensus on if we are going with AST Approach, Declarative Support, etc. Also need a spike on AST to figure out feasibility and build expertise. Also, this task should be broken down into smaller tasks.

@raymondfeng
Copy link
Contributor

We can possibly leverage prettier for TypeScript source code manipulation. I built a quick PoC as follows:

const prettier = require('prettier');
const util = require('util');

async function transform() {
  const prog = await prettier.format(
    `/** class */
    class X {
      a: string;
      b(): string {
        return '1';
      }
    }`,
    {
      parser(text, {typescript}) {
        const ast = typescript(text);
        console.log(util.inspect(ast, {depth: 5}));
        ast.body[0].id.name = 'Y';
        return ast;
      },
    },
  );
  console.log(prog);
}

transform();

You'll now get:

/** class */
class Y {
  a: string;
  b(): string {
    return "1";
  }
}

@marioestradarosa
Copy link
Contributor

@raymondfeng
Copy link
Contributor

@marioestradarosa Thank you for chiming in. The ones you found seem to be only for JavaScript, not TypeScript.

@marioestradarosa
Copy link
Contributor

@raymondfeng there are ports for TS also https://github.com/phenomnomnominal/tsquery

I want to learn from you :-) , why the current typescript (with a linter) would not be suitable for this purpose? or any other package like tsquery as opposed to the excellent POC you mentioned above with prettier?.

@jannyHou
Copy link
Contributor

jannyHou commented Sep 6, 2018

As we discussed in the estimation meeting, this will be a post-ga story and let's create a new issue to document how to update the relation repository as a temporary solution.

@jannyHou jannyHou added post-GA and removed LB4 GA labels Sep 6, 2018
@dhmlau dhmlau removed the p2 label Sep 6, 2018
@marioestradarosa
Copy link
Contributor

marioestradarosa commented Sep 23, 2018

I think that we should have this done in three separate Pull Requests.

I propose the initial PR as follows

  • Add lb4 relation that asks for the following 3 prompts.
    - select first model
    • select relationship type
      • show the current supported relations
    • select second model (here we can display the relationship type to guide the user)
      • Notice that we hint the user in the second list by displaying the plural form of the model based on the relationship type selected

Example:
Select the first Model:
Order
Item
Customer <
Select a relationship type
Belongs To
HasMany <
Select the second Model or we can display something like this: Customers has many ?
Orders <
items

The generator will update the two models in src/models and add the necessary sintax to support it.

Advanced functionality using AST

  • It will also have to infer the ID property from the model(s) and if it can't infer one, then it should ask the user for it.
  • It will check if the relationship is already present in the src/models file to not overwrite it.

Note: The other two PRs proposed should be done in the lb4 repository to support the relationship and in the lb4 controller to generate the appropriate controllers to support the defined relationship.

@dhmlau
Copy link
Member Author

dhmlau commented Oct 27, 2018

Per the discussion in our previous estimation meeting, this task would involve changing of the ts file. Therefore, we've created the spike #1656 to see what's the best approach. With this, I'm moving this task back to Needs Priority.

@orshlom
Copy link

orshlom commented Dec 16, 2018

Hi @dhmlau,
I did some research on creating additional relations for models and repositories using TypeScript AST manipulation, following #1656 spike, and made a comment with my findings.

I am more than confident I can implement the CLI command for relations following your guidelines.
I would be happy to be contribute and be assigned on this issue.

@orshlom
Copy link

orshlom commented Dec 27, 2018

@dhmlau
If a user wants to specify in the FROM model a property other than its ID property (keyFrom attribute), should it be supported in the CLI as well?

@bajtos
Copy link
Member

bajtos commented Jan 7, 2019

I am more than confident I can implement the CLI command for relations following your guidelines.
I would be happy to be contribute and be assigned on this issue.

Sounds great, assigning the issue to you :)

If a user wants to specify in the FROM model a property other than its ID property (keyFrom attribute), should it be supported in the CLI as well?

Personally, I prefer to work in small increments and start with the simplest possible solution. I think that means no user-configurable FROM/keyFrom property, what do you think?

For longer term, I think it makes sense to eventually extend lb4 relation command to allow callers to customize all aspects of the relation to be created, including a custom keyFrom setting.

@TomerSalton
Copy link

Thanks for assigning me into this task :)
I have a question to have a better understanding of the relationships design.

From the documentation and from some tests I performed in my environment I realized:

  1. hasMany doesn't require belongsTo relation on the target model.
  2. hasOne requires belongsTo relation on the target model.
  3. belongsTo doesn't require hasMany/hasOne on the target model.

I guess my question is why hasMany is different from hasOne in the aspect of requiring the belongsTo in the target model.

@elv1s
Copy link
Contributor

elv1s commented Jan 29, 2019

What is special about belongsTo that it is a requirement of hasOne? I'm pretty sure that wasn't a requirement in LB3.

@bajtos
Copy link
Member

bajtos commented May 24, 2019

Closing as done via #2426

@bajtos bajtos closed this as completed May 24, 2019
@bajtos bajtos added this to the May 2019 milestone milestone May 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI feature parity Juggler Relations Model relations (has many, etc.)
Projects
None yet
Development

No branches or pull requests

9 participants