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 issues with routers for custom list-route and detail-routes #4229

Merged
merged 1 commit into from
Jun 29, 2016

Conversation

vikalpj
Copy link
Contributor

@vikalpj vikalpj commented Jun 29, 2016

I have a serializer:

class TestViewSet(viewsets.ViewSet):
    @list_route(['PUT'])
    def update(self, *args, **kwargs):
        return Response({})

    @list_route(['PUT'])
    def hello_rest(self, *args, **kwargs):
        return Response({})

and a router config

router = routers.DefaultRouter(trailing_slash=False)
router.register('test', TestViewSet, base_name='test')

Ideally the above configuration should fail as per https://github.com/tomchristie/django-rest-framework/blob/d41ddc9e5f3346892e46659245522d4be8abdc1a/rest_framework/routers.py#L157. But this passess.

And when I remove the the method hello_rest it fails as expected.

@codecov-io
Copy link

codecov-io commented Jun 29, 2016

Current coverage is 91.55%

Merging #4229 into master will not change coverage

@@             master      #4229   diff @@
==========================================
  Files            51         51          
  Lines          5551       5551          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits           5082       5082          
  Misses          469        469          
  Partials          0          0          

Powered by Codecov. Last updated by d41ddc9...80dedd6

@tomchristie
Copy link
Member

I don't understand the correlation between the change in this PR and the issue described.

@vikalpj
Copy link
Contributor Author

vikalpj commented Jun 29, 2016

@tomchristie so basically I have multiple list routes in the View set and its tested against the loop. Here known host is the iterable and is good for 1 use only.

When the method loop runs and tests for both the methods (update and hello_rest). and it runs 1st for hello_rest and checks the condition correctly. but after this point known_host iterable would be a blank list [].

Now when it tests against update methods, like
if 'update' in []:
this condition would always be false and the exception is never raised.

I had an issue removing a unused route from the codebase where my update function was already overridden as in the example above...

This pull request, forcefully converts the iterator to list, so that this check never fails for the predefined methods.

@xordoquy xordoquy added this to the 3.4.0 Release milestone Jun 29, 2016
@xordoquy xordoquy added the Bug label Jun 29, 2016
@xordoquy
Copy link
Collaborator

Sounds pretty tricky but a good catch.

@tomchristie
Copy link
Member

No problem with the proposed code change whatsoever. Good work.

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

Successfully merging this pull request may close these issues.

4 participants