Define schema for detail routes. #60

dparalen · 2018-03-22T13:58:25Z

closes #3351

werwty · 2018-03-23T04:21:44Z

pulp_file/app/viewsets.py

-            raise serializers.ValidationError(detail=_('Repository URI must be specified.'))
-        repository = self.get_resource(repository_uri, Repository)
+        serializer = _RepositorySyncURLSerializer(data=request.data)
+        if not serializer.is_valid():


Probably just call serializer.is_valid(raise_exception=True) here

werwty · 2018-03-23T04:28:26Z

pulp_file/app/viewsets.py

@@ -28,6 +28,37 @@ class Meta:
        ]


+class _URLField(serializers.URLField):


Why subclass URLField instead of using default URLField?

werwty · 2018-03-23T04:47:44Z

pulp_file/app/viewsets.py

    def publish(self, request, pk):
+        serializer = _RepositoryPublishURLSerializer(data=request.data)
+        if not serializer.is_valid():


Probably just call serializer.is_valid(raise_exception=True) here

werwty · 2018-03-23T05:00:32Z

pulp_file/app/viewsets.py

        publisher = self.get_object()
-        repository = None
-        repository_version = None
-        if 'repository' not in request.data and 'repository_version' not in request.data:


I would recommend cleaning up this logic a bit:

--- a/pulp_file/app/viewsets.py +++ b/pulp_file/app/viewsets.py @@ -121,23 +121,20 @@ class FilePublisherViewSet(PublisherViewSet): repository_uri = serializer.data.get('repository') repository_version_uri = serializer.data.get('repository_version') publisher = self.get_object() - repository, repository_version = None, None + if not repository_uri and not repository_version_uri: raise serializers.ValidationError(_("Either the 'repository' or 'repository_version' " "need to be specified.")) - if repository_uri: - repository = self.get_resource(repository_uri, Repository) - - if repository_version_uri: - repository_version = self.get_resource(repository_version_uri, RepositoryVersion) - - if repository and repository_version: + elif repository_uri and repository_version_uri: raise serializers.ValidationError(_("Either the 'repository' or 'repository_version' " "can be specified - not both.")) - - if not repository_version: + elif repository_uri: + repository = self.get_resource(repository_uri, Repository) repository_version = RepositoryVersion.latest(repository) + else: + repository_version = self.get_resource(repository_version_uri, RepositoryVersion) +

werwty

Nicely done @dparalen! I thought for sure we needed to make upstream changes to drf_openapi to get this to work but I'm so glad you found a suitable solution 🥇 🥂

I'm running into some issues publishing a repository_version http POST $PUBLISHER_HREF'publish/' repository_version=http://pulp3.dev:8000/api/v3/repositories/30ed44e8-f0c3-434c-8bea-d4f646d90f61/versions/1/

Is this the right POST request?

  File "/home/vagrant/devel/pulp_file/pulp_file/app/viewsets.py", line 136, in publish
    repository_version = self.get_resource(repository_version_uri, RepositoryVersion)
  File "/home/vagrant/devel/pulp/pulpcore/pulpcore/app/viewsets/base.py", line 64, in get_resource
    pk = match.kwargs['pk']

pep8speaks · 2018-03-23T13:16:25Z

Hello @dparalen! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on March 23, 2018 at 21:02 Hours UTC

dparalen · 2018-03-23T13:34:20Z

@werwty thanks for the review and the kind words! :D
w/r the logic refactoring, I moved part of it to a custom validator which, according to the docs should be the preferred way to do it; it gives me this nicely structured and (hopefully) self-explanatory no-no when I try to cheat:

(pulp) [vagrant@pulp3 pulpcore]$ phttp  :8000/api/v3/publishers/file/540f8af9-150a-4b1c-921a-da465248dc25/publish/ foo=bar
HTTP/1.0 400 Bad Request
Allow: POST, OPTIONS
Content-Length: 107
Content-Type: application/json
Date: Fri, 23 Mar 2018 12:25:56 GMT
Server: WSGIServer/0.2 CPython/3.6.4
Vary: Accept, Cookie
X-Frame-Options: SAMEORIGIN

{
    "non_field_errors": [
        "Either the 'repository' or 'repository_version' need to be specified but not both."
    ]
}

Unfortunately, I wasn't able to reproduce your error but maybe I'm running out-of-date core...
Please, let me know wdyt.

Cheers,
milan

werwty · 2018-03-23T14:03:57Z

pulp_file/app/viewsets.py

+        required=False
+    )
+
+    def validate(self, data):


werwty · 2018-03-23T17:39:26Z

pulp_file/app/viewsets.py

@@ -82,27 +114,18 @@ class FilePublisherViewSet(PublisherViewSet):
    queryset = FilePublisher.objects.all()
    serializer_class = FilePublisherSerializer

-    @detail_route(methods=('post',))
+    @detail_route(methods=('post',), serializer_class=_RepositoryPublishURLSerializer)
    def publish(self, request, pk):
        publisher = self.get_object()


I would recommend adding a docstring here. This would show up in the auto docs like so:

werwty · 2018-03-23T18:07:45Z

@dparalen I'm still running into an error when publishing repository_versions (but not repository). My pulp repo is up to date, and pulp_file is checked out from this PR. Can you check to see if you can reproduce it?

werwty

I see that I'm running into this issue: https://pulp.plan.io/issues/3466

Since that's the case, I think this PR is ready for merge, I just left an optional suggestion.
Thanks @dparalen

dparalen · 2018-03-23T20:49:04Z

@werwty I just wanted to let you know that indeed that's the issue you're running into and I've been running my core with the fix applied all the time 🙀
...updating

closes #3351

dparalen · 2018-03-26T13:51:04Z

@werwty in case my update is OK, please merge this change as I don't have the commit bit ;)

Thanks,
milan

werwty · 2018-03-26T14:42:44Z

@dparalen ha! I also don't have the commit bit. @daviddavis Can you take care of this?

daviddavis · 2018-03-26T14:47:34Z

👍

dparalen · 2018-03-26T18:04:25Z

lol, thanks guys! 😘

werwty reviewed Mar 23, 2018

View reviewed changes

werwty suggested changes Mar 23, 2018

View reviewed changes

dparalen force-pushed the jortel/issue-3351 branch from efdd634 to bbe3296 Compare March 23, 2018 13:16

dparalen force-pushed the jortel/issue-3351 branch from bbe3296 to cf8baa2 Compare March 23, 2018 13:17

werwty reviewed Mar 23, 2018

View reviewed changes

pulp_file/app/viewsets.py

required=False

)

def validate(self, data):

Copy link

Contributor

werwty Mar 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

werwty reviewed Mar 23, 2018

View reviewed changes

werwty approved these changes Mar 23, 2018

View reviewed changes

Define schema for detail routes.

b33c6d2

closes #3351

dparalen force-pushed the jortel/issue-3351 branch from cf8baa2 to b33c6d2 Compare March 23, 2018 21:02

daviddavis merged commit 413563f into pulp:master Mar 26, 2018

Define schema for detail routes. #60

Define schema for detail routes. #60

dparalen commented Mar 22, 2018

werwty Mar 23, 2018

werwty Mar 23, 2018 •

edited

werwty Mar 23, 2018

werwty Mar 23, 2018

werwty left a comment •

edited

pep8speaks commented Mar 23, 2018 •

edited

dparalen commented Mar 23, 2018

werwty Mar 23, 2018

werwty Mar 23, 2018

werwty commented Mar 23, 2018

werwty left a comment

dparalen commented Mar 23, 2018

dparalen commented Mar 26, 2018

werwty commented Mar 26, 2018

daviddavis commented Mar 26, 2018

dparalen commented Mar 26, 2018

		@@ -28,6 +28,37 @@ class Meta:
		]


		class _URLField(serializers.URLField):

Define schema for detail routes. #60

Define schema for detail routes. #60

Conversation

dparalen commented Mar 22, 2018

werwty Mar 23, 2018

Choose a reason for hiding this comment

werwty Mar 23, 2018 • edited

Choose a reason for hiding this comment

werwty Mar 23, 2018

Choose a reason for hiding this comment

werwty Mar 23, 2018

Choose a reason for hiding this comment

werwty left a comment • edited

Choose a reason for hiding this comment

pep8speaks commented Mar 23, 2018 • edited

Comment last updated on March 23, 2018 at 21:02 Hours UTC

dparalen commented Mar 23, 2018

werwty Mar 23, 2018

Choose a reason for hiding this comment

werwty Mar 23, 2018

Choose a reason for hiding this comment

werwty commented Mar 23, 2018

werwty left a comment

Choose a reason for hiding this comment

dparalen commented Mar 23, 2018

dparalen commented Mar 26, 2018

werwty commented Mar 26, 2018

daviddavis commented Mar 26, 2018

dparalen commented Mar 26, 2018

werwty Mar 23, 2018 •

edited

werwty left a comment •

edited

pep8speaks commented Mar 23, 2018 •

edited