Fixed issues with schema name collisions (#5486)

* Fixed issues with schema name collisions

* Fixed mutating issues in python 3

* Optimized solution

* Fixed isort

* Removed not needed cast

* Fix for key collision

* Added preferred key to preserve if available

* Add accidently removed test

Author: mlubimow

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 Counter, OrderedDict
 from importlib import import_module
 
 from django.conf import settings
@@ -64,21 +64,41 @@ def is_api_view(callback):
 """
 
 
+class LinkNode(OrderedDict):
+    def __init__(self):
+        self.links = []
+        self.methods_counter = Counter()
+        super(LinkNode, self).__init__()
+
+    def get_available_key(self, preferred_key):
+        if preferred_key not in self:
+            return preferred_key
+
+        while True:
+            current_val = self.methods_counter[preferred_key]
+            self.methods_counter[preferred_key] += 1
+
+            key = '{}_{}'.format(preferred_key, current_val)
+            if key not in self:
+                return key
+
+
 def insert_into(target, keys, value):
     """
     Nested dictionary insertion.
 
     >>> 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]:
         if key not in target:
-            target[key] = {}
+            target[key] = LinkNode()
         target = target[key]
+
     try:
-        target[keys[-1]] = value
+        target.links.append((keys[-1], value))
     except TypeError:
         msg = INSERT_INTO_COLLISION_FMT.format(
             value_url=value.url,
@@ -88,6 +108,15 @@ def insert_into(target, keys, value):
         raise ValueError(msg)
 
 
+def distribute_links(obj):
+    for key, value in obj.items():
+        distribute_links(value)
+
+    for preferred_key, link in obj.links:
+        key = obj.get_available_key(preferred_key)
+        obj[key] = link
+
+
 def is_custom_action(action):
     return action not in set([
         'retrieve', 'list', 'create', 'update', 'partial_update', 'destroy'
@@ -255,6 +284,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 +295,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 +318,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...

tests/test_schemas.py

@@ -749,6 +749,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 +783,35 @@ def test_manually_routing_nested_routes(self):
         ]
 
         generator = SchemaGenerator(title='Naming Colisions', patterns=patterns)
+        schema = generator.get_schema()
 
-        with pytest.raises(ValueError):
-            generator.get_schema()
+        expected = coreapi.Document(
+            url='',
+            title='Naming Colisions',
+            content={
+                'test': {
+                    'list': {
+                        'list': 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, suffixes=None):
+        if methods is None:
+            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):
         patterns = [
@@ -797,18 +827,68 @@ 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', suffixes=(None, '0', None, '0'))
 
     def test_from_router(self):
         patterns = [
             url(r'from-router', include(naming_collisions_router.urls)),
         ]
 
         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': 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<pk>\d+)/$', BasicNamingCollisionView.as_view()),
+            url(r'example/(?P<slug>\w+)/$', BasicNamingCollisionView.as_view()),
+        ]
+
+        generator = SchemaGenerator(title='Naming Colisions', patterns=patterns)
+        schema = generator.get_schema()
+
+        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):
+
+        patterns = [
+            url(r'^test/list/', simple_fbv),
+            url(r'^test/(?P<pk>\d+)/list/', simple_fbv),
+        ]
+
+        generator = SchemaGenerator(title='Naming Colisions', patterns=patterns)
+        schema = generator.get_schema()
 
-        with pytest.raises(ValueError):
-            generator.get_schema()
+        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():