Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate Content data in the Stages API #532

Closed
wants to merge 1 commit into from
Closed

Conversation

fao89
Copy link
Member

@fao89 fao89 commented Feb 11, 2020

https://pulp.plan.io/issues/5927
closes #5927

Please be sure you have read our documentation on creating PRs:
https://docs.pulpproject.org/contributing/pull-request-walkthrough.html

@@ -109,17 +111,19 @@ class DeclarativeContent:

__slots__ = (
'content',
'serializer',
Copy link
Member

Choose a reason for hiding this comment

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

Conceptually, each content type pairs with one serializer, so from an object oriented perspective what about defining this relationship on the model? I need to look into the loader code to see what is there already also.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking we could have situations where we have multiple serializers for a Content model. To give an example, for rpm packages that are uploaded we don't validate name on upload (it's fetched from the artifact) but on sync, we might want to validate it exists. That's what led me to believe we should tell the stages api which serializer to use.

Copy link
Member

Choose a reason for hiding this comment

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

I can see the thinking. I imagined the uploading of an artifact that isn't actually a content unit yet. For example in pulp_ansible the tarball is just a tarball until it's imported so the Collection model has one way to represent an already saved one. Uploads wouldn't use that one.

Part of my motivation to change this is that using this code means setting it item-by-item which when dealing with mixed types in sync code is more complicated. Having it declared on the object is easier which I think is helpful to the integrator.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can see the thinking. I imagined the uploading of an artifact that isn't actually a content unit yet. For example in pulp_ansible the tarball is just a tarball until it's imported so the Collection model has one way to represent an already saved one. Uploads wouldn't use that one.

This kind of makes sense to me in the one-shot upload case. However, you can also directly create Content in the API. This seems like it should use a serializer tied to the Content model. As a plugin writer, I think I'd be confused about which serializer I'm supposed to set on the model if I need to have different ones for uploading/directly creating content in the API vs sync.

Part of my motivation to change this is that using this code means setting it item-by-item which when dealing with mixed types in sync code is more complicated. Having it declared on the object is easier which I think is helpful to the integrator.

I can see this motivation and agree we should try to avoid having to set serializers on an item by item basis. Perhaps for now we can go with defining serializer on models but in the future allow plugin writers to optionally set a mapping with contexts:

class FileContent(Content):
    serializer = FileContentSerializer

class TestContent(Content):
    serializer = {'upload': TestContentUploadSerializer, 'sync': 'TestContentSerializer'}

Copy link
Member

Choose a reason for hiding this comment

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

Defining the serializer on the model for now makes sense and adding more later I think does too. Especially since we are favoring DRF validation with serializers over Django's own model validation, I think we need plugin writers to express how something should validate the model.

@pulpbot
Copy link
Member

pulpbot commented Apr 3, 2020

Attached issue: https://pulp.plan.io/issues/5927

@fao89
Copy link
Member Author

fao89 commented Apr 24, 2020

Closing as the story will be rewritten: https://www.redhat.com/archives/pulp-dev/2020-April/msg00075.html

@fao89 fao89 closed this Apr 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants