Skip to content

Conversation

@terencehonles
Copy link

While the existing implementation is simple it: creates a decorated function per request, calls it, and then will discard it. Since the
functionality is straightforward and also encapsulated fairly well in Django, this PR modifies the decorator to copy the original Django decorator, and calls the same helper functions where possible.

Because the functionality is no longer simply wrapping Django's internal implementation, but is still mostly the same, there have been tests added to compare the requests generated by Django vs requests generated by Django Request Framework/this library.

Finally, in order to be less confusing to library writers the call signature of etag_func and last_modified_func has been updated to match Django Request Framework and will pass a DRF Request object instead
of Django's native Request object:

# from the original signature
def original_etag_func(wsgi_request, *args, **kwargs): pass

# to the updated signature
def updated_etag_func(drf_self, drf_request, *args, **kwargs): pass

In order to ease the transition between the two signatures, there has been a flag "use_self" added to the decorators, and a warning will be emitted if use_self has not been set to True.

While the existing implementation is simple it: creates a decorated
function per request, calls it, and then will discard it. Since the
functionality is straightforward and also encapsulated fairly well in
Django, this PR modifies the decorator to copy the original Django
decorator, and calls the same helper functions where possible.

Because the functionality is no longer simply wrapping Django's internal
implementation, but is still mostly the same, there have been tests
added to compare the requests generated by Django vs requests generated
by Django Request Framework/this library.

Finally, in order to be less confusing to library writers the call
signature of `etag_func` and `last_modified_func` has been updated to
match Django Request Framework and will pass a DRF Request object
instead of Django's native Request object::

  # from the original signature
  def original_etag_func(wsgi_request, *args, **kwargs): pass

  # to the updated signature
  def updated_etag_func(drf_self, drf_request, *args, **kwargs): pass

In order to ease the transition between the two signatures, there has
been a flag "use_self" added to the decorators, and a warning will be
emitted if use_self has not been set to True.
@codecov
Copy link

codecov bot commented Jul 31, 2020

Codecov Report

Merging #10 into master will decrease coverage by 7.14%.
The diff coverage is 91.17%.

Impacted file tree graph

@@             Coverage Diff             @@
##            master      #10      +/-   ##
===========================================
- Coverage   100.00%   92.85%   -7.15%     
===========================================
  Files            2        2              
  Lines           21       42      +21     
===========================================
+ Hits            21       39      +18     
- Misses           0        3       +3     
Impacted Files Coverage Δ
rest_framework_condition/decorators.py 92.30% <91.17%> (-7.70%) ⬇️

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 13c076f...7fe7e94. Read the comment docs.

@terencehonles
Copy link
Author

@jozo not sure if you have time to look over this PR?

@jozo
Copy link
Owner

jozo commented Dec 13, 2020

Thank you for your PR @terencehonles . I'm wondering what is the benefit of this PR. Is it performance? If so, how much it improves it? Does it outweigh simplicity of the current implementation?

@terencehonles
Copy link
Author

Well the bigger/biggest benefit is if you need to use the request object that DRF provides you can actually use it. From there you can use request.parser_context to get the view or other things that are passed via the context. This allows conditions that look at what type of API request is being made and makes more sense as this is condition support for DRF not just Django.

As far as performance I figured this was likely just overlooked, but I don't expect Django's conditions expected to be used this way.

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