Refactor and standardize how files are added to playbooks

Files are now (once again) related to playbooks.
We expect the playbook to be created first and then it's files are
created and associated to it.

- the /api/v1/playbook/<id>/files endpoint was removed
- the playbook id is now required when doing a POST on /api/v1/files
- playbook.file referenced a file object and no longer exists.
- playbook.file was replaced by "path" which is the path for the playbook
  file.
- playbook.files still exists but, from an API standpoint, this means
  that to find the files associated to a playbook, we can now do
  something like:

    "/api/v1/files?playbook=%s" % playbook.id

To find the playbook file itself, we can do something like:

    "/api/v1/files?playbook=%s&path=%s" % (playbook.id, playbook.path)

Change-Id: Id51129757e1626313caee4005b081027e5694aba

ara/api/migrations/0001_initial.py

@@ -1,4 +1,4 @@
-# Generated by Django 2.1.3 on 2018-11-14 19:40
+# Generated by Django 2.1.4 on 2018-12-18 19:29
 
 from django.db import migrations, models
 import django.db.models.deletion
@@ -93,8 +93,7 @@ class Migration(migrations.Migration):
                 ('ansible_version', models.CharField(max_length=255)),
                 ('status', models.CharField(choices=[('unknown', 'unknown'), ('running', 'running'), ('completed', 'completed'), ('failed', 'failed')], default='unknown', max_length=25)),
                 ('arguments', models.BinaryField(max_length=4294967295)),
-                ('file', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='playbooks', to='api.File')),
+                ('path', models.CharField(max_length=255)),
-                ('files', models.ManyToManyField(to='api.File')),
                 ('labels', models.ManyToManyField(to='api.Label')),
             ],
             options={
@@ -193,6 +192,11 @@ class Migration(migrations.Migration):
             name='content',
             field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='files', to='api.FileContent'),
         ),
+        migrations.AddField(
+            model_name='file',
+            name='playbook',
+            field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='files', to='api.Playbook'),
+        ),
         migrations.AlterUniqueTogether(
             name='stats',
             unique_together={('host', 'playbook')},
@@ -205,4 +209,8 @@ class Migration(migrations.Migration):
             name='host',
             unique_together={('name', 'playbook')},
         ),
+        migrations.AlterUniqueTogether(
+            name='file',
+            unique_together={('path', 'playbook')},
+        ),
     ]

ara/api/models.py

@@ -44,39 +44,6 @@ class Duration(Base):
     ended = models.DateTimeField(blank=True, null=True)
 
 
-class FileContent(Base):
-    """
-    Contents of a uniquely stored and compressed file.
-    Running the same playbook twice will yield two playbook files but just
-    one file contents.
-    """
-
-    class Meta:
-        db_table = "file_contents"
-
-    sha1 = models.CharField(max_length=40, unique=True)
-    contents = models.BinaryField(max_length=(2 ** 32) - 1)
-
-    def __str__(self):
-        return "<FileContent %s:%s>" % (self.id, self.sha1)
-
-
-class File(Base):
-    """
-    Data about Ansible files (playbooks, tasks, role files, var files, etc).
-    Multiple files can reference the same FileContent record.
-    """
-
-    class Meta:
-        db_table = "files"
-
-    path = models.CharField(max_length=255)
-    content = models.ForeignKey(FileContent, on_delete=models.CASCADE, related_name="files")
-
-    def __str__(self):
-        return "<File %s:%s>" % (self.id, self.path)
-
-
 class Label(Base):
     """
     A label is a generic container meant to group or correlate different
@@ -120,14 +87,48 @@ class Playbook(Duration):
     ansible_version = models.CharField(max_length=255)
     status = models.CharField(max_length=25, choices=STATUS, default=UNKNOWN)
     arguments = models.BinaryField(max_length=(2 ** 32) - 1)
-    file = models.ForeignKey(File, on_delete=models.CASCADE, related_name="playbooks")
+    path = models.CharField(max_length=255)
-    files = models.ManyToManyField(File)
     labels = models.ManyToManyField(Label)
 
     def __str__(self):
         return "<Playbook %s>" % self.id
 
 
+class FileContent(Base):
+    """
+    Contents of a uniquely stored and compressed file.
+    Running the same playbook twice will yield two playbook files but just
+    one file contents.
+    """
+
+    class Meta:
+        db_table = "file_contents"
+
+    sha1 = models.CharField(max_length=40, unique=True)
+    contents = models.BinaryField(max_length=(2 ** 32) - 1)
+
+    def __str__(self):
+        return "<FileContent %s:%s>" % (self.id, self.sha1)
+
+
+class File(Base):
+    """
+    Data about Ansible files (playbooks, tasks, role files, var files, etc).
+    Multiple files can reference the same FileContent record.
+    """
+
+    class Meta:
+        db_table = "files"
+        unique_together = ("path", "playbook")
+
+    path = models.CharField(max_length=255)
+    content = models.ForeignKey(FileContent, on_delete=models.CASCADE, related_name="files")
+    playbook = models.ForeignKey(Playbook, on_delete=models.CASCADE, related_name="files")
+
+    def __str__(self):
+        return "<File %s:%s>" % (self.id, self.path)
+
+
 class Record(Base):
     """
     A rudimentary key/value table to associate arbitrary data to a playbook.

ara/api/serializers.py

@@ -157,16 +157,11 @@ class PlaybookSerializer(DurationSerializer):
         fields = "__all__"
 
     arguments = CompressedObjectField(default=zlib.compress(json.dumps({}).encode("utf8")))
-    file = FileSerializer()
     files = FileSerializer(many=True, default=[])
     hosts = HostSerializer(many=True, default=[])
     labels = LabelSerializer(many=True, default=[])
 
     def create(self, validated_data):
-        # Create the file for the playbook
-        file_dict = validated_data.pop("file")
-        validated_data["file"] = models.File.objects.create(**file_dict)
-
         # Create the playbook without the file and label references for now
         files = validated_data.pop("files")
         hosts = validated_data.pop("hosts")
@@ -174,8 +169,8 @@ class PlaybookSerializer(DurationSerializer):
         playbook = models.Playbook.objects.create(**validated_data)
 
         # Add the files, hosts and the labels in
-        for file in files:
+        for file_ in files:
-            playbook.files.add(models.File.objects.create(**file))
+            playbook.hosts.add(models.File.objects.create(**file_))
         for host in hosts:
             playbook.hosts.add(models.Host.objects.create(**host))
         for label in labels:

ara/api/tests/factories.py

@@ -29,6 +29,16 @@ LABEL_DESCRIPTION = "label description"
 TASK_TAGS = ["always", "never"]
 
 
+class PlaybookFactory(factory.DjangoModelFactory):
+    class Meta:
+        model = models.Playbook
+
+    ansible_version = "2.4.0"
+    status = "running"
+    arguments = utils.compressed_obj(PLAYBOOK_ARGUMENTS)
+    path = "/path/playbook.yml"
+
+
 class FileContentFactory(factory.DjangoModelFactory):
     class Meta:
         model = models.FileContent
@@ -44,6 +54,7 @@ class FileFactory(factory.DjangoModelFactory):
 
     path = "/path/playbook.yml"
     content = factory.SubFactory(FileContentFactory)
+    playbook = factory.SubFactory(PlaybookFactory)
 
 
 class LabelFactory(factory.DjangoModelFactory):
@@ -54,16 +65,6 @@ class LabelFactory(factory.DjangoModelFactory):
     description = utils.compressed_str(LABEL_DESCRIPTION)
 
 
-class PlaybookFactory(factory.DjangoModelFactory):
-    class Meta:
-        model = models.Playbook
-
-    ansible_version = "2.4.0"
-    status = "running"
-    arguments = utils.compressed_obj(PLAYBOOK_ARGUMENTS)
-    file = factory.SubFactory(FileFactory)
-
-
 class PlayFactory(factory.DjangoModelFactory):
     class Meta:
         model = models.Play

ara/api/tests/tests_file.py

@@ -29,22 +29,26 @@ class FileTestCase(APITestCase):
         self.assertEqual(file.content.sha1, file_content.sha1)
 
     def test_file_serializer(self):
-        serializer = serializers.FileSerializer(data={"path": "/path/playbook.yml", "content": factories.FILE_CONTENTS})
+        playbook = factories.PlaybookFactory()
+        serializer = serializers.FileSerializer(
+            data={"path": "/path/playbook.yml", "content": factories.FILE_CONTENTS, "playbook": playbook.id}
+        )
         serializer.is_valid()
         file = serializer.save()
         file.refresh_from_db()
         self.assertEqual(file.content.sha1, utils.sha1(factories.FILE_CONTENTS))
 
     def test_create_file_with_same_content_create_only_one_file_content(self):
+        playbook = factories.PlaybookFactory()
         serializer = serializers.FileSerializer(
-            data={"path": "/path/1/playbook.yml", "content": factories.FILE_CONTENTS}
+            data={"path": "/path/1/playbook.yml", "content": factories.FILE_CONTENTS, "playbook": playbook.id}
         )
         serializer.is_valid()
         file_content = serializer.save()
         file_content.refresh_from_db()
 
         serializer2 = serializers.FileSerializer(
-            data={"path": "/path/2/playbook.yml", "content": factories.FILE_CONTENTS}
+            data={"path": "/path/2/playbook.yml", "content": factories.FILE_CONTENTS, "playbook": playbook.id}
         )
         serializer2.is_valid()
         file_content = serializer2.save()
@@ -55,7 +59,10 @@ class FileTestCase(APITestCase):
 
     def test_create_file(self):
         self.assertEqual(0, models.File.objects.count())
-        request = self.client.post("/api/v1/files", {"path": "/path/playbook.yml", "content": factories.FILE_CONTENTS})
+        playbook = factories.PlaybookFactory()
+        request = self.client.post(
+            "/api/v1/files", {"path": "/path/playbook.yml", "content": factories.FILE_CONTENTS, "playbook": playbook.id}
+        )
         self.assertEqual(201, request.status_code)
         self.assertEqual(1, models.File.objects.count())
 
@@ -76,11 +83,13 @@ class FileTestCase(APITestCase):
         self.assertEqual(file.content.sha1, request.data["sha1"])
 
     def test_update_file(self):
-        file = factories.FileFactory()
+        playbook = factories.PlaybookFactory()
+        file = factories.FileFactory(playbook=playbook)
         old_sha1 = file.content.sha1
         self.assertNotEqual("/path/new_playbook.yml", file.path)
         request = self.client.put(
-            "/api/v1/files/%s" % file.id, {"path": "/path/new_playbook.yml", "content": "# playbook"}
+            "/api/v1/files/%s" % file.id,
+            {"path": "/path/new_playbook.yml", "content": "# playbook", "playbook": playbook.id},
         )
         self.assertEqual(200, request.status_code)
         file_updated = models.File.objects.get(id=file.id)

ara/api/tests/tests_playbook.py

@@ -31,11 +31,7 @@ class PlaybookTestCase(APITestCase):
 
     def test_playbook_serializer(self):
         serializer = serializers.PlaybookSerializer(
-            data={
+            data={"name": "serializer-playbook", "ansible_version": "2.4.0", "path": "/path/playbook.yml"}
-                "name": "serializer-playbook",
-                "ansible_version": "2.4.0",
-                "file": {"path": "/path/playbook.yml", "content": factories.FILE_CONTENTS},
-            }
         )
         serializer.is_valid()
         playbook = serializer.save()
@@ -46,11 +42,7 @@ class PlaybookTestCase(APITestCase):
 
     def test_playbook_serializer_compress_arguments(self):
         serializer = serializers.PlaybookSerializer(
-            data={
+            data={"ansible_version": "2.4.0", "path": "/path/playbook.yml", "arguments": factories.PLAYBOOK_ARGUMENTS}
-                "ansible_version": "2.4.0",
-                "file": {"path": "/path/playbook.yml", "content": factories.FILE_CONTENTS},
-                "arguments": factories.PLAYBOOK_ARGUMENTS,
-            }
         )
         serializer.is_valid()
         playbook = serializer.save()
@@ -82,12 +74,7 @@ class PlaybookTestCase(APITestCase):
     def test_create_playbook(self):
         self.assertEqual(0, models.Playbook.objects.count())
         request = self.client.post(
-            "/api/v1/playbooks",
+            "/api/v1/playbooks", {"ansible_version": "2.4.0", "status": "running", "path": "/path/playbook.yml"}
-            {
-                "ansible_version": "2.4.0",
-                "status": "running",
-                "file": {"path": "/path/playbook.yml", "content": factories.FILE_CONTENTS},
-            },
         )
         self.assertEqual(201, request.status_code)
         self.assertEqual(1, models.Playbook.objects.count())

ara/api/tests/tests_playbook_file.py

@@ -1,61 +0,0 @@
-#  Copyright (c) 2018 Red Hat, Inc.
-#
-#  This file is part of ARA Records Ansible.
-#
-#  ARA is free software: you can redistribute it and/or modify
-#  it under the terms of the GNU General Public License as published by
-#  the Free Software Foundation, either version 3 of the License, or
-#  (at your option) any later version.
-#
-#  ARA is distributed in the hope that it will be useful,
-#  but WITHOUT ANY WARRANTY; without even the implied warranty of
-#  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-#  GNU General Public License for more details.
-#
-#  You should have received a copy of the GNU General Public License
-#  along with ARA.  If not, see <http://www.gnu.org/licenses/>.
-
-from rest_framework.test import APITestCase
-
-from ara.api import models
-from ara.api.tests import factories
-
-
-class PlaybookFileTestCase(APITestCase):
-    def test_create_a_file_and_a_playbook_directly(self):
-        self.assertEqual(0, models.Playbook.objects.all().count())
-        self.assertEqual(0, models.File.objects.all().count())
-        self.client.post(
-            "/api/v1/playbooks",
-            {
-                "ansible_version": "2.4.0",
-                "file": {"path": "/path/playbook.yml", "content": factories.FILE_CONTENTS},
-                "files": [{"path": "/path/host", "content": "Another file"}],
-            },
-        )
-        self.assertEqual(1, models.Playbook.objects.all().count())
-        self.assertEqual(2, models.File.objects.all().count())
-
-    def test_create_file_to_a_playbook(self):
-        playbook = factories.PlaybookFactory()
-        self.assertEqual(1, models.File.objects.all().count())
-        self.client.post(
-            "/api/v1/playbooks/%s/files" % playbook.id,
-            {"path": "/path/playbook.yml", "content": factories.FILE_CONTENTS},
-        )
-        self.assertEqual(2, models.File.objects.all().count())
-        self.assertEqual(1, models.FileContent.objects.all().count())
-
-    def test_create_2_files_with_same_content(self):
-        playbook = factories.PlaybookFactory()
-        number_playbooks = models.File.objects.all().count()
-        number_file_contents = models.FileContent.objects.all().count()
-        content = "# %s" % factories.FILE_CONTENTS
-        self.client.post(
-            "/api/v1/playbooks/%s/files" % playbook.id, {"path": "/path/1/playbook.yml", "content": content}
-        )
-        self.client.post(
-            "/api/v1/playbooks/%s/files" % playbook.id, {"path": "/path/2/playbook.yml", "content": content}
-        )
-        self.assertEqual(number_playbooks + 2, models.File.objects.all().count())
-        self.assertEqual(number_file_contents + 1, models.FileContent.objects.all().count())

ara/api/urls.py

@@ -15,12 +15,13 @@
 #  You should have received a copy of the GNU General Public License
 #  along with ARA.  If not, see <http://www.gnu.org/licenses/>.
 
-from rest_framework_extensions.routers import ExtendedDefaultRouter
+from rest_framework.routers import DefaultRouter
 
 from ara.api import views
 
-router = ExtendedDefaultRouter(trailing_slash=False)
+router = DefaultRouter(trailing_slash=False)
 router.register("labels", views.LabelViewSet, base_name="label")
+router.register("playbooks", views.PlaybookViewSet, base_name="playbook")
 router.register("plays", views.PlayViewSet, base_name="play")
 router.register("tasks", views.TaskViewSet, base_name="task")
 router.register("hosts", views.HostViewSet, base_name="host")
@@ -29,7 +30,4 @@ router.register("files", views.FileViewSet, base_name="file")
 router.register("records", views.RecordViewSet, base_name="record")
 router.register("stats", views.StatsViewSet, base_name="stats")
 
-playbook_routes = router.register("playbooks", views.PlaybookViewSet, base_name="playbook")
-playbook_routes.register("files", views.PlaybookFilesDetail, base_name="file", parents_query_lookups=["playbooks"])
-
 urlpatterns = router.urls

ara/api/views.py

@@ -14,9 +14,8 @@
 #
 #  You should have received a copy of the GNU General Public License
 #  along with ARA.  If not, see <http://www.gnu.org/licenses/>.
-from django.db import transaction
+
 from rest_framework import viewsets
-from rest_framework_extensions.mixins import NestedViewSetMixin
 
 from ara.api import models, serializers
 
@@ -32,17 +31,6 @@ class PlaybookViewSet(viewsets.ModelViewSet):
     filter_fields = ("name", "status")
 
 
-class PlaybookFilesDetail(NestedViewSetMixin, viewsets.ModelViewSet):
-    queryset = models.File.objects.all()
-    serializer_class = serializers.FileSerializer
-
-    def perform_create(self, serializer):
-        playbook = models.Playbook.objects.get(pk=self.get_parents_query_dict()["playbooks"])
-        with transaction.atomic(savepoint=False):
-            instance = serializer.save()
-            playbook.files.add(instance)
-
-
 class PlayViewSet(viewsets.ModelViewSet):
     queryset = models.Play.objects.all()
     serializer_class = serializers.PlaySerializer
@@ -70,6 +58,7 @@ class ResultViewSet(viewsets.ModelViewSet):
 class FileViewSet(viewsets.ModelViewSet):
     queryset = models.File.objects.all()
     serializer_class = serializers.FileSerializer
+    filter_fields = ("playbook", "path")
 
 
 class RecordViewSet(viewsets.ModelViewSet):

requirements.txt

@@ -3,7 +3,6 @@ pbr!=2.1.0,>=2.0.0 # Apache-2.0
 Django>=2
 djangorestframework
 django-cors-headers
-drf-extensions
 django-filter
 django-environ
 dynaconf