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

[SPIKE]Legacy Juggler - Providing repositories based on relations #995

Closed
kjdelisle opened this issue Feb 14, 2018 · 8 comments
Closed

[SPIKE]Legacy Juggler - Providing repositories based on relations #995

kjdelisle opened this issue Feb 14, 2018 · 8 comments
Assignees
Milestone

Comments

@kjdelisle
Copy link
Contributor

kjdelisle commented Feb 14, 2018

Timeboxed to 2 weeks from start

Overview

We need to build support in the legacy juggler bridge to allow usage of the current juggler's relation engine. To do this, we need to answer some questions about what form and format it will take.

Scenario

In this scenario, we want to retrieve the relationship that connects a Customer to their Orders so that we can return a Customer with its Orders embedded in it as a part of the call to getCustomerWithOrders.

Example pseudo-TS:

// customer.repository.ts

export class CustomerRepository extends EntityCrudRepository<Customer, number> {
  // ctor goes here

  getRelations(relationName: string, customer: Customer, ctx?: Context)
    : OrderRepository {
      // find the constructor of the item under relation
      const rel = Customer.definition.relations[relationName];
    
    // use rel.modelTo and find (in the metadata) the reference to the correct
    // repository class
    
    // somehow, we need to find context!

    // Get the Binding<OrderRepository> using one of these approaches:
    const orderRepoBinding = ctx.find('repositories.OrderRepository');
    // OR
    const orderRepoBinding = ctx.findByTag('repositories:Order').first();
    // List the pros v. cons of each as a part of the spike

    // Next, we either grab the typical instance of the OrderRepository via
    // injection and return that directly...
    return orderRepoBinding.getValue(ctx);

    // ...OR we create a constrained instance of the repository using the 
    // initial instance, by wrapping it.
    const orderRepo = orderRepoBinding.getValue(ctx);
    return ConstrainedRepositoryFactory.build(
      orderRepoBinding, rel.constraints);
  }
}

// order.repository.ts
export class OrderRepository extends EntityCrudRepository<Order, number> {
 // this would be the typical implementation
}

// customer.controller.ts
export class CustomerController {
  
  constructor(
    @repository('CustomerRepository') protected customerRepository: CustomerRepository,
  )

  async getCustomerWithOrders(id: number) {
    const customer = await this.customerRepository.findById(id);
    const orderRepo = customer.getRelations('order', customer);
    const orders = orderRepo.find({
      where: {
        createdDate: {
          $gt: new Date('2012-01-01'),
        },
      }
    });
    customer.orders = orders;
    return customer;
  }
}

@model()
export class Customer extends Entity {
  orders: Order
}

Questions to Answer

  • What sort of metadata do we need to create and/or retrieve so that we can find the repository based on the known model type (ex. finding OrderRepository using Order)?
  • What work would be required to add metadata between the repository binding for a particular class and its constructor?
    • Compare this to the tag-based approach and list the pros v. cons
  • What sort of work would be required to create a wrapper factory that replaces a repository instance's functions with constrained versions? (ex. the update function of the wrapped repo doesn't allow editing of the foreign key customerId on orders since that breaks the relationship constraint)
@arash01
Copy link
Contributor

arash01 commented Feb 22, 2018

Can't wait to see this feature in loopback-next..

@dhmlau
Copy link
Member

dhmlau commented Feb 26, 2018

Cross posting from #419

In LB3, we support the following types of model relation:

 -  BelongsTo
 -  HasOne
 -  HasMany
 -  HasManyThrough
 -  HasAndBelongsToMany
 -  Polymorphic
 -  EmbedsOne
 -  EmbedsMany
 -  ReferencesMany

@jannyHou
Copy link
Contributor

jannyHou commented Mar 8, 2018

@dhmlau I think this spike can focus on building the skeleton for the navigation among models(repositories), we can consider it as a factory, any specific relation's details can be discussed in separate stories.

The pseudo code is a great start, a few questions based on it:

  • At run time, we resolve the related repository(orderRepo in this case) by ctx, does the ctx equal to application by default? And it allows to be overridden?

  • IIUC the relation related files in legacy juggler relations.js, relation-definition.js, scope.js are not directly related to this spike, so we can put any logic in those files aside until we implement a certain relation(and if some logic are reusable).

  • In LB3 we dynamically build a relation property for model instance, is it still a good UX we want to keep? From the code above, it seems the new "relation" is essentially a set of controller functions.

  • How to integrate the legacy juggler's DSL(query)? Especially include, which implies a navigation to query related model under the hood. From the pseudo code, seems the only thing we leverage in LB4 is the legacy juggler's CRUD implementation(dao.js).

  • Given the pseudo code, we'd better clarify:

    • which part is the code/logic added in package repository
    • what is the code we generate by cli/template
    • what is the code user need to write

@b-admike
Copy link
Contributor

@raymondfeng I've been trying to create a test app that demonstrates the use case here. I would greatly appreciate some guidance based on @jannyHou's concerns and the following:

What sort of metadata do we need to create and/or retrieve so that we can find the repository based on the known model type (ex. finding OrderRepository using Order)?

Can we leverage RepositoryMetadata to reverse lookup the repo based on the model?

Assuming a Customer model hasMany relation with Order model, would users be expected to write out customerId property for Order or should that be inferred by juggler bridge?

@sbacem
Copy link

sbacem commented Mar 20, 2018

is it possible to rename model relation to

@OneToOne
@OneToMany
@ManyToOne
@ManyToMany

Like in Hibernate, Doctrine or TypeOrm

@b-admike
Copy link
Contributor

@sbacem thank you for your comment. I think it is possible to use that naming convention and I feel it makes it more intuitive and show the relationship from both sides of the field. It might actually make it easier to describe relationships in one shot as opposed to say having two complimentary decorators (for e.g. @hasMany and @belongsTo). On the other hand, I'm sure there are points to be made to continue to use it as the we have it in LoopBack 3.

@b-admike b-admike modified the milestones: March 2018, April 2018 Apr 2, 2018
@dhmlau dhmlau changed the title Legacy Juggler - Providing repositories based on relations [SPIKE]Legacy Juggler - Providing repositories based on relations Apr 6, 2018
@dhmlau dhmlau added the DP3 label Apr 19, 2018
@b-admike
Copy link
Contributor

Sorry for the late update, everyone. You can find some of the prototype code in the spike #1194. We have discussed about the findings and here is my proposed list of tasks to come out of this:

  • Inclusion of related models (same as LB3)
  • Infer target repository when related models are backed by the same datasource, otherwise let user explicitly provide the target repository
  • Use relational decorators' metadata to construct constrained target repository instance on relational properties
  • Scoping for single models: https://loopback.io/doc/en/lb3/Model-definition-JSON-file.html#scopes
  • Remove execute function out of Repository interface and into its own interface for arbitrary SQL commands.
  • Split Repository interface into different interfaces based on the persistence function type i.e. LookupRepository interface to have all the Retrieval methods, WriteRepository (I'm sure there is a better name), would have the create methods, MutationRepository might have the update and related methods (this might fall under the previous one), and DestroyRepository for deletes.
    • Explore the use of a mixin for a smart way of sharing the implementation bits from the different repositories.
      Infer target repository when related models are backed by the same datasource, otherwise let user explicitly provide the target repository
  • 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.
  • incorporating relations in existing examples or creating a new example
  • Which relations should we support?
    • HasMany and BelongsTo for starters

There are notes at https://github.com/strongloop/loopback-next/blob/b4da30ed38b897f8804a655a0a64240cbb0962f5/docs/site/Model-relations.md as well.

From @bajtos,

We need a vision of how to iteratively deliver small increments of functionality, where each increment is something that our users (LB4 app developers) can start using. In our call, we were discussing the first smallest step. IIRC, it was HasMany with a single operation find, covering both repository/persistence and REST API in a controller method. The next step as I remember it: add create method to repository/persistence layer and expose it in REST API as a new controller method. After that, I think we can go wider - one task can be adding all remaining HasMany methods, another task can be BelongsTo. Many of the tasks you listed make sense to do after those two initial tasks are done. Some of them should be probably done as part of those two initial tasks. For example, "remove execute" and "split Repository interface" - I think that should be done in the extend needed by those baby steps.

This sentiment is shared by myself and others on the team, and I'm working on the first increments described above in a new PR.

@dhmlau
Copy link
Member

dhmlau commented May 24, 2018

spike is complete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants