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

fix(repository): build relations based on their names #1939

Merged
merged 1 commit into from
Oct 30, 2018
Merged

Conversation

raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Oct 29, 2018

The current implementation uses property names as the key of
relations and it's not compatible with legacy juggler.

See #1909

Checklist

  • 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

The current implementation uses property names as the key of
relations and it's not compatible with legacy juggler.

See #1909
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not able to fully comprehend the ramification of proposed implementation changes. But since I don't see any obvious problem and the changes in the test suite looks reasonable, the patch LGTM 👍

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

I realized this PR is just a starting point to fully support include. There are a few issues:

  1. LB4 adds a property for hasMany relation. For example: A customer class will have orders property but loopback-datasource-juggler does not like that for relational databases
  2. We don't convert LB4 relations into juggler model definition - see https://github.com/strongloop/loopback-next/blob/master/packages/repository/src/repositories/legacy-juggler-bridge.ts#L131. Mapping LB4 relations into juggler is difficult as it needs to remap the source/target model too.

@raymondfeng raymondfeng merged commit 2046701 into master Oct 30, 2018
@raymondfeng raymondfeng deleted the fix-1909 branch October 30, 2018 17:00
@raymondfeng raymondfeng mentioned this pull request Oct 31, 2018
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Relations Model relations (has many, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants