Add more fields and validation to twine upload endpoint #995
Conversation
gerrod3
force-pushed
the
more-twine-upload-fields
branch
3 times, most recently
from
88c8488 to
1a0c240
Compare
pulp_python/app/pypi/serializers.py
Outdated
| min_length=64, | ||
| max_length=64, | ||
| ) | ||
| protocol_version = serializers.IntegerField( |
There was a problem hiding this comment.
Would not it be better to use ChoiceField instead of IntegerField with min and max values?
| if filetype := data.get("filetype"): | ||
| if filetype != packagetype: |
There was a problem hiding this comment.
Maybe if (filetype := data.get("filetype")) and filetype != packagetype: to save one nesting?
|
I think this is worthy of the changelog - minimally because of the validations. |
| version = serializers.CharField( | ||
| help_text=_("Version of the package."), | ||
| required=False, | ||
| ) |
There was a problem hiding this comment.
edit: maybe I'm misunderstanding, I suppose you mention that the values do come from the uploaded artifact, but the RPM plugin also does so and doesn't add any of the metadata to the upload serializer, so this is different than what I'm familiar with.
I'm confused about the fact that all of it seems to be optional. If it's all optional than why allow the user to specify in the first place (maybe these are all just gripes with the API they've chosen and not with the fact that Pulp needs to implement it)
Is it just the case that twine provides all this information on uploads and the API (including e.g. warehouse) is meant to just trust it unconditionally, if provided? Or does it get verified? And if it gets verified then against what, and why bother with accepting it if there's a canonical source?
Seems like it would be ideal for most of these details to be parsed from package metadata instead of provided alongside the upload? (though IIRC that was briefly discussed earlier, I guess my point is that if we add it now would it not become useless should metadata later be parsed directly from the package). But again maybe this is just complaining about the API
There was a problem hiding this comment.
Yeah, maybe I should remove them. Reading the PyPI code (https://github.com/pypi/warehouse/blob/117eafc3739f738a2c29fbadbb858bf502ba1ff0/warehouse/forklift/legacy.py#L618-L623) it seems they trust all the metadata that is posted in the request, but we have always been extracting the metadata from the file so these fields are kind of useless.
There was a problem hiding this comment.
So, what is the resolution, stay or remove?
There was a problem hiding this comment.
They're gone, look at the latest push.
gerrod3
force-pushed
the
more-twine-upload-fields
branch
2 times, most recently
from
8f4f288 to
077993f
Compare
gerrod3
force-pushed
the
more-twine-upload-fields
branch
from
077993f to
ea1dc16
Compare
3 participants
Prep work for PEP 740. I noticed there were new fields to implement: https://docs.pypi.org/api/upload/. The serializer ignores fields we don't list and technically we don't need these fields to perform the upload, they are just a nice sanity check. Currently of the required fields name, version and pyversion I am not doing any extra validation. We get the values by extracting the metadata from the artifact in the upload task so hard to validate in the request serializer.