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 select for update #906

Merged
merged 5 commits into from Feb 7, 2021
Merged

Conversation

dag18e
Copy link
Contributor

@dag18e dag18e commented Dec 16, 2020

Fixes #866

Description of the Change

Oracle Database 12c does not allow limits on select_for_update statements. This change removes the limit and instead just uses a get.

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated
  • CHANGELOG.md updated (only for user relevant changes)
  • author name in AUTHORS

@coveralls
Copy link

coveralls commented Dec 16, 2020

Pull Request Test Coverage Report for Build 1522

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.06%) to 94.954%

Files with Coverage Reduction New Missed Lines %
oauth2_provider/models.py 1 97.0%
Totals Coverage Status
Change from base Build 1520: -0.06%
Covered Lines: 1336
Relevant Lines: 1407

💛 - Coveralls

@MattBlack85
Copy link
Contributor

@dag18e I understand your point but your change is gonna drop the lock on the database row and it's completely changing the intended behaviour

@MattBlack85
Copy link
Contributor

MattBlack85 commented Dec 17, 2020

@dag18e 🤦 I was reviewing this on my mobile and I didn't notice how it really was. Ignore my first comment.
I have another question tho, I suppose .filter() was used cause we may have more than one token, if we use .get() and there is more than one token the exception is not caught?
Maybe a simple slicing could be used instead?

Also, an entry into the CHANGELOG is required

@dag18e
Copy link
Contributor Author

dag18e commented Jan 13, 2021

@MattBlack85 Hey man, sorry about the delay here. Had some other stuff come up + holidays so I'm just getting back to this. On your note about filter vs get, I changed it a bit to use filter. Basically, I wrap it in a list and grab the first in the list.

This ought to retain the original functionality, but it also doesn't apply a limit to the request so it works with Oracle 12c.

I'll be adding info to docs and changelog once tests pass Travis.

@codecov
Copy link

codecov bot commented Jan 13, 2021

Codecov Report

Merging #906 (fde3b61) into master (3010059) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #906   +/-   ##
=======================================
  Coverage   95.04%   95.04%           
=======================================
  Files          25       25           
  Lines        1070     1070           
=======================================
  Hits         1017     1017           
  Misses         53       53           

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 3010059...fde3b61. Read the comment docs.

@dag18e
Copy link
Contributor Author

dag18e commented Jan 13, 2021

@MattBlack85 Actually, do you think I need to add anything to the documentation? I don't really feel this change is worthy of a change there, but you'd know better than me.

@n2ygk n2ygk added this to the 1.3.4 milestone Feb 7, 2021
@n2ygk n2ygk self-requested a review February 7, 2021 16:44
Copy link
Member

@n2ygk n2ygk left a comment

Choose a reason for hiding this comment

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

@MattBlack85 I think this is good with the change to using a transaction.

Thanks @dag18e

@n2ygk n2ygk merged commit 331b49d into jazzband:master Feb 7, 2021
token = refresh_token_model.objects.select_for_update().filter(
pk=self.pk, revoked__isnull=True
)
except refresh_token_model.DoesNotExist:
Copy link
Contributor

Choose a reason for hiding this comment

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

@dag18e @n2ygk I think we added some things here that are not really needed:

  1. AFAIK .filter() can't raise DoesNotExist
  2. L412 casts the Queryset to a list and then slicing, I do no think this is needed and this slows down things a bit

I feel these things should be addressed in a follow up PR

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.

LIMIT/OFFSET is not supported with select_for_update on this database backend.
5 participants