From 7bc5f1e4a3a309a6d4bb1c121fd6f76624ed6fd0 Mon Sep 17 00:00:00 2001 From: Marcin Lubimow Date: Sat, 7 Oct 2017 19:14:10 +0100 Subject: [PATCH 1/8] Fixed issues with schema name collisions --- rest_framework/schemas/generators.py | 40 +++++- tests/test_schemas.py | 183 ++++++++++++++++++++------- 2 files changed, 174 insertions(+), 49 deletions(-) diff --git a/rest_framework/schemas/generators.py b/rest_framework/schemas/generators.py index a9fa15b878..9a0929bbf3 100644 --- a/rest_framework/schemas/generators.py +++ b/rest_framework/schemas/generators.py @@ -64,6 +64,12 @@ def is_api_view(callback): """ +class LinkNode(OrderedDict): + def __init__(self): + self.links = [] + super(LinkNode, self).__init__() + + def insert_into(target, keys, value): """ Nested dictionary insertion. @@ -71,14 +77,15 @@ def insert_into(target, keys, value): >>> example = {} >>> insert_into(example, ['a', 'b', 'c'], 123) >>> example - {'a': {'b': {'c': 123}}} + LinkNode({'a': LinkNode({'b': LinkNode({'c': LinkNode(links=[123])}}}))) """ - for key in keys[:-1]: + for key in keys: if key not in target: - target[key] = {} + target[key] = LinkNode() target = target[key] + try: - target[keys[-1]] = value + target.links.append(value) except TypeError: msg = INSERT_INTO_COLLISION_FMT.format( value_url=value.url, @@ -88,6 +95,27 @@ def insert_into(target, keys, value): raise ValueError(msg) +def get_unique_key(obj, base_key): + i = 0 + while True: + key = '{}_{}'.format(base_key, i) + if key not in obj: + return key + i += 1 + + +def distribute_links(obj, parent=None, parent_key='root'): + if parent is None: + parent = obj + + for link in obj.links: + key = get_unique_key(parent, parent_key) + parent[key] = link + + for key, value in obj.items(): + distribute_links(value, obj, key) + + def is_custom_action(action): return action not in set([ 'retrieve', 'list', 'create', 'update', 'partial_update', 'destroy' @@ -255,6 +283,7 @@ def get_schema(self, request=None, public=False): if not url and request is not None: url = request.build_absolute_uri() + distribute_links(links) return coreapi.Document( title=self.title, description=self.description, url=url, content=links @@ -265,7 +294,7 @@ def get_links(self, request=None): Return a dictionary containing all the links that should be included in the API schema. """ - links = OrderedDict() + links = LinkNode() # Generate (path, method, view) given (path, method, callback). paths = [] @@ -288,6 +317,7 @@ def get_links(self, request=None): subpath = path[len(prefix):] keys = self.get_keys(subpath, method, view) insert_into(links, keys, link) + return links # Methods used when we generate a view instance from the raw callback... diff --git a/tests/test_schemas.py b/tests/test_schemas.py index 901f4c6c56..c5c418c011 100644 --- a/tests/test_schemas.py +++ b/tests/test_schemas.py @@ -19,7 +19,6 @@ AutoSchema, ManualSchema, SchemaGenerator, get_schema_view ) from rest_framework.schemas.generators import EndpointEnumerator -from rest_framework.schemas.utils import is_list_view from rest_framework.test import APIClient, APIRequestFactory from rest_framework.utils import formatting from rest_framework.views import APIView @@ -121,7 +120,8 @@ def test_anonymous_request(self): title='Example API', content={ 'example': { - 'list': coreapi.Link( + 'list': {}, + 'list_0': coreapi.Link( url='/example/', action='get', fields=[ @@ -130,17 +130,20 @@ def test_anonymous_request(self): coreapi.Field('ordering', required=False, location='query', schema=coreschema.String(title='Ordering', description='Which field to use when ordering the results.')) ] ), - 'custom_list_action': coreapi.Link( + 'custom_list_action': {}, + 'custom_list_action_0': coreapi.Link( url='/example/custom_list_action/', action='get' ), 'custom_list_action_multiple_methods': { - 'read': coreapi.Link( + 'read': {}, + 'read_0': coreapi.Link( url='/example/custom_list_action_multiple_methods/', action='get' ) }, - 'read': coreapi.Link( + 'read': {}, + 'read_0': coreapi.Link( url='/example/{id}/', action='get', fields=[ @@ -151,6 +154,7 @@ def test_anonymous_request(self): } } ) + assert response.data == expected def test_authenticated_request(self): @@ -163,7 +167,8 @@ def test_authenticated_request(self): title='Example API', content={ 'example': { - 'list': coreapi.Link( + 'list': {}, + 'list_0': coreapi.Link( url='/example/', action='get', fields=[ @@ -172,7 +177,8 @@ def test_authenticated_request(self): coreapi.Field('ordering', required=False, location='query', schema=coreschema.String(title='Ordering', description='Which field to use when ordering the results.')) ] ), - 'create': coreapi.Link( + 'create': {}, + 'create_0': coreapi.Link( url='/example/', action='post', encoding='application/json', @@ -181,7 +187,8 @@ def test_authenticated_request(self): coreapi.Field('b', required=False, location='form', schema=coreschema.String(title='B')) ] ), - 'read': coreapi.Link( + 'read': {}, + 'read_0': coreapi.Link( url='/example/{id}/', action='get', fields=[ @@ -189,7 +196,8 @@ def test_authenticated_request(self): coreapi.Field('ordering', required=False, location='query', schema=coreschema.String(title='Ordering', description='Which field to use when ordering the results.')) ] ), - 'custom_action': coreapi.Link( + 'custom_action': {}, + 'custom_action_0': coreapi.Link( url='/example/{id}/custom_action/', action='post', encoding='application/json', @@ -200,7 +208,8 @@ def test_authenticated_request(self): coreapi.Field('d', required=False, location='form', schema=coreschema.String(title='D')), ] ), - 'custom_action_with_list_fields': coreapi.Link( + 'custom_action_with_list_fields': {}, + 'custom_action_with_list_fields_0': coreapi.Link( url='/example/{id}/custom_action_with_list_fields/', action='post', encoding='application/json', @@ -211,21 +220,25 @@ def test_authenticated_request(self): coreapi.Field('b', required=True, location='form', schema=coreschema.Array(title='B', items=coreschema.String())), ] ), - 'custom_list_action': coreapi.Link( + 'custom_list_action': {}, + 'custom_list_action_0': coreapi.Link( url='/example/custom_list_action/', action='get' ), 'custom_list_action_multiple_methods': { - 'read': coreapi.Link( + 'read': {}, + 'read_0': coreapi.Link( url='/example/custom_list_action_multiple_methods/', action='get' ), - 'create': coreapi.Link( + 'create': {}, + 'create_0': coreapi.Link( url='/example/custom_list_action_multiple_methods/', action='post' ) }, - 'update': coreapi.Link( + 'update': {}, + 'update_0': coreapi.Link( url='/example/{id}/', action='put', encoding='application/json', @@ -236,7 +249,8 @@ def test_authenticated_request(self): coreapi.Field('ordering', required=False, location='query', schema=coreschema.String(title='Ordering', description='Which field to use when ordering the results.')) ] ), - 'partial_update': coreapi.Link( + 'partial_update': {}, + 'partial_update_0': coreapi.Link( url='/example/{id}/', action='patch', encoding='application/json', @@ -247,7 +261,8 @@ def test_authenticated_request(self): coreapi.Field('ordering', required=False, location='query', schema=coreschema.String(title='Ordering', description='Which field to use when ordering the results.')) ] ), - 'delete': coreapi.Link( + 'delete': {}, + 'delete_0': coreapi.Link( url='/example/{id}/', action='delete', fields=[ @@ -329,17 +344,20 @@ def test_schema_for_regular_views(self): title='Example API', content={ 'example': { - 'create': coreapi.Link( + 'create': {}, + 'create_0': coreapi.Link( url='/example/', action='post', fields=[] ), - 'list': coreapi.Link( + 'list': {}, + 'list_0': coreapi.Link( url='/example/', action='get', fields=[] ), - 'read': coreapi.Link( + 'read': {}, + 'read_0': coreapi.Link( url='/example/{id}/', action='get', fields=[ @@ -347,7 +365,8 @@ def test_schema_for_regular_views(self): ] ), 'sub': { - 'list': coreapi.Link( + 'list': {}, + 'list_0': coreapi.Link( url='/example/{id}/sub/', action='get', fields=[ @@ -382,17 +401,20 @@ def test_schema_for_regular_views(self): title='Example API', content={ 'example': { - 'create': coreapi.Link( + 'create': {}, + 'create_0': coreapi.Link( url='/api/v1/example/', action='post', fields=[] ), - 'list': coreapi.Link( + 'list': {}, + 'list_0': coreapi.Link( url='/api/v1/example/', action='get', fields=[] ), - 'read': coreapi.Link( + 'read': {}, + 'read_0': coreapi.Link( url='/api/v1/example/{id}/', action='get', fields=[ @@ -400,7 +422,8 @@ def test_schema_for_regular_views(self): ] ), 'sub': { - 'list': coreapi.Link( + 'list': {}, + 'list_0': coreapi.Link( url='/api/v1/example/{id}/sub/', action='get', fields=[ @@ -437,7 +460,8 @@ def test_schema_for_regular_views(self): title='Example API', content={ 'example1': { - 'list': coreapi.Link( + 'list': {}, + 'list_0': coreapi.Link( url='/example1/', action='get', fields=[ @@ -446,17 +470,20 @@ def test_schema_for_regular_views(self): coreapi.Field('ordering', required=False, location='query', schema=coreschema.String(title='Ordering', description='Which field to use when ordering the results.')) ] ), - 'custom_list_action': coreapi.Link( + 'custom_list_action': {}, + 'custom_list_action_0': coreapi.Link( url='/example1/custom_list_action/', action='get' ), 'custom_list_action_multiple_methods': { - 'read': coreapi.Link( + 'read': {}, + 'read_0': coreapi.Link( url='/example1/custom_list_action_multiple_methods/', action='get' ) }, - 'read': coreapi.Link( + 'read': {}, + 'read_0': coreapi.Link( url='/example1/{id}/', action='get', fields=[ @@ -494,7 +521,8 @@ def test_schema_for_regular_views(self): title='Example API', content={ 'example': { - 'list': coreapi.Link( + 'list': {}, + 'list_0': coreapi.Link( url='/example/', action='get', fields=[] @@ -666,7 +694,8 @@ def test_schema_generator_excludes_correctly(self): title='Exclusions', content={ 'included-fbv': { - 'list': coreapi.Link(url='/included-fbv/', action='get') + 'list': {}, + 'list_0': coreapi.Link(url='/included-fbv/', action='get') } } ) @@ -749,6 +778,10 @@ class NamingCollisionView(generics.RetrieveUpdateDestroyAPIView): serializer_class = BasicModelSerializer +class BasicNamingCollisionView(generics.RetrieveAPIView): + queryset = BasicModel.objects.all() + + class NamingCollisionViewSet(GenericViewSet): """ Example via: https://stackoverflow.com/questions/43778668/django-rest-framwork-occured-typeerror-link-object-does-not-support-item-ass/ @@ -779,9 +812,31 @@ def test_manually_routing_nested_routes(self): ] generator = SchemaGenerator(title='Naming Colisions', patterns=patterns) + schema = generator.get_schema() + + expected = coreapi.Document( + url='', + title='Naming Colisions', + content={ + 'test': { + 'list': { + 'list': {}, + 'list_0': coreapi.Link(url='/test/list/', action='get') + }, + 'list_0': coreapi.Link(url='/test', action='get') + } + } + ) + + assert expected == schema + + def _verify_cbv_links(self, loc, url, methods=None, number=0): + if methods is None: + methods = ('read', 'update', 'partial_update', 'delete') - with pytest.raises(ValueError): - generator.get_schema() + for method in methods: + key = '{}_{}'.format(method, number) + assert loc[key].url == url def test_manually_routing_generic_view(self): patterns = [ @@ -797,8 +852,14 @@ def test_manually_routing_generic_view(self): generator = SchemaGenerator(title='Naming Colisions', patterns=patterns) - with pytest.raises(ValueError): - generator.get_schema() + schema = generator.get_schema() + + self._verify_cbv_links(schema['test']['delete'], '/test/delete/') + self._verify_cbv_links(schema['test']['put'], '/test/put/') + self._verify_cbv_links(schema['test']['get'], '/test/get/') + self._verify_cbv_links(schema['test']['update'], '/test/update/') + self._verify_cbv_links(schema['test']['retrieve'], '/test/retrieve/') + self._verify_cbv_links(schema['test'], '/test') def test_from_router(self): patterns = [ @@ -806,18 +867,52 @@ def test_from_router(self): ] generator = SchemaGenerator(title='Naming Colisions', patterns=patterns) + schema = generator.get_schema() + desc = schema['detail_0'].description # not important here + + expected = coreapi.Document( + url='', + title='Naming Colisions', + content={ + 'detail': { + 'detail_export': {}, + 'detail_export_0': coreapi.Link( + url='/from-routercollision/detail/export/', + action='get', + description=desc) + }, + 'detail_0': coreapi.Link( + url='/from-routercollision/detail/', + action='get', + description=desc + ) + } + ) + + assert schema == expected + + def test_url_under_same_key_not_replaced(self): + patterns = [ + url(r'example/(?P\d+)/$', BasicNamingCollisionView.as_view()), + url(r'example/(?P\w+)/$', BasicNamingCollisionView.as_view()), + ] + + generator = SchemaGenerator(title='Naming Colisions', patterns=patterns) + schema = generator.get_schema() - with pytest.raises(ValueError): - generator.get_schema() + assert schema['example']['read'] == {} + assert schema['example']['read_0'].url == '/example/{id}/' + assert schema['example']['read_1'].url == '/example/{slug}/' + def test_url_under_same_key_not_replaced_another(self): -def test_is_list_view_recognises_retrieve_view_subclasses(): - class TestView(generics.RetrieveAPIView): - pass + patterns = [ + url(r'^test/list/', simple_fbv), + url(r'^test/(?P\d+)/list/', simple_fbv), + ] - path = '/looks/like/a/list/view/' - method = 'get' - view = TestView() + generator = SchemaGenerator(title='Naming Colisions', patterns=patterns) + schema = generator.get_schema() - is_list = is_list_view(path, method, view) - assert not is_list, "RetrieveAPIView subclasses should not be classified as list views." + assert schema['test']['list']['list_0'].url == '/test/list/' + assert schema['test']['list']['list_1'].url == '/test/{id}/list/' From ce12c0bbd9aac1a1ac028e268b0757eeae82d884 Mon Sep 17 00:00:00 2001 From: Marcin Lubimow Date: Sat, 7 Oct 2017 19:31:55 +0100 Subject: [PATCH 2/8] Fixed mutating issues in python 3 --- rest_framework/schemas/generators.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rest_framework/schemas/generators.py b/rest_framework/schemas/generators.py index 9a0929bbf3..10427f2101 100644 --- a/rest_framework/schemas/generators.py +++ b/rest_framework/schemas/generators.py @@ -112,8 +112,8 @@ def distribute_links(obj, parent=None, parent_key='root'): key = get_unique_key(parent, parent_key) parent[key] = link - for key, value in obj.items(): - distribute_links(value, obj, key) + for key in list(obj.keys()): + distribute_links(obj[key], obj, key) def is_custom_action(action): From bde90d26566f74ea5cde72ee58a49b7d834fcd3a Mon Sep 17 00:00:00 2001 From: Marcin Lubimow Date: Sun, 8 Oct 2017 15:25:11 +0100 Subject: [PATCH 3/8] Optimized solution --- rest_framework/schemas/generators.py | 44 ++++------- tests/test_schemas.py | 108 ++++++++++----------------- 2 files changed, 53 insertions(+), 99 deletions(-) diff --git a/rest_framework/schemas/generators.py b/rest_framework/schemas/generators.py index 10427f2101..083f6bf7d8 100644 --- a/rest_framework/schemas/generators.py +++ b/rest_framework/schemas/generators.py @@ -4,7 +4,7 @@ See schemas.__init__.py for package overview. """ import warnings -from collections import OrderedDict +from collections import OrderedDict, Counter from importlib import import_module from django.conf import settings @@ -67,8 +67,14 @@ def is_api_view(callback): class LinkNode(OrderedDict): def __init__(self): self.links = [] + self.methods_counter = Counter() super(LinkNode, self).__init__() + def get_next_key(self, method): + current_val = self.methods_counter[method] + self.methods_counter[method] += 1 + return '{}_{}'.format(method, current_val) + def insert_into(target, keys, value): """ @@ -95,25 +101,13 @@ def insert_into(target, keys, value): raise ValueError(msg) -def get_unique_key(obj, base_key): - i = 0 - while True: - key = '{}_{}'.format(base_key, i) - if key not in obj: - return key - i += 1 - - -def distribute_links(obj, parent=None, parent_key='root'): - if parent is None: - parent = obj +def distribute_links(obj): + for key, value in obj.items(): + distribute_links(value) for link in obj.links: - key = get_unique_key(parent, parent_key) - parent[key] = link - - for key in list(obj.keys()): - distribute_links(obj[key], obj, key) + key = obj.get_next_key(str(link.action)) + obj[key] = link def is_custom_action(action): @@ -438,16 +432,8 @@ def get_keys(self, subpath, method, view): if is_custom_action(action): # Custom action, eg "/users/{pk}/activate/", "/users/active/" - if len(view.action_map) > 1: - action = self.default_mapping[method.lower()] - if action in self.coerce_method_names: - action = self.coerce_method_names[action] - return named_path_components + [action] - else: - return named_path_components[:-1] + [action] - - if action in self.coerce_method_names: - action = self.coerce_method_names[action] + if len(view.action_map) == 1: + return named_path_components[:-1] # Default action, eg "/users/", "/users/{pk}/" - return named_path_components + [action] + return named_path_components diff --git a/tests/test_schemas.py b/tests/test_schemas.py index c5c418c011..08e4a023a8 100644 --- a/tests/test_schemas.py +++ b/tests/test_schemas.py @@ -120,8 +120,7 @@ def test_anonymous_request(self): title='Example API', content={ 'example': { - 'list': {}, - 'list_0': coreapi.Link( + 'get_0': coreapi.Link( url='/example/', action='get', fields=[ @@ -130,20 +129,17 @@ def test_anonymous_request(self): coreapi.Field('ordering', required=False, location='query', schema=coreschema.String(title='Ordering', description='Which field to use when ordering the results.')) ] ), - 'custom_list_action': {}, - 'custom_list_action_0': coreapi.Link( + 'get_1': coreapi.Link( url='/example/custom_list_action/', action='get' ), 'custom_list_action_multiple_methods': { - 'read': {}, - 'read_0': coreapi.Link( + 'get_0': coreapi.Link( url='/example/custom_list_action_multiple_methods/', action='get' ) }, - 'read': {}, - 'read_0': coreapi.Link( + 'get_2': coreapi.Link( url='/example/{id}/', action='get', fields=[ @@ -167,8 +163,7 @@ def test_authenticated_request(self): title='Example API', content={ 'example': { - 'list': {}, - 'list_0': coreapi.Link( + 'get_0': coreapi.Link( url='/example/', action='get', fields=[ @@ -177,8 +172,7 @@ def test_authenticated_request(self): coreapi.Field('ordering', required=False, location='query', schema=coreschema.String(title='Ordering', description='Which field to use when ordering the results.')) ] ), - 'create': {}, - 'create_0': coreapi.Link( + 'post_0': coreapi.Link( url='/example/', action='post', encoding='application/json', @@ -187,8 +181,7 @@ def test_authenticated_request(self): coreapi.Field('b', required=False, location='form', schema=coreschema.String(title='B')) ] ), - 'read': {}, - 'read_0': coreapi.Link( + 'get_2': coreapi.Link( url='/example/{id}/', action='get', fields=[ @@ -196,8 +189,7 @@ def test_authenticated_request(self): coreapi.Field('ordering', required=False, location='query', schema=coreschema.String(title='Ordering', description='Which field to use when ordering the results.')) ] ), - 'custom_action': {}, - 'custom_action_0': coreapi.Link( + 'post_1': coreapi.Link( url='/example/{id}/custom_action/', action='post', encoding='application/json', @@ -208,8 +200,7 @@ def test_authenticated_request(self): coreapi.Field('d', required=False, location='form', schema=coreschema.String(title='D')), ] ), - 'custom_action_with_list_fields': {}, - 'custom_action_with_list_fields_0': coreapi.Link( + 'post_2': coreapi.Link( url='/example/{id}/custom_action_with_list_fields/', action='post', encoding='application/json', @@ -220,25 +211,21 @@ def test_authenticated_request(self): coreapi.Field('b', required=True, location='form', schema=coreschema.Array(title='B', items=coreschema.String())), ] ), - 'custom_list_action': {}, - 'custom_list_action_0': coreapi.Link( + 'get_1': coreapi.Link( url='/example/custom_list_action/', action='get' ), 'custom_list_action_multiple_methods': { - 'read': {}, - 'read_0': coreapi.Link( + 'get_0': coreapi.Link( url='/example/custom_list_action_multiple_methods/', action='get' ), - 'create': {}, - 'create_0': coreapi.Link( + 'post_0': coreapi.Link( url='/example/custom_list_action_multiple_methods/', action='post' ) }, - 'update': {}, - 'update_0': coreapi.Link( + 'put_0': coreapi.Link( url='/example/{id}/', action='put', encoding='application/json', @@ -249,8 +236,7 @@ def test_authenticated_request(self): coreapi.Field('ordering', required=False, location='query', schema=coreschema.String(title='Ordering', description='Which field to use when ordering the results.')) ] ), - 'partial_update': {}, - 'partial_update_0': coreapi.Link( + 'patch_0': coreapi.Link( url='/example/{id}/', action='patch', encoding='application/json', @@ -261,7 +247,6 @@ def test_authenticated_request(self): coreapi.Field('ordering', required=False, location='query', schema=coreschema.String(title='Ordering', description='Which field to use when ordering the results.')) ] ), - 'delete': {}, 'delete_0': coreapi.Link( url='/example/{id}/', action='delete', @@ -344,20 +329,17 @@ def test_schema_for_regular_views(self): title='Example API', content={ 'example': { - 'create': {}, - 'create_0': coreapi.Link( + 'post_0': coreapi.Link( url='/example/', action='post', fields=[] ), - 'list': {}, - 'list_0': coreapi.Link( + 'get_0': coreapi.Link( url='/example/', action='get', fields=[] ), - 'read': {}, - 'read_0': coreapi.Link( + 'get_1': coreapi.Link( url='/example/{id}/', action='get', fields=[ @@ -365,8 +347,7 @@ def test_schema_for_regular_views(self): ] ), 'sub': { - 'list': {}, - 'list_0': coreapi.Link( + 'get_0': coreapi.Link( url='/example/{id}/sub/', action='get', fields=[ @@ -401,20 +382,17 @@ def test_schema_for_regular_views(self): title='Example API', content={ 'example': { - 'create': {}, - 'create_0': coreapi.Link( + 'post_0': coreapi.Link( url='/api/v1/example/', action='post', fields=[] ), - 'list': {}, - 'list_0': coreapi.Link( + 'get_0': coreapi.Link( url='/api/v1/example/', action='get', fields=[] ), - 'read': {}, - 'read_0': coreapi.Link( + 'get_1': coreapi.Link( url='/api/v1/example/{id}/', action='get', fields=[ @@ -422,8 +400,7 @@ def test_schema_for_regular_views(self): ] ), 'sub': { - 'list': {}, - 'list_0': coreapi.Link( + 'get_0': coreapi.Link( url='/api/v1/example/{id}/sub/', action='get', fields=[ @@ -460,8 +437,7 @@ def test_schema_for_regular_views(self): title='Example API', content={ 'example1': { - 'list': {}, - 'list_0': coreapi.Link( + 'get_0': coreapi.Link( url='/example1/', action='get', fields=[ @@ -470,20 +446,17 @@ def test_schema_for_regular_views(self): coreapi.Field('ordering', required=False, location='query', schema=coreschema.String(title='Ordering', description='Which field to use when ordering the results.')) ] ), - 'custom_list_action': {}, - 'custom_list_action_0': coreapi.Link( + 'get_1': coreapi.Link( url='/example1/custom_list_action/', action='get' ), 'custom_list_action_multiple_methods': { - 'read': {}, - 'read_0': coreapi.Link( + 'get_0': coreapi.Link( url='/example1/custom_list_action_multiple_methods/', action='get' ) }, - 'read': {}, - 'read_0': coreapi.Link( + 'get_2': coreapi.Link( url='/example1/{id}/', action='get', fields=[ @@ -521,8 +494,7 @@ def test_schema_for_regular_views(self): title='Example API', content={ 'example': { - 'list': {}, - 'list_0': coreapi.Link( + 'get_0': coreapi.Link( url='/example/', action='get', fields=[] @@ -694,8 +666,7 @@ def test_schema_generator_excludes_correctly(self): title='Exclusions', content={ 'included-fbv': { - 'list': {}, - 'list_0': coreapi.Link(url='/included-fbv/', action='get') + 'get_0': coreapi.Link(url='/included-fbv/', action='get') } } ) @@ -820,10 +791,9 @@ def test_manually_routing_nested_routes(self): content={ 'test': { 'list': { - 'list': {}, - 'list_0': coreapi.Link(url='/test/list/', action='get') + 'get_0': coreapi.Link(url='/test/list/', action='get') }, - 'list_0': coreapi.Link(url='/test', action='get') + 'get_0': coreapi.Link(url='/test', action='get') } } ) @@ -832,7 +802,7 @@ def test_manually_routing_nested_routes(self): def _verify_cbv_links(self, loc, url, methods=None, number=0): if methods is None: - methods = ('read', 'update', 'partial_update', 'delete') + methods = ('get', 'put', 'patch', 'delete') for method in methods: key = '{}_{}'.format(method, number) @@ -868,20 +838,19 @@ def test_from_router(self): generator = SchemaGenerator(title='Naming Colisions', patterns=patterns) schema = generator.get_schema() - desc = schema['detail_0'].description # not important here + desc = schema['get_0'].description # not important here expected = coreapi.Document( url='', title='Naming Colisions', content={ 'detail': { - 'detail_export': {}, - 'detail_export_0': coreapi.Link( + 'get_0': coreapi.Link( url='/from-routercollision/detail/export/', action='get', description=desc) }, - 'detail_0': coreapi.Link( + 'get_0': coreapi.Link( url='/from-routercollision/detail/', action='get', description=desc @@ -900,9 +869,8 @@ def test_url_under_same_key_not_replaced(self): generator = SchemaGenerator(title='Naming Colisions', patterns=patterns) schema = generator.get_schema() - assert schema['example']['read'] == {} - assert schema['example']['read_0'].url == '/example/{id}/' - assert schema['example']['read_1'].url == '/example/{slug}/' + assert schema['example']['get_0'].url == '/example/{id}/' + assert schema['example']['get_1'].url == '/example/{slug}/' def test_url_under_same_key_not_replaced_another(self): @@ -914,5 +882,5 @@ def test_url_under_same_key_not_replaced_another(self): generator = SchemaGenerator(title='Naming Colisions', patterns=patterns) schema = generator.get_schema() - assert schema['test']['list']['list_0'].url == '/test/list/' - assert schema['test']['list']['list_1'].url == '/test/{id}/list/' + assert schema['test']['list']['get_0'].url == '/test/list/' + assert schema['test']['list']['get_1'].url == '/test/{id}/list/' From 95ed94ae60c2c1b1dc5fc31eede0c727d38a317f Mon Sep 17 00:00:00 2001 From: Marcin Lubimow Date: Sun, 8 Oct 2017 15:29:09 +0100 Subject: [PATCH 4/8] Fixed isort --- rest_framework/schemas/generators.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rest_framework/schemas/generators.py b/rest_framework/schemas/generators.py index 083f6bf7d8..d1b43d80ec 100644 --- a/rest_framework/schemas/generators.py +++ b/rest_framework/schemas/generators.py @@ -4,7 +4,7 @@ See schemas.__init__.py for package overview. """ import warnings -from collections import OrderedDict, Counter +from collections import Counter, OrderedDict from importlib import import_module from django.conf import settings From f1f657d24f448925850b0f257a2694488b405a27 Mon Sep 17 00:00:00 2001 From: Marcin Lubimow Date: Sun, 8 Oct 2017 15:37:27 +0100 Subject: [PATCH 5/8] Removed not needed cast --- rest_framework/schemas/generators.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rest_framework/schemas/generators.py b/rest_framework/schemas/generators.py index d1b43d80ec..4a2aeac366 100644 --- a/rest_framework/schemas/generators.py +++ b/rest_framework/schemas/generators.py @@ -106,7 +106,7 @@ def distribute_links(obj): distribute_links(value) for link in obj.links: - key = obj.get_next_key(str(link.action)) + key = obj.get_next_key(link.action) obj[key] = link From 50b3c05eb326a8d32cd08522c0a72e765355498f Mon Sep 17 00:00:00 2001 From: Marcin Lubimow Date: Mon, 9 Oct 2017 08:16:35 +0100 Subject: [PATCH 6/8] Fix for key collision --- rest_framework/schemas/generators.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/rest_framework/schemas/generators.py b/rest_framework/schemas/generators.py index 4a2aeac366..77e96b85f6 100644 --- a/rest_framework/schemas/generators.py +++ b/rest_framework/schemas/generators.py @@ -71,9 +71,13 @@ def __init__(self): super(LinkNode, self).__init__() def get_next_key(self, method): - current_val = self.methods_counter[method] - self.methods_counter[method] += 1 - return '{}_{}'.format(method, current_val) + while True: + current_val = self.methods_counter[method] + self.methods_counter[method] += 1 + + key = '{}_{}'.format(method, current_val) + if key not in self: + return key def insert_into(target, keys, value): From dcd64d09826c8f150ea40ef6512d168a74b48836 Mon Sep 17 00:00:00 2001 From: Marcin Lubimow Date: Mon, 9 Oct 2017 10:14:16 +0100 Subject: [PATCH 7/8] Added preferred key to preserve if available --- rest_framework/schemas/generators.py | 33 ++++++---- tests/test_schemas.py | 94 +++++++++++++++------------- 2 files changed, 71 insertions(+), 56 deletions(-) diff --git a/rest_framework/schemas/generators.py b/rest_framework/schemas/generators.py index 77e96b85f6..2f4dd63db9 100644 --- a/rest_framework/schemas/generators.py +++ b/rest_framework/schemas/generators.py @@ -70,12 +70,15 @@ def __init__(self): self.methods_counter = Counter() super(LinkNode, self).__init__() - def get_next_key(self, method): + def get_available_key(self, preferred_key): + if preferred_key not in self: + return preferred_key + while True: - current_val = self.methods_counter[method] - self.methods_counter[method] += 1 + current_val = self.methods_counter[preferred_key] + self.methods_counter[preferred_key] += 1 - key = '{}_{}'.format(method, current_val) + key = '{}_{}'.format(preferred_key, current_val) if key not in self: return key @@ -89,13 +92,13 @@ def insert_into(target, keys, value): >>> example LinkNode({'a': LinkNode({'b': LinkNode({'c': LinkNode(links=[123])}}}))) """ - for key in keys: + for key in keys[:-1]: if key not in target: target[key] = LinkNode() target = target[key] try: - target.links.append(value) + target.links.append((keys[-1], value)) except TypeError: msg = INSERT_INTO_COLLISION_FMT.format( value_url=value.url, @@ -109,8 +112,8 @@ def distribute_links(obj): for key, value in obj.items(): distribute_links(value) - for link in obj.links: - key = obj.get_next_key(link.action) + for preferred_key, link in obj.links: + key = obj.get_available_key(preferred_key) obj[key] = link @@ -436,8 +439,16 @@ def get_keys(self, subpath, method, view): if is_custom_action(action): # Custom action, eg "/users/{pk}/activate/", "/users/active/" - if len(view.action_map) == 1: - return named_path_components[:-1] + if len(view.action_map) > 1: + action = self.default_mapping[method.lower()] + if action in self.coerce_method_names: + action = self.coerce_method_names[action] + return named_path_components + [action] + else: + return named_path_components[:-1] + [action] + + if action in self.coerce_method_names: + action = self.coerce_method_names[action] # Default action, eg "/users/", "/users/{pk}/" - return named_path_components + return named_path_components + [action] diff --git a/tests/test_schemas.py b/tests/test_schemas.py index 08e4a023a8..eaace9a7b9 100644 --- a/tests/test_schemas.py +++ b/tests/test_schemas.py @@ -120,7 +120,7 @@ def test_anonymous_request(self): title='Example API', content={ 'example': { - 'get_0': coreapi.Link( + 'list': coreapi.Link( url='/example/', action='get', fields=[ @@ -129,17 +129,17 @@ def test_anonymous_request(self): coreapi.Field('ordering', required=False, location='query', schema=coreschema.String(title='Ordering', description='Which field to use when ordering the results.')) ] ), - 'get_1': coreapi.Link( + 'custom_list_action': coreapi.Link( url='/example/custom_list_action/', action='get' ), 'custom_list_action_multiple_methods': { - 'get_0': coreapi.Link( + 'read': coreapi.Link( url='/example/custom_list_action_multiple_methods/', action='get' ) }, - 'get_2': coreapi.Link( + 'read': coreapi.Link( url='/example/{id}/', action='get', fields=[ @@ -150,7 +150,6 @@ def test_anonymous_request(self): } } ) - assert response.data == expected def test_authenticated_request(self): @@ -163,7 +162,7 @@ def test_authenticated_request(self): title='Example API', content={ 'example': { - 'get_0': coreapi.Link( + 'list': coreapi.Link( url='/example/', action='get', fields=[ @@ -172,7 +171,7 @@ def test_authenticated_request(self): coreapi.Field('ordering', required=False, location='query', schema=coreschema.String(title='Ordering', description='Which field to use when ordering the results.')) ] ), - 'post_0': coreapi.Link( + 'create': coreapi.Link( url='/example/', action='post', encoding='application/json', @@ -181,7 +180,7 @@ def test_authenticated_request(self): coreapi.Field('b', required=False, location='form', schema=coreschema.String(title='B')) ] ), - 'get_2': coreapi.Link( + 'read': coreapi.Link( url='/example/{id}/', action='get', fields=[ @@ -189,7 +188,7 @@ def test_authenticated_request(self): coreapi.Field('ordering', required=False, location='query', schema=coreschema.String(title='Ordering', description='Which field to use when ordering the results.')) ] ), - 'post_1': coreapi.Link( + 'custom_action': coreapi.Link( url='/example/{id}/custom_action/', action='post', encoding='application/json', @@ -200,7 +199,7 @@ def test_authenticated_request(self): coreapi.Field('d', required=False, location='form', schema=coreschema.String(title='D')), ] ), - 'post_2': coreapi.Link( + 'custom_action_with_list_fields': coreapi.Link( url='/example/{id}/custom_action_with_list_fields/', action='post', encoding='application/json', @@ -211,21 +210,21 @@ def test_authenticated_request(self): coreapi.Field('b', required=True, location='form', schema=coreschema.Array(title='B', items=coreschema.String())), ] ), - 'get_1': coreapi.Link( + 'custom_list_action': coreapi.Link( url='/example/custom_list_action/', action='get' ), 'custom_list_action_multiple_methods': { - 'get_0': coreapi.Link( + 'read': coreapi.Link( url='/example/custom_list_action_multiple_methods/', action='get' ), - 'post_0': coreapi.Link( + 'create': coreapi.Link( url='/example/custom_list_action_multiple_methods/', action='post' ) }, - 'put_0': coreapi.Link( + 'update': coreapi.Link( url='/example/{id}/', action='put', encoding='application/json', @@ -236,7 +235,7 @@ def test_authenticated_request(self): coreapi.Field('ordering', required=False, location='query', schema=coreschema.String(title='Ordering', description='Which field to use when ordering the results.')) ] ), - 'patch_0': coreapi.Link( + 'partial_update': coreapi.Link( url='/example/{id}/', action='patch', encoding='application/json', @@ -247,7 +246,7 @@ def test_authenticated_request(self): coreapi.Field('ordering', required=False, location='query', schema=coreschema.String(title='Ordering', description='Which field to use when ordering the results.')) ] ), - 'delete_0': coreapi.Link( + 'delete': coreapi.Link( url='/example/{id}/', action='delete', fields=[ @@ -329,17 +328,17 @@ def test_schema_for_regular_views(self): title='Example API', content={ 'example': { - 'post_0': coreapi.Link( + 'create': coreapi.Link( url='/example/', action='post', fields=[] ), - 'get_0': coreapi.Link( + 'list': coreapi.Link( url='/example/', action='get', fields=[] ), - 'get_1': coreapi.Link( + 'read': coreapi.Link( url='/example/{id}/', action='get', fields=[ @@ -347,7 +346,7 @@ def test_schema_for_regular_views(self): ] ), 'sub': { - 'get_0': coreapi.Link( + 'list': coreapi.Link( url='/example/{id}/sub/', action='get', fields=[ @@ -382,17 +381,17 @@ def test_schema_for_regular_views(self): title='Example API', content={ 'example': { - 'post_0': coreapi.Link( + 'create': coreapi.Link( url='/api/v1/example/', action='post', fields=[] ), - 'get_0': coreapi.Link( + 'list': coreapi.Link( url='/api/v1/example/', action='get', fields=[] ), - 'get_1': coreapi.Link( + 'read': coreapi.Link( url='/api/v1/example/{id}/', action='get', fields=[ @@ -400,7 +399,7 @@ def test_schema_for_regular_views(self): ] ), 'sub': { - 'get_0': coreapi.Link( + 'list': coreapi.Link( url='/api/v1/example/{id}/sub/', action='get', fields=[ @@ -437,7 +436,7 @@ def test_schema_for_regular_views(self): title='Example API', content={ 'example1': { - 'get_0': coreapi.Link( + 'list': coreapi.Link( url='/example1/', action='get', fields=[ @@ -446,17 +445,17 @@ def test_schema_for_regular_views(self): coreapi.Field('ordering', required=False, location='query', schema=coreschema.String(title='Ordering', description='Which field to use when ordering the results.')) ] ), - 'get_1': coreapi.Link( + 'custom_list_action': coreapi.Link( url='/example1/custom_list_action/', action='get' ), 'custom_list_action_multiple_methods': { - 'get_0': coreapi.Link( + 'read': coreapi.Link( url='/example1/custom_list_action_multiple_methods/', action='get' ) }, - 'get_2': coreapi.Link( + 'read': coreapi.Link( url='/example1/{id}/', action='get', fields=[ @@ -494,7 +493,7 @@ def test_schema_for_regular_views(self): title='Example API', content={ 'example': { - 'get_0': coreapi.Link( + 'list': coreapi.Link( url='/example/', action='get', fields=[] @@ -666,7 +665,7 @@ def test_schema_generator_excludes_correctly(self): title='Exclusions', content={ 'included-fbv': { - 'get_0': coreapi.Link(url='/included-fbv/', action='get') + 'list': coreapi.Link(url='/included-fbv/', action='get') } } ) @@ -791,21 +790,26 @@ def test_manually_routing_nested_routes(self): content={ 'test': { 'list': { - 'get_0': coreapi.Link(url='/test/list/', action='get') + 'list': coreapi.Link(url='/test/list/', action='get') }, - 'get_0': coreapi.Link(url='/test', action='get') + 'list_0': coreapi.Link(url='/test', action='get') } } ) assert expected == schema - def _verify_cbv_links(self, loc, url, methods=None, number=0): + def _verify_cbv_links(self, loc, url, methods=None, suffixes=None): if methods is None: - methods = ('get', 'put', 'patch', 'delete') - - for method in methods: - key = '{}_{}'.format(method, number) + methods = ('read', 'update', 'partial_update', 'delete') + if suffixes is None: + suffixes = (None for m in methods) + + for method, suffix in zip(methods, suffixes): + if suffix is not None: + key = '{}_{}'.format(method, suffix) + else: + key = method assert loc[key].url == url def test_manually_routing_generic_view(self): @@ -829,7 +833,7 @@ def test_manually_routing_generic_view(self): self._verify_cbv_links(schema['test']['get'], '/test/get/') self._verify_cbv_links(schema['test']['update'], '/test/update/') self._verify_cbv_links(schema['test']['retrieve'], '/test/retrieve/') - self._verify_cbv_links(schema['test'], '/test') + self._verify_cbv_links(schema['test'], '/test', suffixes=(None, '0', None, '0')) def test_from_router(self): patterns = [ @@ -838,19 +842,19 @@ def test_from_router(self): generator = SchemaGenerator(title='Naming Colisions', patterns=patterns) schema = generator.get_schema() - desc = schema['get_0'].description # not important here + desc = schema['detail_0'].description # not important here expected = coreapi.Document( url='', title='Naming Colisions', content={ 'detail': { - 'get_0': coreapi.Link( + 'detail_export': coreapi.Link( url='/from-routercollision/detail/export/', action='get', description=desc) }, - 'get_0': coreapi.Link( + 'detail_0': coreapi.Link( url='/from-routercollision/detail/', action='get', description=desc @@ -869,8 +873,8 @@ def test_url_under_same_key_not_replaced(self): generator = SchemaGenerator(title='Naming Colisions', patterns=patterns) schema = generator.get_schema() - assert schema['example']['get_0'].url == '/example/{id}/' - assert schema['example']['get_1'].url == '/example/{slug}/' + assert schema['example']['read'].url == '/example/{id}/' + assert schema['example']['read_0'].url == '/example/{slug}/' def test_url_under_same_key_not_replaced_another(self): @@ -882,5 +886,5 @@ def test_url_under_same_key_not_replaced_another(self): generator = SchemaGenerator(title='Naming Colisions', patterns=patterns) schema = generator.get_schema() - assert schema['test']['list']['get_0'].url == '/test/list/' - assert schema['test']['list']['get_1'].url == '/test/{id}/list/' + assert schema['test']['list']['list'].url == '/test/list/' + assert schema['test']['list']['list_0'].url == '/test/{id}/list/' From 214cc40752b4d461d96cad261914e7076fa4ea62 Mon Sep 17 00:00:00 2001 From: Marcin Lubimow Date: Thu, 12 Oct 2017 11:50:49 +0100 Subject: [PATCH 8/8] Add accidently removed test --- tests/test_schemas.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/test_schemas.py b/tests/test_schemas.py index eaace9a7b9..5acabaf20d 100644 --- a/tests/test_schemas.py +++ b/tests/test_schemas.py @@ -19,6 +19,7 @@ AutoSchema, ManualSchema, SchemaGenerator, get_schema_view ) from rest_framework.schemas.generators import EndpointEnumerator +from rest_framework.schemas.utils import is_list_view from rest_framework.test import APIClient, APIRequestFactory from rest_framework.utils import formatting from rest_framework.views import APIView @@ -888,3 +889,15 @@ def test_url_under_same_key_not_replaced_another(self): assert schema['test']['list']['list'].url == '/test/list/' assert schema['test']['list']['list_0'].url == '/test/{id}/list/' + + +def test_is_list_view_recognises_retrieve_view_subclasses(): + class TestView(generics.RetrieveAPIView): + pass + + path = '/looks/like/a/list/view/' + method = 'get' + view = TestView() + + is_list = is_list_view(path, method, view) + assert not is_list, "RetrieveAPIView subclasses should not be classified as list views."