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: attach FOR NO KEY UPDATE lock to query if required #8008

Merged
merged 1 commit into from Nov 8, 2021

Conversation

das-mensch
Copy link
Contributor

@das-mensch das-mensch commented Aug 2, 2021

Description of change

FOR NO KEY UPDATE was introduced in typeorm 0.2.25 but the relevant query
part was never added due to missing check in FindOptionUtils as @ykovalenko-gentech
stated out. I've added a test to ensure this behaviour will not change in the future.

Fixes #7717

Pull-Request Checklist

  • Code is up-to-date with the master branch
  • npm run lint passes with this change
  • npm run test passes with this change
  • This pull request links relevant issues as Fixes #0000
  • There are new or updated unit tests validating the change
  • Documentation has been updated to reflect this change N/A
  • The new commits follow conventions explained in CONTRIBUTING.md

FOR NO KEY UPDATE was introduced in typeorm 0.2.25 but the relevant query
part was never added due to missing check in FindOptionUtils

Closes: typeorm#7717
@@ -155,7 +155,14 @@ export class FindOptionsUtils {
if (options.lock) {
if (options.lock.mode === "optimistic") {
qb.setLock(options.lock.mode, options.lock.version);
} else if (options.lock.mode === "pessimistic_read" || options.lock.mode === "pessimistic_write" || options.lock.mode === "dirty_read" || options.lock.mode === "pessimistic_partial_write" || options.lock.mode === "pessimistic_write_or_fail") {
} else if (
options.lock.mode === "pessimistic_read" ||

Choose a reason for hiding this comment

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

Is it better to store all possible locks in const array and call array.includes() instead of chaining equality checks.

Copy link
Contributor Author

@das-mensch das-mensch Aug 3, 2021

Choose a reason for hiding this comment

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

Absolutely. The locking code base seems to have grown a lot in the past and needs some refactoring. I would like to create a separate PR for cleaning up. This is to be understood as a "hot fix" as the refactoring will need a bit of time.

@das-mensch
Copy link
Contributor Author

Is there a way to re-trigger the circleci test for cockroachdb? It seems it may just hung up. Not quite familiar with cockroachdb...

@pleerock
Copy link
Member

pleerock commented Nov 8, 2021

Thank you for contribution! 🎉

@pleerock pleerock merged commit 9692930 into typeorm:master Nov 8, 2021
HeartPattern pushed a commit to HeartPattern/typeorm that referenced this pull request Nov 29, 2021
FOR NO KEY UPDATE was introduced in typeorm 0.2.25 but the relevant query
part was never added due to missing check in FindOptionUtils

Closes: typeorm#7717
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.

PostgreSQL - Repository find lock mode for_no_key_update is not being added to query
3 participants