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

Include related models in "GET /mymodels" results #1889

Closed
2 tasks
GuillaumeJasmin opened this issue Oct 20, 2018 · 16 comments
Closed
2 tasks

Include related models in "GET /mymodels" results #1889

GuillaumeJasmin opened this issue Oct 20, 2018 · 16 comments
Labels
feature parity feature Relations Model relations (has many, etc.) Repository Issues related to @loopback/repository package

Comments

@GuillaumeJasmin
Copy link

GuillaumeJasmin commented Oct 20, 2018

Description

Hi,
I start a big migration since Loopback 4 is now the current release.
However, there is many things not documented.

I try to use include filter, but it seems doesn't work. I'm not able to use it on my current project. I followed examples on the docs.

I first thought that my configuration was bad, so I tried it on loopback4-example-todo-list, but it doesn't work on the example too.

Current Behavior

When I try this request:

http://localhost:3000/todo-lists?filter={"include": [{"relation": "todos"}]}

or this

http://localhost:3000/todo-lists?filter[include]=todos

I get this 500 error:

500 Error: Relation "todos" is not defined for TodoList model

Expected Behavior

Should works like Loopback 3

How I can work with relation and the include filter on Loopback 4 ?

Thanks :)

Acceptance Criteria

  • All relations are supported (HasMany, BelongsTo, HasOne, etc.)
  • When passing the parameter name, quotes are not required to wrap around the param name. i.e.
    It should be:
http://localhost:3000/todo-lists?filter={"include": [{"relation": "todos"}]}

not:

http://localhost:3000/todo-lists?"filter"={"include": [{"relation": "todos"}]}
@dhmlau
Copy link
Member

dhmlau commented Oct 22, 2018

@GuillaumeJasmin, it seems to be working with:

http://localhost:3000/todo-lists?"filter"={"include": [{"relation": "todos"}]}

not (please note the quotes around the param name):

http://localhost:3000/todo-lists?filter={"include": [{"relation": "todos"}]}

Then it comes to the question that whether the param name should be wrapped around the quotes. I'll get back to you on that. Hope it will get you going first.

Another way to test (which I usually do) is to use the API Explorer, which can be found at:

http://localhost:3000/explorer

@dhmlau dhmlau self-assigned this Oct 22, 2018
@dhmlau dhmlau removed their assignment Oct 22, 2018
@dhmlau
Copy link
Member

dhmlau commented Oct 22, 2018

@GuillaumeJasmin, talked to @raymondfeng and it looked like a bug that the quotes are needed for param name. Marking it as bug.

@mrbatista
Copy link
Contributor

mrbatista commented Oct 25, 2018

@GuillaumeJasmin the include functionality is not yet implemented.

http://localhost:3000/todo-lists?"filter"={"include":[{"relation":"todos"}]}

The expect behavior is to return for each todo lists the list of todos, but nothing is returned.
This seems a lack of functionality.

@mrbatista
Copy link
Contributor

mrbatista commented Oct 25, 2018

@GuillaumeJasmin see #1352 #1721

@GuillaumeJasmin
Copy link
Author

GuillaumeJasmin commented Oct 25, 2018

@dhmlau Thanks for your answer, however this doesn't work, and as @mrbatista say, it seems not implemented.

I don't understand the last release of Loopback.
Loopback 4 was released as the "current" release. So, we should be able to use LB4 in replacement of LB3, with at least the same features (after some hours of migration, of course, maybe days if the application is big).
But this is not the case.
I loose time to understand and try to use Loopback 4, but I can't think that the release 4 is the stable version.
I love Loopback because it's easy to use, and we have no need to write many code to build a powerful back-end service

But it seems that Loopback 4 is more complicated and not ready for production. I understand the vision of LB4 to have a small core system, and the rest as extension. It's seems powerful. But the migration between LB3 and LB4 is too complicated. For example, why do not provide a set of extensions in order to keep the simplicity of LB3, without the needed to create repository, controller for default CRUD actions (only if we need custom actions) ?

I dropped my last back-end frameworks for Loopback since few years with a single motivation: simplicity. But it seems not to be the case with LB4. It's maybe more extensible, but need hard work, and the simplicity is not still here.

I know it's not the best place to tell that, but this issue is one more lack of functionality on LB4 compare to LB3. I have to stop trying to work on LB4.

Maybe I forget a thing, or I don't understand the future of Loopback ?

@bajtos bajtos added feature and removed bug labels Oct 25, 2018
@bajtos
Copy link
Member

bajtos commented Oct 25, 2018

This is a missing feature indeed, I am relabeling the issue.

@GuillaumeJasmin thank you for writing down your concerns and feelings about LoopBack 4. We are fully aware of the gap in feature parity between LoopBack 3 and LoopBack 4 and that LB4 is not a drop-in replacement for LB3 in many cases. On the other hand, if we were waiting until LB4 reached functional parity with LB3, then it would take few more years to reach the GA release, considering the size of our team and the scope of LB3 features. To avoid that situation, we decided to cut down the scope, release 4.0 GA early and start gathering feedback about which features are most important to our users. That allows us to better prioritize our efforts on areas where we can add most value.

I'll try to find some time later to respond to other parts of your comment in more details, maybe in a blog post or a doc page.

@bajtos bajtos changed the title filter "relation" doesn't work Include related models in "GET /mymodels" results Oct 25, 2018
@bajtos bajtos added Relations Model relations (has many, etc.) Repository Issues related to @loopback/repository package labels Oct 25, 2018
@GuillaumeJasmin
Copy link
Author

GuillaumeJasmin commented Oct 25, 2018

Thank you @bajtos for this clarification comment about LB4. Maybe the release name "current" is confusing ? Usually, a stable version of software doesn't remove a powerful feature included into the previous release.
If you release 4.0 in order to get some feedback to prioritize, and you know that the 4.0 release doesn't currently include all nice features of 3.0, it's not a stable version (except if the feature is willingly removed). I think this situation is the same than beta version: a way to share it to the end users, but with the warning that it's not stable.

I think you shouldn't communicate about a stable version, because users thinks "ok, I can now use it on production like the previous version, with some days of migration". I think this could create many frustration for the end developers.
Maybe I'm the only one frustrated by that, so my point of view is not important, but I think others have come up against the same issues when they try a migration.

I think it's just a communication issue around "current" and "beta".

However, thanks for your work on Loopback, it's a great tool, and I hope (and believe) LB4 will be as simply as LB3 after legacy features finished and some useful extensions that reproduce the default behavior of LB3 (I would like to contribute and develop theses extensions, but I need time to completely understand how write good extensions on LB4)

@GuillaumeJasmin
Copy link
Author

API explorer is confusing because it show an include filter, even if the feature is missing.

screen shot 2018-10-25 at 16 40 39

@dhmlau I can't use API explorer because it doesn't work with nested object:
look the filter field on the screenshot, and then the request URL: the filter is not sent. It seems related to swagger-api/swagger-js#1385

screen shot 2018-10-25 at 16 42 39

@dhmlau
Copy link
Member

dhmlau commented Oct 25, 2018

@GuillaumeJasmin, thanks for letting me know. Yes, the UI has limitation to allow nested properties. We've captured this in issue #1470 as well.

@bajtos
Copy link
Member

bajtos commented Oct 26, 2018

FWIW, the actual work on supporting inclusion of related models is tracked by the following two issues: #1352 and #1721

@franfran3
Copy link

Looks like escaping the parameter name "filter" does not solve the problem as it ignore filters:

http://localhost:3000/todo-lists?"filter"={"where": {"id":4}, "include":[{"relation":"todos"}]}

does not apply where neither include.

@bajtos
Copy link
Member

bajtos commented Oct 26, 2018

Thank you @bajtos for this clarification comment about LB4. Maybe the release name "current" is confusing ?
I think it's just a communication issue around "current" and "beta".

I agree with you, we need to find a way how to better communicate the status of LoopBack 4 and set reasonable expectations.

For example, why do not provide a set of extensions in order to keep the simplicity of LB3, without the needed to create repository, controller for default CRUD actions (only if we need custom actions) ?

Yes please! It's our design goal to keep LoopBack 4+ extensible and allow the community to experiment with different styles of project layout. We don't have bandwidth to implement and maintain all such extensions ourselves, but we are happy to help anybody willing to step up and write such extension themselves.

As for repositories, it's already possible to use DefaultCrudRepository without creating per-model repository classes.

In #740, @raymondfeng has proposed an extension providing REST API via a base controller class CrudController that provides usual querying and CRUD operations, similar to what the controllers scaffolded by lb4 controller do (but without the scaffolding step).

With these two buildings blocks, I imagine one can write a small piece of code accepting a model class an exposing it via REST API, see the mock-up implementation below. It's just an illustration to show what I mean, it will probably not work out of the box and may require cleanup.

// usage - this can be automated via a custom Booter
import {Product, Category} from './models';

export class MyApplication extends RepositoryMixin(RestApplication) {
 constructor() {
    super();  
    exposeModelViaRest(app, Product);
    exposeModelViaRest(app, Category;
  }
}

// implementation
function exposeModelViaRest<T extends Entity>(
  app: ApplicationWithRepositories,
  dataSourceName: string, 
  modelCtor: Class<T>
) {
  const REPOSITORY_KEY = `repositories.${modelCtor.modelName}Repository`;
  app.bind(REPOSITORY_KEY).toDynamicValue(() => {
    const ds = await ctx.get(`datasources.${dataSourceName}`);
    return new DefaultCrudRepository(modelCtor, ds);
  });

  app.controller(createCrudControllerForModel(modelCtor, REPOSITORY_KEY));
}

function createCrudControllerForModel<T extends Entity>(
  modelCtor: class<T>,
  repositoryKey: string
) {
  // It is not possible to access closure variables like "BaseController"
  // from a dynamically defined function. The solution is to
  // create a dynamically defined factory function that accepts
  // closure variables as arguments.
  const name = modelCtor.modelName + 'Controller';
  const factory = new Function('BaseController', 'repositoryName', `
    class ${name} extends BaseController {
      constructor(repository) {
        super(repository);
      }
    }`);

  const controllerCtor = factory(CrudController, repositoryName);
  // apply @inject() decorator on the first constructor parameter
  inject(repositoryKey)(controllerCtor, undefined, 0);
  return controllerCtor;
}        

I am pretty sure there are other ways how auto-wire REST API for models, let's experiment!

@bajtos
Copy link
Member

bajtos commented Oct 30, 2018

For example, why do not provide a set of extensions in order to keep the simplicity of LB3, without the needed to create repository, controller for default CRUD actions (only if we need custom actions) ?

It turns out we already have stories to address this use case, see Declarative Support #565 and Define model relations in JSON file or in JavaScript code #695

@bajtos
Copy link
Member

bajtos commented Nov 2, 2018

@dhmlau I think we should close this issue as a duplicate of #1352 and copy any relevant information over to the other issue. Thoughts?

@nabdelgadir nabdelgadir added this to the November 2018 Milestone milestone Nov 2, 2018
@bajtos
Copy link
Member

bajtos commented Nov 6, 2018

I have posted a comment in the other issue with a proposal how to implement inclusion of related models at Repository level, see #1352 (comment)

@bajtos
Copy link
Member

bajtos commented Feb 12, 2019

Closing this issue as a duplicate of

See also:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature parity feature Relations Model relations (has many, etc.) Repository Issues related to @loopback/repository package
Projects
None yet
Development

No branches or pull requests

6 participants