Repository: Yelp/swagger_spec_validator

Issue:
Validator does not check uniqueness of operation ids
According to the Swagger spec, the `operationId` of an operation object is:

> Unique string used to identify the operation. The id MUST be unique among all operations described in the API. Tools and libraries MAY use the operationId to uniquely identify an operation, therefore, it is recommended to follow common programming naming conventions.

The validator does not currently check that `operationId`s are unique across the API.  This would be a helpful feature because some codegen tools fail if this constraint is not met.

Discussion:

Patch:
diff --git a/swagger_spec_validator/validator20.py b/swagger_spec_validator/validator20.py
index fe17ded..77920c1 100644
--- a/swagger_spec_validator/validator20.py
+++ b/swagger_spec_validator/validator20.py
@@ -7,6 +7,7 @@ from __future__ import unicode_literals
 import functools
 import logging
 import string
+from collections import defaultdict
 
 from jsonschema.validators import Draft4Validator
 from jsonschema.validators import RefResolver
@@ -196,6 +197,8 @@ def validate_apis(apis, deref):
     :raises: :py:class:`swagger_spec_validator.SwaggerValidationError`
     :raises: :py:class:`jsonschema.exceptions.ValidationError`
     """
+    operation_tag_to_operation_id_set = defaultdict(set)
+
     for api_name, api_body in iteritems(apis):
         api_body = deref(api_body)
         api_params = deref(api_body.get('parameters', []))
@@ -206,6 +209,20 @@ def validate_apis(apis, deref):
             if oper_name == 'parameters' or oper_name.startswith('x-'):
                 continue
             oper_body = deref(api_body[oper_name])
+            oper_tags = deref(oper_body.get('tags', [None]))
+
+            # Check that, if this operation has an operationId defined,
+            # no other operation with a same tag also has that
+            # operationId.
+            operation_id = oper_body.get('operationId')
+            if operation_id is not None:
+                for oper_tag in oper_tags:
+                    if operation_id in operation_tag_to_operation_id_set[oper_tag]:
+                        raise SwaggerValidationError(
+                            "Duplicate operationId: {}".format(operation_id)
+                        )
+                    operation_tag_to_operation_id_set[oper_tag].add(operation_id)
+
             oper_params = deref(oper_body.get('parameters', []))
             validate_duplicate_param(oper_params, deref)
             all_path_params = list(set(


Test patch:
diff --git a/tests/validator20/validate_apis_test.py b/tests/validator20/validate_apis_test.py
index 56f1e14..d1198d6 100644
--- a/tests/validator20/validate_apis_test.py
+++ b/tests/validator20/validate_apis_test.py
@@ -152,3 +152,91 @@ def test_api_check_default_fails(partial_parameter_spec, validator, instance):
     validation_error = excinfo.value.args[1]
     assert validation_error.instance == instance
     assert validation_error.validator == validator
+
+
+@pytest.mark.parametrize(
+    'apis',
+    [
+        {
+            '/api': {
+                'get': {
+                    'operationId': 'duplicateOperationId',
+                    'responses': {},
+                },
+                'post': {
+                    'operationId': 'duplicateOperationId',
+                    'responses': {},
+                },
+            },
+        },
+        {
+            '/api1': {
+                'get': {
+                    'operationId': 'duplicateOperationId',
+                    'responses': {},
+                },
+            },
+            '/api2': {
+                'get': {
+                    'operationId': 'duplicateOperationId',
+                    'responses': {},
+                },
+            },
+        },
+        {
+            '/api1': {
+                'get': {
+                    'operationId': 'duplicateOperationId',
+                    'tags': ['tag1', 'tag2'],
+                    'responses': {},
+                },
+            },
+            '/api2': {
+                'get': {
+                    'operationId': 'duplicateOperationId',
+                    'tags': ['tag1'],
+                    'responses': {},
+                },
+            },
+        },
+    ]
+)
+def test_duplicate_operationIds_fails(apis):
+    with pytest.raises(SwaggerValidationError) as excinfo:
+        validate_apis(apis, lambda x: x)
+
+    swagger_validation_error = excinfo.value
+    error_message = swagger_validation_error.args[0]
+
+    assert error_message == "Duplicate operationId: duplicateOperationId"
+
+
+@pytest.mark.parametrize(
+    'apis',
+    [
+        {
+            '/api1': {
+                'get': {
+                    'operationId': 'duplicateOperationId',
+                    'tags': ['tag1'],
+                    'responses': {},
+                },
+            },
+            '/api2': {
+                'get': {
+                    'operationId': 'duplicateOperationId',
+                    'tags': ['tag2'],
+                    'responses': {},
+                },
+            },
+            '/api3': {
+                'get': {
+                    'operationId': 'duplicateOperationId',
+                    'responses': {},
+                },
+            },
+        },
+    ]
+)
+def test_duplicate_operationIds_succeeds_if_tags_differ(apis):
+    validate_apis(apis, lambda x: x)
diff --git a/tests/validator20/validate_rich_spec_test.py b/tests/validator20/validate_rich_spec_test.py
index 593b9c0..0f5df04 100644
--- a/tests/validator20/validate_rich_spec_test.py
+++ b/tests/validator20/validate_rich_spec_test.py
@@ -36,10 +36,11 @@ def test_failure_on_duplicate_operation_parameters(swagger_spec):
 
 
 def test_failure_on_unresolvable_path_parameter(swagger_spec):
-    swagger_spec['paths']['/pet/{foo}'] = swagger_spec['paths']['/pet']
+    swagger_spec['paths']['/pet/{petId}']['get']['parameters'] = []
+
     with pytest.raises(SwaggerValidationError) as exc_info:
         validate_spec(swagger_spec)
-    assert "Path parameter 'foo' used is not documented on '/pet/{foo}'" in str(exc_info.value)
+    assert "Path parameter 'petId' used is not documented on '/pet/{petId}'" in str(exc_info.value)
 
 
 def test_failure_on_path_parameter_used_but_not_defined(swagger_spec):