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): use unique indexes for hasOne FK constraint #2126

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@b-admike
Copy link
Member

b-admike commented Dec 6, 2018

re-work hasOne relation based on #2005 (comment) and consequent comments. See if this approach is plausible. Memory connector doesn't have concept of unique indexes AFAIK, so I'll have to make a fix there to get the uniqueness on create test passing. @bajtos @raymondfeng PTAL.

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

@b-admike b-admike requested review from bajtos and raymondfeng as code owners Dec 6, 2018

@bajtos
Copy link
Member

bajtos left a comment

I am fine with researching the ramifications of this approach, but I don't think it's a good solution that will work for general use.

expect(persisted.toObject()).to.deepEqual(address.toObject());
});

it('refuses to create related model instance twice', async () => {
const address = await controller.createCustomerAddress(existingCustomerId, {
street: '123 test avenue',
});
// FIXME(b-admike): memory connector doesn't support unique indexes

This comment has been minimized.

@bajtos

bajtos Dec 6, 2018

Member

Please check other commonly used connectors too! For example:

  • MongoDB
  • CouchDB/Cloudant
  • MySQL
  • PostgreSQL
  • Oracle
  • MSSQL

This comment has been minimized.

@b-admike

b-admike Dec 6, 2018

Member

So far from what I checked, MongoDB, MysQL, PostgreSQL, and Oracle support unique indexes. Will check the others and report back 👍

@@ -27,5 +27,6 @@ export class Order extends Entity {
isShipped: boolean;

@belongsTo(() => Customer)
@property()

This comment has been minimized.

@bajtos

bajtos Dec 6, 2018

Member

IMO, this is a breaking change, since you are requiring all existing users of belongsTo to add @property decorator to their models. Making a breaking change comes with (maintenance) costs, see https://github.com/strongloop/loopback-next/blob/8394c74d5369d7dcce861c0ed30e033a3e7ce3ae/docs/site/DEVELOPING.md#making-breaking-changes

This comment has been minimized.

@b-admike

b-admike Dec 6, 2018

Member

Good point. After reading the guide about frequency of breaking changes and what warrants them/support model, I have decided to revert these changes for now and just keep this PR about usage of unique indexes. @raymondfeng thoughts?

@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Dec 6, 2018

I feel we need to step back a bit and review our overall design for relations and how to map it to what different SQL/NoSQL databases support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment