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: await PK on create before updating relationships #737

Open
wants to merge 2 commits into
base: stable
Choose a base branch
from

Conversation

nickschot
Copy link
Contributor

@nickschot nickschot commented Jan 3, 2019

In the case of a belongsTo <-> hasOne relationship Lux attempts to update/overwrite the belongsTo FK. For new records (CREATE) however, this has the side effect of (at least with postgres) not working correctly. It attempts to run the query within the CREATE transaction resulting in a "nextval(...)" in the query instead of the (currently nonexistent) primary key.

NOTE: side-effect is that duplicate ID's might occur when DB does not have a UNIQUE constraint on the FK.

This PR has been updated with code to restructure the create method of the model so it await's the insert query before updating the relationships. This way we are sure to have a valid PK (instead of the nextval stuff).

NOTE: The only downside of this approach is that the relationships are not set on validation. It might be possible to set belongsTo relationships before validation, but we must not use the created query yet as that will use an invalid PK.

   NOTE: side-effect is that duplicate ID's might occur when DB does not have a UNIQUE constraint on the FK
@codecov
Copy link

codecov bot commented Jan 3, 2019

Codecov Report

Merging #737 into stable will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           stable     #737   +/-   ##
=======================================
  Coverage   92.41%   92.41%           
=======================================
  Files         181      181           
  Lines        2018     2018           
=======================================
  Hits         1865     1865           
  Misses        153      153
Impacted Files Coverage Δ
...database/relationship/utils/update-relationship.js 77.41% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 812555a...0b77eea. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Jan 3, 2019

Codecov Report

Merging #737 into stable will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           stable     #737   +/-   ##
=======================================
  Coverage   92.41%   92.41%           
=======================================
  Files         181      181           
  Lines        2018     2018           
=======================================
  Hits         1865     1865           
  Misses        153      153
Impacted Files Coverage Δ
...database/relationship/utils/update-relationship.js 77.41% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 812555a...0b77eea. Read the comment docs.

@codecov
Copy link

codecov bot commented Jan 3, 2019

Codecov Report

Merging #737 into stable will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           stable     #737   +/-   ##
=======================================
  Coverage   92.41%   92.41%           
=======================================
  Files         181      181           
  Lines        2018     2018           
=======================================
  Hits         1865     1865           
  Misses        153      153
Impacted Files Coverage Δ
...database/relationship/utils/update-relationship.js 77.41% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 812555a...c39cdd7. Read the comment docs.

@codecov
Copy link

codecov bot commented Jan 3, 2019

Codecov Report

Merging #737 into stable will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           stable     #737   +/-   ##
=======================================
  Coverage   92.41%   92.41%           
=======================================
  Files         181      181           
  Lines        2018     2018           
=======================================
  Hits         1865     1865           
  Misses        153      153
Impacted Files Coverage Δ
...database/relationship/utils/update-relationship.js 77.41% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 812555a...0b77eea. Read the comment docs.

@nickschot
Copy link
Contributor Author

I am now thinking this error is caused by something (presumably lux) parsing the nextVal function into a string causing Postgres to error.

@nickschot
Copy link
Contributor Author

This problem also exists with the update hasOne relationship.

@nickschot
Copy link
Contributor Author

nickschot commented Jan 10, 2019

See #739 , the "fix" proposed in this PR is not really desirable.

…ore updating relationships

- undo isNew check in updateRelationships
@nickschot nickschot changed the title fix: don't update belongsTo relationships for new records fix: await PK on create before updating relationships Jan 23, 2019
@nickschot
Copy link
Contributor Author

PR has been updated with a new and much better fix.

@mdconaway
Copy link

Any progress on evaluating/merging this fix?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants