loopbackio / loopback-next

LoopBack makes it easy to build modern API applications that require complex integrations.
https://loopback.io
Other
4.95k stars 1.07k forks source link

hasMany relation to same Entity fails #1571

Closed jdumont0201 closed 5 years ago

jdumont0201 commented 6 years ago

Description / Steps to reproduce / Feature proposal

Can we have an hasMany relation between objects of the same Entity ? Say an object that refers to other objects of the same type.

Apparently it cannot find the target key, or loops endlessly when specified explicitly.

Current Behavior

Start from the TodoList example. Add an hasMany relation to Todo that links to other Todos. todo.repository.ts

import {DefaultCrudRepository,HasManyRepositoryFactory, juggler} from '@loopback/repository';
import {Todo} from '../models';
import {inject} from '@loopback/core';

export class TodoRepository extends DefaultCrudRepository<Todo,typeof Todo.prototype.id> {
  //EDITED HERE
  public todos: HasManyRepositoryFactory<Todo, typeof Todo.prototype.id>;
  constructor(
    @inject('datasources.db') protected datasource: juggler.DataSource,
  ) {
    super(Todo, datasource);
    //EDITED HERE
    this.todos = this._createHasManyRepositoryFactoryFor(
      'todos',
      this
    );
  }
}

todo.model.ts

import {Entity, property,hasMany, model} from '@loopback/repository';

@model()
export class Todo extends Entity {
    //EDITED HERE
  @hasMany(Todo) todos: Todo[];

  @property({
    type: 'number',
    id: true,
  })
  id?: number;

  @property({
    type: 'string',
    required: true,
  })
  title: string;

  @property({
    type: 'string',
  })
  desc?: string;

  @property({
    type: 'boolean',
  })
  isComplete: boolean;

  @property() todoListId: number;

  getId() {
    return this.id;
  }

  constructor(data?: Partial<Todo>) {
    super(data);
  }
}

fails with

Cannot start the application. Error: foreign key todoId not found on Todo model's juggler definition
    at /home/jbmdu/dev/politron/test/loopback4-example-todo-list/node_modules/@loopback/repository/dist8/src/decorators/relation.decorator.js:77:19
    at DecorateProperty (/home/jbmdu/dev/politron/test/loopback4-example-todo-list/node_modules/reflect-metadata/Reflect.js:553:33)
    at Object.decorate (/home/jbmdu/dev/politron/test/loopback4-example-todo-list/node_modules/reflect-metadata/Reflect.js:123:24)
    at __decorate (/home/jbmdu/dev/politron/test/loopback4-example-todo-list/dist8/src/models/todo.model.js:8:92)
    at Object.<anonymous> (/home/jbmdu/dev/politron/test/loopback4-example-todo-list/dist8/src/models/todo.model.js:25:1)
    at Module._compile (module.js:652:30)
    at Object.Module._extensions..js (module.js:663:10)
    at Module.load (module.js:565:32)
    at tryModuleLoad (module.js:505:12)
    at Function.Module._load (module.js:497:3)
    at Module.require (module.js:596:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/home/jbmdu/dev/politron/test/loopback4-example-todo-list/dist8/src/models/index.js:10:10)
    at Module._compile (module.js:652:30)
    at Object.Module._extensions..js (module.js:663:10)
    at Module.load (module.js:565:32)

If I specify the keyTo target in todo.model.ts, then it fails with max call stack exceeded.

export class Todo extends Entity {
  @hasMany(Todo,{keyTo:'parentId'}) childs: Todo[];
  @property({ type: 'number'  }) parentId?: number;
  //  ...
Cannot start the application. RangeError: Maximum call stack size exceeded
    at metaToJsonProperty (/home/jbmdu/dev/politron/test/loopback4-example-todo-list/node_modules/@loopback/repository-json-schema/dist8/src/build-schema.js:77:21)
    at modelToJsonSchema (/home/jbmdu/dev/politron/test/loopback4-example-todo-list/node_modules/@loopback/repository-json-schema/dist8/src/build-schema.js:117:32)
    at getJsonSchema (/home/jbmdu/dev/politron/test/loopback4-example-todo-list/node_modules/@loopback/repository-json-schema/dist8/src/build-schema.js:22:27)
    at modelToJsonSchema (/home/jbmdu/dev/politron/test/loopback4-example-todo-list/node_modules/@loopback/repository-json-schema/dist8/src/build-schema.js:120:32)

Expected Behavior

See Reporting Issues for more tips on writing good issues

Acceptance Criteria

virkt25 commented 6 years ago

@jdumont0201 This is an interesting problem. What is the particular use case you are trying to solve here? I would expect an error when trying to do this because you are essentially defining a recursive relation here ... Todo -> Todo 1. Todo 1 -> Todo 2. Todo 1 -> Todo 3. Todo 2 -> Todo 4. And now the final chain should be as follows:

Todo -> Todo 1 -> Todo 2 -> Todo 4
                          |-> Todo 3

This can keep going on forever and ever. My suggestion would be to try a different data model where a Model is extended to create new Models.

Define Todo Model ... Now create 2 new Models as follows:

class ParentTodo extends Todo {
  constructor(data?: Partial<Todo>) { super(data); }
}

class ChildTodo extends Todo {
  constructor(data?: Partial<Todo>) { super(data); }
}

And now you can establish a relation of ParentTodo hasMany ChildTodo and this shouldn't result in an infinite recursion / stack errors all the while both these Models will rely on the same base model so any changes made to one will reflect to the other.

Thoughts?

jdumont0201 commented 6 years ago

There are many models that have fractal properties, i.e. contains entities that have the same properties.

Some usecases: Describe a geographical area that contains smaller area with same properties, itself included in a larger one. Relations are

In Loopback 3, I went with hasManyThough, specifying foreignKey and keyThrough for each pair of relation and two belongsTo relations on the through entity. It allows multiple /contained_in /containing relationships

I also like your inheritence idea, I'll give it a try and keep you informed I will retry on Lookback4 when hasManyThrough is supported. Cheers.

virkt25 commented 6 years ago

Thanks for providing a use-case! Let me know how the inheritance idea works out / any issues you run into it, we'll be more than happy to help.

As for support for hasManyThrough, we're working on adding more relations to LoopBack 4 (Ex: this month we'll get belongsTo). While I don't have a time for hasManyThrough, having a user ask for it helps us prioritize the relation / feature. Thanks for the feedback! :)

shimks commented 6 years ago

Here is the problem I see purely based on the error stack trace given: Using the decorators from @loopback/openapi-v3 package will queue up creating JSON definitions for custom models they are decorating. Due to how they're being written, the module will attempt to create definition for Todo infinitely in the case when it is defined recursively.

Even if we fix this issue, I am not sure whether that would still allow the program to work as intended as @virkt25's comment above. IMO, we need to investigate this issue by:

I'd imagine some language problem we have with JS would be solved by this PR, #1618, via the usage of resolvers, but I'm not too sure.

bajtos commented 6 years ago

In my opinion, a recursive hasMany relation is perfectly valid.

Consider an application modeling company employees. Each manager is an Employee and supervises one or more other employees (Employee has many Employee). It may be easier to understand when we look at the relation from the other side - an Employee "belongs to" another Employee via manager relation. In the data schema of Employee model, each employee has managerId property acting as a foreign key. (The question is how to deal with the top manager that does not report to any other employee. I am not sure if foreign keys can be optional?)

I like the plan proposed by @shimks in his comment 👍

  • creating test cases and fixing the infinite recursion problem for @loopback/repository-json-schema
  • creating test cases and investigating recursive use of @hasMany decorators

@jdumont0201 would you like to take a look at this yourself and contribute a patch or two to fix the issues?

I think the following test case is a good starting point - just copy the code into a new test case, modify the model definition to contain a property storing an instance of the same model, and then find out how to make the test pass.

https://github.com/strongloop/loopback-next/blob/ec37f2fe90f9cda7802d6adcd9632ce8d1516c59/packages/repository-json-schema/test/integration/build-schema.integration.ts#L285-L315

@shimks please correct my advice if needed.

jannyHou commented 6 years ago

let's do a quick spike:

dhmlau commented 6 years ago

Discussed with @raymondfeng , this task will be postGA but would like the spike to be in for GA. @jannyHou , after the spike task is created, could you please mark it as LB4 GA and put it under the LB4 GA release? Thanks.

jannyHou commented 6 years ago

@dhmlau The spike story is created in https://github.com/strongloop/loopback-next/issues/1682

hvlawren commented 5 years ago

There's a similar issue in BelongsTo. I have a model called Category with a parentId field that points to the parent Category

export class Category extends Entity {
  @property({
    type: 'number',
    id: true,
    generated: true
  })
  id?: number;

  @belongsTo(() => Category)
  parentId: number;

  @belongsTo(() => Domain)
  containerId: number;

  @property({
    type: 'boolean',
    default: false,
  })
  internal?: boolean;

  @property({
    type: 'string',
    required: true,
  })
  title: string;

  @property({
    type: 'string',
  })
  description?: string;

  @property({
    type: 'string',
  })
  state?: string;

  constructor(data?: Partial<Category>) {
    super(data);
  }
}

I created a /categories/{id}/parent endpoint per the docs

When I call that endpoint I get this error:

Unhandled error in GET /categories/2/parent: 500 Error: Circular dependency detected: controllers.CategoryParentController --> @CategoryParentController.constructor[0] --> repositories.CategoryRepository --> @CategoryRepository.constructor[2] --> repositories.CategoryRepository

Let me know if that's tied into this issue or if a separate issue should be opened for this one.

bajtos commented 5 years ago

@hvlawren could you please post the dependency injection configuration for your CategoryParentController and CategoryRepository?

bajtos commented 5 years ago

@hvlawren actually, the problem you have encountered is almost certainly different from what is being discussed in this issue, would you mind opening a new GitHub issue please?

hvlawren commented 5 years ago

I created https://github.com/strongloop/loopback-next/issues/2118 -- Thanks!

dhmlau commented 5 years ago

@nabdelgadir , could you please add the acceptance criteria based on the spike #1682? Thanks.