Enforce unified list field consistency

Discovered via bug: controller_node field present in
project update list but not in detail view

Added tests to assert that "unified" list serializer
produces same fields as the ordinary serializer,
for unified jobs & unified JTs

Added test to check that list serializers do differ
from detail serializer except for allowed differences

Added test to check that list serializers are applied
correctly - only on list views, and that detail views,
likewise, do not use list serializers

Fix the many many bugs discovered by these new
testing mechanisms

awx/api/serializers.py

@@ -671,7 +671,7 @@ class UnifiedJobTemplateSerializer(BaseSerializer):
         else:
             return super(UnifiedJobTemplateSerializer, self).get_types()
 
-    def to_representation(self, obj):
+    def get_sub_serializer(self, obj):
         serializer_class = None
         if type(self) is UnifiedJobTemplateSerializer:
             if isinstance(obj, Project):
@@ -684,6 +684,10 @@ class UnifiedJobTemplateSerializer(BaseSerializer):
                 serializer_class = SystemJobTemplateSerializer
             elif isinstance(obj, WorkflowJobTemplate):
                 serializer_class = WorkflowJobTemplateSerializer
+        return serializer_class
+
+    def to_representation(self, obj):
+        serializer_class = self.get_sub_serializer(obj)
         if serializer_class:
             serializer = serializer_class(instance=obj, context=self.context)
             # preserve links for list view
@@ -766,7 +770,7 @@ class UnifiedJobSerializer(BaseSerializer):
 
         return summary_fields
 
-    def to_representation(self, obj):
+    def get_sub_serializer(self, obj):
         serializer_class = None
         if type(self) is UnifiedJobSerializer:
             if isinstance(obj, ProjectUpdate):
@@ -781,6 +785,10 @@ class UnifiedJobSerializer(BaseSerializer):
                 serializer_class = SystemJobSerializer
             elif isinstance(obj, WorkflowJob):
                 serializer_class = WorkflowJobSerializer
+        return serializer_class
+
+    def to_representation(self, obj):
+        serializer_class = self.get_sub_serializer(obj)
         if serializer_class:
             serializer = serializer_class(instance=obj, context=self.context)
             # preserve links for list view
@@ -818,7 +826,7 @@ class UnifiedJobListSerializer(UnifiedJobSerializer):
         else:
             return super(UnifiedJobListSerializer, self).get_types()
 
-    def to_representation(self, obj):
+    def get_sub_serializer(self, obj):
         serializer_class = None
         if type(self) is UnifiedJobListSerializer:
             if isinstance(obj, ProjectUpdate):
@@ -832,7 +840,11 @@ class UnifiedJobListSerializer(UnifiedJobSerializer):
             elif isinstance(obj, SystemJob):
                 serializer_class = SystemJobListSerializer
             elif isinstance(obj, WorkflowJob):
-                serializer_class = WorkflowJobSerializer
+                serializer_class = WorkflowJobListSerializer
+        return serializer_class
+
+    def to_representation(self, obj):
+        serializer_class = self.get_sub_serializer(obj)
         if serializer_class:
             serializer = serializer_class(instance=obj, context=self.context)
             ret = serializer.to_representation(obj)
@@ -1480,7 +1492,9 @@ class ProjectUpdateDetailSerializer(ProjectUpdateSerializer):
 
 class ProjectUpdateListSerializer(ProjectUpdateSerializer, UnifiedJobListSerializer):
 
-    pass
+    class Meta:
+        model = ProjectUpdate
+        fields = ('*', '-controller_node')  # field removal undone by UJ serializer
 
 
 class ProjectUpdateCancelSerializer(ProjectUpdateSerializer):
@@ -2200,7 +2214,9 @@ class InventoryUpdateSerializer(UnifiedJobSerializer, InventorySourceOptionsSeri
 
 class InventoryUpdateListSerializer(InventoryUpdateSerializer, UnifiedJobListSerializer):
 
-    pass
+    class Meta:
+        model = InventoryUpdate
+        fields = ('*', '-controller_node')  # field removal undone by UJ serializer
 
 
 class InventoryUpdateCancelSerializer(InventoryUpdateSerializer):
@@ -3547,12 +3563,6 @@ class WorkflowJobTemplateSerializer(JobTemplateMixin, LabelsListMixin, UnifiedJo
         return vars_validate_or_raise(value)
 
 
-# TODO:
-class WorkflowJobTemplateListSerializer(WorkflowJobTemplateSerializer):
-    pass
-
-
-# TODO:
 class WorkflowJobSerializer(LabelsListMixin, UnifiedJobSerializer):
 
     class Meta:
@@ -3583,7 +3593,6 @@ class WorkflowJobSerializer(LabelsListMixin, UnifiedJobSerializer):
         return ret
 
 
-# TODO:
 class WorkflowJobListSerializer(WorkflowJobSerializer, UnifiedJobListSerializer):
 
     class Meta:
@@ -3866,7 +3875,10 @@ class AdHocCommandListSerializer(AdHocCommandSerializer, UnifiedJobListSerialize
 
 
 class SystemJobListSerializer(SystemJobSerializer, UnifiedJobListSerializer):
-    pass
+
+    class Meta:
+        model = SystemJob
+        fields = ('*', '-controller_node')  # field removal undone by UJ serializer
 
 
 class JobHostSummarySerializer(BaseSerializer):

awx/api/views.py

@@ -642,7 +642,7 @@ class InstanceUnifiedJobsList(SubListAPIView):
 
     view_name = _("Instance Jobs")
     model = UnifiedJob
-    serializer_class = UnifiedJobSerializer
+    serializer_class = UnifiedJobListSerializer
     parent_model = Instance
 
     def get_queryset(self):
@@ -689,7 +689,7 @@ class InstanceGroupUnifiedJobsList(SubListAPIView):
 
     view_name = _("Instance Group Running Jobs")
     model = UnifiedJob
-    serializer_class = UnifiedJobSerializer
+    serializer_class = UnifiedJobListSerializer
     parent_model = InstanceGroup
     relationship = "unifiedjob_set"
 
@@ -800,7 +800,7 @@ class ScheduleCredentialsList(LaunchConfigCredentialsBase):
 class ScheduleUnifiedJobsList(SubListAPIView):
 
     model = UnifiedJob
-    serializer_class = UnifiedJobSerializer
+    serializer_class = UnifiedJobListSerializer
     parent_model = Schedule
     relationship = 'unifiedjob_set'
     view_name = _('Schedule Jobs List')
@@ -1058,7 +1058,7 @@ class OrganizationProjectsList(SubListCreateAttachDetachAPIView):
 class OrganizationWorkflowJobTemplatesList(SubListCreateAttachDetachAPIView):
 
     model = WorkflowJobTemplate
-    serializer_class = WorkflowJobTemplateListSerializer
+    serializer_class = WorkflowJobTemplateSerializer
     parent_model = Organization
     relationship = 'workflows'
     parent_key = 'organization'
@@ -1380,7 +1380,7 @@ class ProjectNotificationTemplatesSuccessList(SubListCreateAttachDetachAPIView):
 class ProjectUpdatesList(SubListAPIView):
 
     model = ProjectUpdate
-    serializer_class = ProjectUpdateSerializer
+    serializer_class = ProjectUpdateListSerializer
     parent_model = Project
     relationship = 'project_updates'
 
@@ -1491,7 +1491,7 @@ class ProjectUpdateScmInventoryUpdates(SubListCreateAPIView):
 
     view_name = _("Project Update SCM Inventory Updates")
     model = InventoryUpdate
-    serializer_class = InventoryUpdateSerializer
+    serializer_class = InventoryUpdateListSerializer
     parent_model = ProjectUpdate
     relationship = 'scm_inventory_updates'
     parent_key = 'source_project_update'
@@ -2830,7 +2830,7 @@ class InventorySourceGroupsList(SubListDestroyAPIView):
 class InventorySourceUpdatesList(SubListAPIView):
 
     model = InventoryUpdate
-    serializer_class = InventoryUpdateSerializer
+    serializer_class = InventoryUpdateListSerializer
     parent_model = InventorySource
     relationship = 'inventory_updates'
 
@@ -3701,7 +3701,7 @@ class WorkflowJobNodeAlwaysNodesList(WorkflowJobNodeChildrenBaseList):
 class WorkflowJobTemplateList(WorkflowsEnforcementMixin, ListCreateAPIView):
 
     model = WorkflowJobTemplate
-    serializer_class = WorkflowJobTemplateListSerializer
+    serializer_class = WorkflowJobTemplateSerializer
     always_allow_superuser = False
 
 

awx/main/tests/unit/api/serializers/test_unified_serializers.py

@@ -0,0 +1,69 @@
+# AWX
+from awx.api import serializers
+from awx.main.models import UnifiedJob, UnifiedJobTemplate
+
+# DRF
+from rest_framework.generics import ListAPIView
+
+
+def test_unified_template_field_consistency():
+    '''
+    Example of what is being tested:
+    The endpoints /projects/N/ and /projects/ should have the same fields as
+    that same project when it is serialized by the unified job template serializer
+    in /unified_job_templates/
+    '''
+    for cls in UnifiedJobTemplate.__subclasses__():
+        detail_serializer = getattr(serializers, '{}Serializer'.format(cls.__name__))
+        unified_serializer = serializers.UnifiedJobTemplateSerializer().get_sub_serializer(cls())
+        assert set(detail_serializer().fields.keys()) == set(unified_serializer().fields.keys())
+
+
+def test_unified_job_list_field_consistency():
+    '''
+    Example of what is being tested:
+    The endpoint /project_updates/ should have the same fields as that
+    project update when it is serialized by the unified job template serializer
+    in /unified_jobs/
+    '''
+    for cls in UnifiedJob.__subclasses__():
+        list_serializer = getattr(serializers, '{}ListSerializer'.format(cls.__name__))
+        unified_serializer = serializers.UnifiedJobListSerializer().get_sub_serializer(cls())
+        assert set(list_serializer().fields.keys()) == set(unified_serializer().fields.keys()), (
+            'Mismatch between {} list serializer & unified list serializer'.format(cls)
+        )
+
+
+def test_unified_job_detail_exclusive_fields():
+    '''
+    For each type, assert that the only fields allowed to be exclusive to
+    detail view are the allowed types
+    '''
+    allowed_detail_fields = frozenset(
+        ('result_traceback', 'job_args', 'job_cwd', 'job_env', 'event_processing_finished')
+    )
+    for cls in UnifiedJob.__subclasses__():
+        list_serializer = getattr(serializers, '{}ListSerializer'.format(cls.__name__))
+        detail_serializer = getattr(serializers, '{}Serializer'.format(cls.__name__))
+        list_fields = set(list_serializer().fields.keys())
+        detail_fields = set(detail_serializer().fields.keys()) - allowed_detail_fields
+        assert list_fields == detail_fields, 'List / detail mismatch for serializers of {}'.format(cls)
+
+
+def test_list_views_use_list_serializers(all_views):
+    '''
+    Check that the list serializers are only used for list views,
+    and vice versa
+    '''
+    list_serializers = tuple(
+        getattr(serializers, '{}ListSerializer'.format(cls.__name__)) for
+        cls in (UnifiedJob.__subclasses__() + [UnifiedJob])
+    )
+    for View in all_views:
+        if hasattr(View, 'model') and issubclass(getattr(View, 'model'), UnifiedJob):
+            if issubclass(View, ListAPIView):
+                assert issubclass(View.serializer_class, list_serializers), (
+                    'View {} serializer {} is not a list serializer'.format(View, View.serializer_class)
+                )
+            else:
+                assert not issubclass(View.model, list_serializers)

awx/main/tests/unit/conftest.py

@@ -3,6 +3,11 @@ import logging
 
 from mock import PropertyMock
 
+from awx.api.urls import urlpatterns as api_patterns
+
+# Django
+from django.core.urlresolvers import RegexURLResolver, RegexURLPattern
+
 
 @pytest.fixture(autouse=True)
 def _disable_database_settings(mocker):
@@ -10,6 +15,33 @@ def _disable_database_settings(mocker):
     m.return_value = []
 
 
+@pytest.fixture()
+def all_views():
+    '''
+    returns a set of all views in the app
+    '''
+    patterns = set([])
+    url_views = set([])
+    # Add recursive URL patterns
+    unprocessed = set(api_patterns)
+    while unprocessed:
+        to_process = unprocessed.copy()
+        unprocessed = set([])
+        for pattern in to_process:
+            if hasattr(pattern, 'lookup_str') and not pattern.lookup_str.startswith('awx.api'):
+                continue
+            patterns.add(pattern)
+            if isinstance(pattern, RegexURLResolver):
+                for sub_pattern in pattern.url_patterns:
+                    if sub_pattern not in patterns:
+                        unprocessed.add(sub_pattern)
+    # Get view classes
+    for pattern in patterns:
+        if isinstance(pattern, RegexURLPattern) and hasattr(pattern.callback, 'view_class'):
+            url_views.add(pattern.callback.view_class)
+    return url_views
+
+
 @pytest.fixture()
 def dummy_log_record():
     return logging.LogRecord(

awx/main/tests/unit/test_views.py

@@ -5,9 +5,6 @@ import mock
 from rest_framework import exceptions
 from rest_framework.generics import ListAPIView
 
-# Django
-from django.core.urlresolvers import RegexURLResolver, RegexURLPattern
-
 # AWX
 from awx.main.views import ApiErrorView
 from awx.api.views import JobList, InventorySourceList
@@ -58,33 +55,12 @@ def test_disable_post_on_v1_inventory_source_list(version, supports_post):
         assert ('POST' in inv_source_list.allowed_methods) == supports_post
 
 
-def test_views_have_search_fields():
+def test_views_have_search_fields(all_views):
-    from awx.api.urls import urlpatterns as api_patterns
-    patterns = set([])
-    url_views = set([])
-    # Add recursive URL patterns
-    unprocessed = set(api_patterns)
-    while unprocessed:
-        to_process = unprocessed.copy()
-        unprocessed = set([])
-        for pattern in to_process:
-            if hasattr(pattern, 'lookup_str') and not pattern.lookup_str.startswith('awx.api'):
-                continue
-            patterns.add(pattern)
-            if isinstance(pattern, RegexURLResolver):
-                for sub_pattern in pattern.url_patterns:
-                    if sub_pattern not in patterns:
-                        unprocessed.add(sub_pattern)
-    # Get view classes
-    for pattern in patterns:
-        if isinstance(pattern, RegexURLPattern) and hasattr(pattern.callback, 'view_class'):
-            cls = pattern.callback.view_class
-            if issubclass(cls, ListAPIView):
-                url_views.add(pattern.callback.view_class)
-
     # Gather any views that don't have search fields defined
     views_missing_search = []
-    for View in url_views:
+    for View in all_views:
+        if not issubclass(View, ListAPIView):
+            continue
         view = View()
         if not hasattr(view, 'search_fields') or len(view.search_fields) == 0:
             views_missing_search.append(view)