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

[Docs] belongsTo() not working in loopback4 - Cannot read property 'target' of undefined #2639

Closed
Ajtak opened this issue Mar 23, 2019 · 24 comments · Fixed by #3808
Closed
Assignees
Labels
Docs Relations Model relations (has many, etc.)

Comments

@Ajtak
Copy link

Ajtak commented Mar 23, 2019

I have create api which fetch the data from database(PostgreSQL). I have same problem as #2209

Table 1 : type(id, name)
Table 2 : tournament(id, type_id, title)
obrazek
PostgreSQL structure: https://github.com/Ajtak/lb4-target-error/blob/master/POSTGRESTRUCTURE.sql

Table type is belongs to table tournament. I want fetch data from type table then respective tournament table data will fetch.

It will return error : 500 TypeError: Cannot read property 'target' of undefined

Sample repo replicate problem:
git clone https://github.com/Ajtak/lb4-target-error.git

Just I want to get tournament with our types.
Thx.

@nabdelgadir
Copy link
Contributor

I tried out your application and it seems the problem is coming from tournament.model.ts, specifically:

@belongsTo(() => Type)
person_id: number;

I tested it with an in-memory connector, but it seems to work if you change person_id to something like type/typeId (type_id or personId won't work either).

Can you try changing it and see if it solves your problem?

@Ajtak
Copy link
Author

Ajtak commented Mar 26, 2019

If I changed type_id to type, i see only

 [
     {
          "id":1,
          "type":1,
          "title":"XXXX"
    }
 ]

without type's name. What is problem?

Expected output

 [
     {
          "id":1,
          "type": {
                  "id": 1,
                  "name": "ABCD"
           },
          "title":"XXXX"
    }
 ]`

@nabdelgadir
Copy link
Contributor

If you want the actual Type object instead of its id, change

@belongsTo(() => Type)
type: number;

to

@belongsTo(() => Type)
type: Type;

@dhmlau dhmlau added the Relations Model relations (has many, etc.) label Mar 30, 2019
@Ajtak
Copy link
Author

Ajtak commented Mar 31, 2019

If I changed datatype, id only is returned again instead of object.
Is need some change in tournament controller?

// Uncomment these imports to begin using these cool features!
// import {inject} from '@loopback/context';
import {Filter, repository} from '@loopback/repository';
import {param,  get, getFilterSchemaFor} from '@loopback/rest';
import {TournamentRepository} from "../repositories";
import {Tournament} from "../models";

export class TournamentController {
    constructor(
        @repository(TournamentRepository)
        public tournamentRepository: TournamentRepository,
    ) { }
    @get('/tournaments', /*{...}*/)
    async find(
        @param.query.object('filter', getFilterSchemaFor(Tournament)) filter?: Filter): Promise<Tournament[]> {
        return await this.tournamentRepository.find(filter);
    }
}

@nabdelgadir
Copy link
Contributor

Our find functions are the same. Can you check the explorer and see if you see this example value:

Screen Shot 2019-04-01 at 10 24 21 AM

And this schema:

Screen Shot 2019-04-01 at 10 31 07 AM

I have a copy of lb4-target-error, if you push your changes to that repo, I can take a look and see what's going on.

@hubertperron
Copy link

Same problem here, I can't figure out how to make loopback lazyload any relations.

@dhmlau
Copy link
Member

dhmlau commented Apr 11, 2019

@Ajtak, I haven't got a chance to run your sample. There is a default name for "belongsTo" property, i.e. {modelName}+Id (camel case). So, the property name in your case should be typeId.

@belongsTo(() => Type)
typeId: number;

If you want to keep the type_id name, you'll need to specify it in the "belongsTo" decorator:

@belongsTo(() => Type, {keyTo: 'type_id'})
  typeId: number;

See https://loopback.io/doc/en/lb4/BelongsTo-relation.html#defining-a-belongsto-relation.

Hope it helps.

cc @b-admike

@dhmlau
Copy link
Member

dhmlau commented Apr 18, 2019

@Ajtak, does the above comment help in getting rid of the error? Thanks.

@samarpanB
Copy link
Contributor

@dhmlau I was going through a similar situation in my code and found that it actually doesn't work somehow. In my case, I needed the property name in my model class to be camel-cased but the db column name to be snake-cased. As per documentation, 'keyTo' is for specifying column name on target model and 'keyFrom' is for specifying column name on source model. So I used keyFrom. But it threw parse error.
This is what I tried.

@belongsTo(() => UserTenant, {keyFrom: 'created_by'})
  createdBy?: number;

@dhmlau
Copy link
Member

dhmlau commented May 10, 2019

@samarpanB, when I was creating the modified CoffeeShop app, I've tried it a bit for @hasMany decorator. Haven't got how to make it work in belongsTo (perhaps I was missing something but didn't get a chance to look into it).


My notes was below:

The expected foreign key name is in the format of {source-model-name-camel-case}+Id, i.e. coffeeShopId in our case. If you wish to specify a different name, you need to modify the @hasMany decorator in the CoffeeShop model to include this information.

  @hasMany(() => Review, {keyTo: 'new-foreign-key-name'})
  reviews?: Review[];

i.e. CoffeeShop hasMany Reviews, and Review belongsTo CoffeeShop.


Hope it helps.

@samarpanB
Copy link
Contributor

@dhmlau I tried that as well with belongs to, it didn’t work either.

@samarpanB
Copy link
Contributor

@dhmlau I got it to work finally. Below code works.

 @belongsTo(
    () => UserTenant,
    {keyFrom: 'created_by', name: 'created_by'},
    {
      name: 'created_by',
    },
  )
  createdBy?: number;

For the findOne or find methods to work via juggler, it requires the name key in third parameter of belongsto decorator. For the _createBelongsToAccessorFor function in repository, it requires name key in second parameter. That's how I got it work perfectly.

@dhmlau
Copy link
Member

dhmlau commented Jun 18, 2019

I think we need to better document how to do this customization. Adding Docs label.

@dhmlau dhmlau changed the title belongsTo() not working in loopback4 - Cannot read property 'target' of undefined [Docs] belongsTo() not working in loopback4 - Cannot read property 'target' of undefined Jun 18, 2019
@dhmlau dhmlau added the 2019Q3 label Jun 18, 2019
@dhmlau
Copy link
Member

dhmlau commented Jun 24, 2019

Acceptance Criteria

  • in the model relation documentations, clarify the use of keyFrom and keyTo in case that users are having different column names as the model property names.

@dhmlau dhmlau added the p1 label Aug 20, 2019
@dhmlau dhmlau added this to the Sept 2019 milestone milestone Sep 3, 2019
@deepakrkris deepakrkris self-assigned this Sep 9, 2019
@deepakrkris
Copy link
Contributor

there is some work going on in #1352 that might need this issue to be revalidated.

@deepakrkris
Copy link
Contributor

@dhmlau examples and test cases for the "keyFrom" parameter are also missing. So I think fixing the docs alone will not be sufficient.

@dhmlau
Copy link
Member

dhmlau commented Sep 18, 2019

@deepakrkris, I think if we don't have test cases covered, then we should add it. Supposedly if the docs are clear, we are ok not to have added this in our examples, e.g. the todo-list example.

@bajtos
Copy link
Member

bajtos commented Sep 19, 2019

AFAICT, we do have tests to verify how to use @belongsTo decorator when the FK name is using underscore-case (e.g. type_id).

Here is a model definition that works in our tests:

https://github.com/strongloop/loopback-next/blob/7bf6f12f1da721a78071904fa02252577f4d1cba/packages/repository-tests/src/crud/relations/fixtures/models/order.model.ts#L41-L42

Here are some of the relevant commits & pull requests:

Considering that this issue was reported several months before those two pull requests, I think we need to find out what's the current status.

Based on the discussion above and the pull requests I referred to, I think this is the solution:

Fix Tournament model (see https://github.com/Ajtak/lb4-target-error/blob/ac48d8baf5b54519d5d1288782f0d39fa5522fbf/src/models/tournament.model.ts) as follows:

-    @belongsTo(() => Type)
+    @belongsTo(() => Type, {name: 'person'})
    person_id: number;

Fix TournamentRepository (see https://github.com/Ajtak/lb4-target-error/blob/ac48d8baf5b54519d5d1288782f0d39fa5522fbf/src/repositories/tournament.repository.ts) to use the correct relation name:

        this.type = this.createBelongsToAccessorFor(
-            'type',
+            'person',
            customerRepositoryGetter,
        );

Here is what I would like you @deepakrkris to do:

  • Find out how to fix the sample app provided by @Ajtak https://github.com/Ajtak/lb4-target-error. My proposal above may or may not be sufficient.

  • Update the documentation to better explain how to define belongsTo relation when the key is not following our camelCase convention.

  • The sample app provided by @Ajtak defines only @belongsTo relation. Typical applications will configure @hasMany relation too. It would be great to cover this part in the documentation changes too. Here is a hint from Custom foreign key name #3326:

    @hasMany(() => Order, {keyTo: 'shipment_id'})
    shipmentOrders: Order[];

Regarding the example shown by @samarpanB in #2639 (comment).

 @belongsTo(
    () => UserTenant,
    {keyFrom: 'created_by', name: 'created_by'},
    {
      name: 'created_by',
    },
  )
  createdBy?: number;

IMO, it's a slightly different scenario. It shows how to use a different property name in LoopBack model & REST API payloads, not the one used by the database. (E.g. change created_by column in the database to createdBy property in LoopBack.)

It would be great to have this scenario documented too, but I feel it's a "nice to have" feature in the context of this issue. Plus as you wrote above, we don't have good test coverage for keyFrom, so we should add new tests to ensure the setup described in our docs is actually working.

Let's start with documenting the first scenario, where the FK property used the same name as the backing database column. Once that PR is landed, then we can decide whether to work on keyFrom as part of this issue or open a new one.

Regarding example apps, I agree with @dhmlau that we don't need to include these two scenarios in our example apps. We want the example apps to be easy to read and understand, not a kitchen-sink-like behemoth showing everything the framework can do.

@dhmlau
Copy link
Member

dhmlau commented Sep 27, 2019

Deferred to Oct, as there is ongoing discussion: https://github.com/strongloop/loopback-next/pull/3808/files#r329042766

@dhmlau dhmlau added 2019Q4 and removed 2019Q3 labels Sep 27, 2019
@odykyi

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Docs Relations Model relations (has many, etc.)
Projects
None yet
9 participants