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

hasMany relation to same Entity fails #1571

Closed
1 task
jdumont0201 opened this issue Jul 28, 2018 · 13 comments
Closed
1 task

hasMany relation to same Entity fails #1571

jdumont0201 opened this issue Jul 28, 2018 · 13 comments
Assignees
Labels
bug feature needs grooming Relations Model relations (has many, etc.)

Comments

@jdumont0201
Copy link

jdumont0201 commented Jul 28, 2018

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
Copy link
Contributor

virkt25 commented Aug 21, 2018

@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?

@virkt25 virkt25 self-assigned this Aug 21, 2018
@jdumont0201
Copy link
Author

jdumont0201 commented Aug 21, 2018

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

  • contained_in / containing
  • governed_by / governing.

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
Copy link
Contributor

virkt25 commented Aug 21, 2018

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
Copy link
Contributor

shimks commented Aug 21, 2018

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:

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

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.

@shimks shimks added the bug label Aug 21, 2018
@bajtos
Copy link
Member

bajtos commented Aug 23, 2018

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
Copy link
Contributor

jannyHou commented Sep 6, 2018

let's do a quick spike:

  • create a test case for this scenario
  • troubleshoot why it fails, discover the gap.
    timebox it to 2 days.

@dhmlau
Copy link
Member

dhmlau commented Sep 6, 2018

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
Copy link
Contributor

jannyHou commented Sep 7, 2018

@dhmlau The spike story is created in #1682

@dhmlau dhmlau removed the post-GA label Nov 2, 2018
@dhmlau dhmlau added the Relations Model relations (has many, etc.) label Nov 28, 2018
@hvlawren
Copy link

hvlawren commented Dec 4, 2018

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
Copy link
Member

bajtos commented Dec 4, 2018

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

@bajtos
Copy link
Member

bajtos commented Dec 4, 2018

@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
Copy link

hvlawren commented Dec 4, 2018

I created #2118 -- Thanks!

@dhmlau
Copy link
Member

dhmlau commented Mar 1, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug feature needs grooming Relations Model relations (has many, etc.)
Projects
None yet
Development

No branches or pull requests

8 participants