fix(repository): belongsTo accessor should return undefined if foreign key is not included

[agnes512]

fixes #4147

Relates to #2333

Current behavior

For instances:

Customer {
  id: 1,
  name: 'exists'
}

Order {
  id: 1,
  name: 'has fk',
  customerId: 1
}
Order {
  id: 2,
  name: 'customer doesn't exist',
  customerId: 999
}
Order {
  id: 3,
  name: 'no fk',
}

The result of await orderRepo.customer(order.id = 3); returns the first customer in the database even it doesn't have the foreign key

Proposal

After reading through #2333, this is only a workaround.
Should check the foreign key value of the source instance.
Since different databases use different empty value, we check both undefined and null.

I think this issue can be closed as duplicate..

• when user uses lb4, the memory connector is mostly for tutorials. The priority is relatively low.
• the goal of Memory connector should enforce referential integrity #2333 is to set referential integrity instead of returning [] in this case.
• this workaround changes the result of other connectors. They are able to throw error with id "constraint {\"id\":null}". With the change, they return [].

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

• npm test passes on your machine
• New tests added or existing tests modified to cover all changes
• Code conforms with the style guide
• API Documentation in code was updated
• Documentation in /docs/site was updated
• Affected artifact templates in packages/cli were updated
• Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

[jannyHou]

👍 with a suggestion.

[jannyHou]

maybe use a falsy check like if(!foreignKeyValue) {}?

[agnes512]

👍

@jannyHou Another thing is, we didn't notice that we already have #2333 opened. #4147 is duplicate. I don't know if I should continue on this PR. Because this 'fix' will change some behavior of some connectors ( please the proposal above).
So I am thinking to close this PR without merging it. Thoughts?

[jannyHou]

@agnes512 I think #2333 and #4147 are talking about different issues:

2333: means when CREATE related item, if you provide a non-existing fk(like you have users with id 1 through 10, but you create an order for user with id=11), the record will still be created, which is wrong. The solution would be checking a fk's corresponding source model instance exists in db first, then perform the creation. The story is more like an improvement of our design.

4147: looks like a bug, when GET related item, the included items are wrong. like a todo item with id=1 doesn't belong to any todoList, which means it has an empty fk, but GET todos/1/todoList returns a todo list...and please note the WRONGLY returned todo list does exist in the db, the problem is it doesn't own the todo specified in the request path...

[jannyHou]

Back to this PR, since the belongsTo relation only has a get method(because child only has the get access to its parent, but cannot modify it), I am good with your approach here :) just return an empty result when fk is missing.

Something you may want to double check: should it return an empty ARRAY or just null/undefined?

[agnes512]

You're right. I think the belongsTo accessor only checks the existence. And hasMany and hasOne do creation and traverse.

As for the return, I think undefined is better than [].

Once we done the memory connector referential integrity, this workaround should be removed, and let each connector to handle it.

[nabdelgadir]

As for the return, I think undefined is better than [].

In the PR description you said the connectors return [], so should we try to be consistent with that?

[agnes512]

@nabdelgadir that was my proposal before. But I think Janny's suggestion makes more sense. Will update the description and the title later

[nabdelgadir]

Can you add a link to the issue?

[agnes512]

Good suggestion