Implementation of nested route framework using definitions in viewsets

[raphendyr]

trafficstars

I wasn't really happy that nested routes define url kwargs in register method. That split query filtering from definition of queryset. Also writing hyperlinkedidentityfield for these nested paths became overly complex. In addition the url kwargs differ in all nested routes and that makes reading lsit of api urls a bit too complex.

So, I created map in viewset that defines how url kwargs are used in filtering queryset and defining kwarg name only in parent viewset.

I created this PR as place for comments and to tell me if this approach is not what you think as good.

Currently this change would break backwards compatibility and should probably be implemented as new set of classes. Also there is no documentation or test changes yet. I just wanted to get your guys opinions on this matter first.

Here are few examples what this looks in our a-plus django application:

# routes
api = ExtendedDefaultRouter()

api.register(r'users',
             userprofile.api.views.UserViewSet,
             base_name='user')

with api.register(r'courses',
                  course.api.views.CourseViewSet,
                  base_name='course') as courses:
    courses.register(r'exercises',
                     course.api.views.CourseExercisesViewSet,
                     base_name='course-exercises')
    courses.register(r'students',
                     course.api.views.CourseStudentsViewSet,
                     base_name='course-students')

# viewsets
class CourseViewSet(ListSerializerMixin, viewsets.ReadOnlyModelViewSet):
    queryset = CourseInstance.objects.filter(visible_to_students=True)
    permission_classes = [permissions.IsAuthenticated]
    lookup_url_kwarg = 'course_id'
    listserializer_class = CourseBriefSerializer
    serializer_class = CourseSerializer

class CourseExercisesViewSet(NestedViewSetMixin, viewsets.ReadOnlyModelViewSet):
    permission_classes = [permissions.IsAuthenticated]
    lookup_url_kwarg = 'exercisemodule_id'
    serializer_class = CourseModuleSerializer
    queryset = CourseModule.objects.all()
    parent_lookup_map = {'course_id': 'course_instance.id'}

class CourseStudentsViewSet(NestedViewSetMixin, viewsets.ReadOnlyModelViewSet):
    permission_classes = [permissions.IsAuthenticated]
    lookup_url_kwarg = 'user_id'
    serializer_class = UserBriefSerialiser
    queryset = UserProfile.objects.all()
    parent_lookup_map = {'course_id': 'enrolled.id'}

# one of the serializers
class CourseSerializer(CourseBriefSerializer):
    exercises = NestedHyperlinkedIdentityField(view_name='api:course-exercises-list', format='html')
    students = NestedHyperlinkedIdentityField(view_name='api:course-students-list', format='html')

    class Meta(CourseBriefSerializer.Meta):
        fields = CourseBriefSerializer.Meta.fields + (
            'starting_time',
            'ending_time',
            'exercises',
            'students',
        )

[auvipy]

thanks for the PR. please wait for detail feedback. on a side note could you manage some time to fix the compat issues and failing tests on a separate PR/PR's?

[auvipy]

@raphendyr could you check the test failing? and manage some time to pass the 1.8 failing tests?

[raphendyr]

I will look into tests and documentation if this is good addition. Didn't spend time on those before some more higher level feedback on idea.

Also, should this be implemented as another Mixin, so the old syntax and usage style is not removed? Would it be better that way?

[auvipy]

could you provide some docs too for the proposed changes? As it will be bc breaking change you could try to deprecate any thing that you thing should be deprecated. your changes seems OK to me.

[raphendyr]

Ok. I'll start looking to make this real PR on next week. Hopefully I get it done there.

[raphendyr]

I reimplemented my reimplementation as this new way requires less iteration.

Old tests work on new code (old use case should still work). I'm still in progress of writing new test cases for new code. Documentation also needs some work still.

Updated my branch so other people can take look at it too.

[auvipy]

thank you for you contribution :)

[auvipy]

can you also look on this https://github.com/chibisov/drf-extensions/pull/157

[raphendyr]

I updated the implementation. Old one can be found here.

I'll keep this PR updated, so people can test this too. I try to get this finalized in month or two, but the main project that is using this requires my main focus.

[auvipy]

hi are you planning to start working on it?

[raphendyr]

I hope if I can validate the code against current head. As I have learned about swagger, the walkable apis aren't so good approach anymore. This means ``NestedHyperlinkedIdentityField` isn't so important part. The nesting for route configuration works well (been in production for all of this time).

I'm currently working only few days a week on the project that uses this, but I do my best to allocate some time to get this PR sorted out.

I think we are missing documentation and testes for the new code. Presuming my memory serves me right I adjusted the tests to check that old syntax still works.

[auvipy]

I believe this can be rebased against master

[auvipy]

@chibisov I would like you to have some thoughts

[Xazzzi]

Any progress here?

[raphendyr]

I try to make rebase in few weeks.

[raphendyr]

I'm back to full day jpb for the summer, so I have time to get this finally done. Rebased on top of current master here https://github.com/raphendyr/drf-extensions/tree/reimplement_nested_routes_v3 . Gonna test that out and then finalize it. I'll update the PR at that point.

[auvipy]

@raphendyr thanks for your effort.

[auvipy]

could you please rebase?

[raphendyr]

I'll allocate time this autumn for this. I'll include the nested route part, but I'll drop the hyperlink stuff for now. Crawlable APIs aren't a good solutions. OpenAPI/Swagger is way better design.

Sadly, I have had a bit too much work on different parts for past year to look into this.

[auvipy]

lets wait for drf 3.9 then