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

Create related model in hasMany relation inconsistency #1944

Closed
israelglar opened this issue Oct 30, 2018 · 8 comments · Fixed by #2178
Closed

Create related model in hasMany relation inconsistency #1944

israelglar opened this issue Oct 30, 2018 · 8 comments · Fixed by #2178
Labels
bug good first issue Relations Model relations (has many, etc.)

Comments

@israelglar
Copy link

israelglar commented Oct 30, 2018

Description / Steps to reproduce / Feature proposal

I have 2 Models: Profile and Team. The relation is Team hasMany Profile.

I have a controller for that relation TeamProfilesController

export class TeamProfilesController {
  constructor(
    @repository(TeamRepository) protected teamRepository: TeamRepository,
  ) {}

  @post('/teams/{id}/members')
  async createMember(
    @param.path.string('id') teamId: typeof Team.prototype.id,
    @requestBody() profileData: Profile,
  ): Promise<Profile> {
    return await this.teamRepository.members(teamId).create(profileData);
  }

  @get('/teams/{id}/members')
  async getMember(
    @param.path.string('id') teamId: typeof Team.prototype.id,
    @param.query.object('filter', getFilterSchemaFor(Profile)) filter?: Filter,
  ): Promise<Profile[]> {
    return await this.teamRepository.members(teamId).find(filter);
  }
}

If I already have a team created and use POST /teams/{id}/members, a new profile is created 👍

Current Behavior

The problem is that on my team repository the POST /teams receives:

{
  "id": "string",
  "name": "string",
  "type": "string",
  "members": [
    {
      "id": "string",
      "username": "string",
      "password": "string",
      "name": "string",
      "email": "string"
    }
  ]
}

The repository accepts members as valid property and if I create a team with the property members set, what ends up in my Database is:

{
    "_id" : ObjectId("5bd83d9ae8abbe501d4b68d0"),
    "name" : "testTeam",
    "members" : [ 
        {
            "id" : "should run function Profilerepository.create that creates a new ObjectId",
            "username" : "test user",
            "password" : "test pass",
            "name" : "test name",
            "email" : "[email protected]"
        }
    ]
}

No new Profile is created. What is created is a Team that has property members embedded in it. And the objects on that array don't qualify as Profile.
When I try to GET /teams/5bd83d9ae8abbe501d4b68d0/members is returns empty array.

Expected Behavior

When I create a team with the property members I Expected 1 of 2.
1. One Team and one profile is created.

{
    "_id" : ObjectId("5bd83d9ae8abbe501d4b68d0"),
    "name" : "testTeam"
}
{
  "id" : "should run function Profilerepository.create that creates a new ObjectId",
  "username" : "test user",
  "password" : "test pass",
  "name" : "test name",
  "email" : "[email protected]"
}

2. Like LB3 to not be possible to create a Team with the members property on the POST /teams. And if the property is sent, so get caught by the model as an invalid property because it is a "strict" model.

@dhmlau
Copy link
Member

dhmlau commented Oct 30, 2018

@b-admike , could you please take a look? Thanks.

@dhmlau dhmlau added the Relations Model relations (has many, etc.) label Oct 30, 2018
@bajtos
Copy link
Member

bajtos commented Nov 2, 2018

Thank you @israelglar for reporting this issue. It looks like a bug in our current implementation to me, possibly specific to MongoDB connector.

We don't support embedded relations in LB4 yet. Even though the Team model has a property to carry related Profile members, the data is supposed to be stored in two different collections (Team collection and Profile collection).

An attempt to create a Team with embedded members should fail with a descriptive error (option 2 from your expectations).

IIRC, when we decorate a property with @hasMany, we also mark it as @property.array. I think that may be the root cause, because then juggler thinks that we have a property embedding the related models.

We can try to change @hasMany decorator to NOT decorate the model property. That should fix the creation part - because our models are strict, a payload sending members property should be rejected because the model does not have any members property defined.

I am not entirely sure about ramifications of this change. For example, I think the JSON/OpenAPI schema generated for Team model will no longer include teams property. Which is correct for endpoints (operations) editing the data, but may not be desirable for endpoints (operations) retrieving the data once we implement support for inclusion of related models - see #1352 and #1889. I guess we can worry about the impacts on inclusion of related models later when we actually implement it.

So, I think this issue may be relatively easy to fix at the end:

  • Write an acceptance or integration test that will send a POST request to create a model with related models include and verify that the request is rejected with a validation error.
  • Modify @hasMany decorator - remove the call of @property.array(). Some tests should start failing after this change, fix them to make different assumptions.

@israelglar would you like to contribute this patch yourself?

@SharonItz
Copy link
Contributor

Hi Bajtos,
It's look like we have 2 issues here, module validation and @hasmany decorator.
Do we need to separate them?
Also, can you explain what is the expected behavior when modifying the @hasmany decorator?
we still need to have a property on the source model that represents the has many relations with the target model.

thx
Sharon

@bajtos
Copy link
Member

bajtos commented Nov 27, 2018

It's look like we have 2 issues here, module validation and @hasmany decorator.
Do we need to separate them?

To me, this looks like a single issue. I think I don't fully understand what you are trying to say?

Also, can you explain what is the expected behavior when modifying the @hasmany decorator?
we still need to have a property on the source model that represents the has many relations with the target model.

The behavior I am expecting from the modified @hasMany decorator:

  • it sets relation metadata so that it can be picked up by DefaultCrudRepository.prototype._createHasManyRepositoryFactoryFor
  • it does not mark the underlying TypeScript property as a model property (does not call @property.array decorator)

@ludohenin
Copy link
Contributor

@bajtos I have a use case where it does make sense to me to have the decorated property be part of the model. I'm actually POC-ing with LB4 and try to implement some kind of translatable property support where my models looks like:

Base Model

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

  @hasMany(() => MyModelTranslation)
  translations: MyModelTranslation[];

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

Translation Model

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

  @property({
    type: 'number',
    required: true,
  })
  myModelId: number;

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

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

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

In my case the url form model/{id}/translation doesn't fit very well. But maybe I'm just wrong and this is part of the embedded relations (btw any ETA on that feature ?), and in embeded scenario, the property has to be there anyway, right ?

@SharonItz
Copy link
Contributor

Hi @bajtos
#1944

I understand your comment and I will happy to start working on this issue.
I already remove the call off @property.array() and related module POST was rejected.
Also, I saw some failed tests that need to be fixed.

Thx
Sharon

@AnanthGopal
Copy link

AnanthGopal commented Apr 17, 2019

I try to get data like below (Parent and child data).

[{
    "_id" : ObjectId("5bd83d9ae8abbe501d4b68d0"),
    "name" : "testTeam",
    "todos" : [ 
        {
            "id" : "should run function Profilerepository.create that creates a new ObjectId",
            "username" : "test user",
            "password" : "test pass",
            "name" : "test name",
            "email" : "[email protected]"
        }
    ]
},
{
    "_id" : ObjectId("5bd83d9ae8abbe501d4b68d0"),
    "name" : "testTeam",
    "todos" : [ 
        {
            "id" : "should run function Profilerepository.create that creates a new ObjectId",
            "username" : "test user",
            "password" : "test pass",
            "name" : "test name",
            "email" : "[email protected]"
        }
    ]
}]

But I Got the error "the relation is not defined for model loopback 4" when I use include filter option. Below code, I used in my application

@get('/todo-lists', {
    responses: {
      '200': {
        description: 'Array of TodoList model instances',
        content: { 'application/json': { schema: { 'x-ts-type': TodoList } } },
      },
    },
  })
  async find(
  ): Promise<TodoList[]> {
    return await this.todoListRepository.find({ include: [{ relation: 'todos' }] });
  }

@b-admike
Copy link
Contributor

@AnanthGopal Please look at #1352 for inclusion of related models.

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

Successfully merging a pull request may close this issue.

7 participants