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

Splitting seralizers into read and write ones #707

Merged
merged 1 commit into from May 26, 2020

Conversation

fao89
Copy link
Member

@fao89 fao89 commented May 19, 2020

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

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

@pulpbot
Copy link
Member

pulpbot commented May 19, 2020

Warning: Issue #6346 is not at NEW/ASSIGNED/POST.

if not field.read_only: # Removing read_only fields
write_fields[field_name] = field

if len(read_fields) == len(write_fields):
Copy link
Member

Choose a reason for hiding this comment

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

I think that comparison is too risky. Can you compare the keys() for equality?

Comment on lines 508 to 511
contain_write_fields = len(serializer.fields) > len(read_fields)
if self.method.lower() == "get" or not contain_write_fields:
serializer.fields = read_fields
return serializer
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
contain_write_fields = len(serializer.fields) > len(read_fields)
if self.method.lower() == "get" or not contain_write_fields:
serializer.fields = read_fields
return serializer
contain_write_only_fields = len(serializer.fields) > len(read_fields)
if self.method.lower() == "get" or not contain_write_only_fields:
serializer.fields = read_fields
return serializer

Also: are serializer.fields and read_fields not exactly the same in this case?

serializer.fields = read_fields
return serializer

return self._convert_serializer(serializer, WriteOnly)
Copy link
Member

Choose a reason for hiding this comment

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

I belive we agreed to perform this transformation on the read-serializer and not the write-serializer.

Copy link
Member

Choose a reason for hiding this comment

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

That's right ... we want the write serializer to keep the original name and the read serializer to change to OriginalNameRead.

The motivation for this outlined here: https://www.redhat.com/archives/pulp-dev/2020-April/msg00101.html

@fao89 fao89 force-pushed the write_serializer branch 5 times, most recently from e28ff1d to ee54b5d Compare May 21, 2020 17:29
@@ -33,7 +33,6 @@ class SingleArtifactContentSerializer(BaseContentSerializer):
relative_path = serializers.CharField(
help_text=_("Path where the artifact is located relative to distributions base_path"),
validators=[fields.relative_path_validator],
write_only=True,
Copy link
Member

Choose a reason for hiding this comment

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

That use of write_only should be preserved. As the database for content does not have that field.

Copy link
Member Author

@fao89 fao89 May 25, 2020

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

That's interesting, because in those cases, the write_onlyness should be deactivated dynamically in the __init__ of the serializer. Are you assigning the model attribute on the ReadOnly serializer you are creating on the fly?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll put a rpdb to see why it is ignoring the __init__

Copy link
Member Author

Choose a reason for hiding this comment

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

with ReadOnly mixin I was getting the fields from the original serializer class, so the __init__ was not triggered. So instead of creating a new serializer class from the mixin, I'm doing it by modifying the serializer object.

Copy link
Member

@mdellweg mdellweg left a comment

Choose a reason for hiding this comment

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

I left a question, how to maybe simplify the pr. It should not be blocking this PR.

Thanks for adressing all my concerns.


class ReadOnlySerializer(original_serializer.__class__):
class Meta(serializer_meta):
ref_name = read_ref_name
Copy link
Member

Choose a reason for hiding this comment

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

So this new class is there to change the ref_name. Anyway we can get away without crating that transient class by just setting that ref_name on the instance original_serializer (like we do with the fields two lines down)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried, but it changes the original serialize, and I ended up with SerializerNameReadReadRead

Copy link
Member

Choose a reason for hiding this comment

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

Then just ignore this one.

@fao89 fao89 requested a review from dkliban May 26, 2020 21:12
@dkliban
Copy link
Member

dkliban commented May 26, 2020

I tested this by creating a PythonPythonPackageContent. Worked great!

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