✨ Fix deserialization issues by keithrfung · Pull Request #139 · Election-Tech-Initiative/electionguard-python

keithrfung · 2020-08-05T19:47:14Z

Issue

Fixes #132
Fixes #120
Fixes #122 while obsoleting PR #123

Description

For many of the core ElectionGuard classes, deserialization of a serialized class does not yield the original or has issues.

Replace post_init with factory methods
Deserialization has issues with the post_init methods that we'd been using, similar to InitVar. Factory make_ methods were created for constructing the classes.

Resolve equality methods for testing
To make sure equality testing worked among the core ElectionGuard classes, there were some additional changes. Some of them use @DataClass(eq=True...) to auto-generate the methods. This doesn't work for lists which might come back in a different order than they were written out. For those cases, existing _list_eq() method is used in custom eq methods.

Remove frozen tag
A frozen=True dataclass with a non-frozen superclass causes an inheritance error (ignored by Python, flagged by PyCharm) to have . Since our Serializable isn't frozen, this is a problem. The frozen attribute was removed from the dataclasses only relevant to this PR.

Change crypto_hash to class method from property

Add serialization methods for datetime

Allow strings to be passed into mpz for conversion

Resolve issues with Proof deserialization
Proof serialized correctly but had issues deserializing with the conversions

Add "None" deserialization
"None" was not deserializing properly and caused issues with certain Optionals

Testing

test_serializable
test_end_to_end_election

Checklist

🚨Please review the guidelines for contributing to this repository.

🤔 CONSIDER adding a unit test if your PR resolves an issue.
✅ DO check open PR's to avoid duplicates.
✅ DO keep pull requests small so they can be easily reviewed.
✅ DO build locally before pushing.
✅ DO make sure tests pass.
✅ DO make sure any new changes are documented.
✅ DO make sure not to introduce any compiler warnings.
❌AVOID breaking the continuous integration build.
❌AVOID making significant changes to the overall architecture.

💚Thank you!

keithrfung · 2020-08-10T20:18:31Z



+@dataclass
+class PublishedPlaintextTally(ElectionObjectBase):


These classes were made due to the unique serialization to just remove the other elements of the Tally. This simplifies it to just the items to be published. Drastically simplifying the deserialization as well.

keithrfung · 2020-08-10T20:31:14Z


-        if self.REMOVE_OUTPUT:
-            rmtree(RESULTS_DIR)
+    def verify_results(self) -> None:


This test deserializes all the outputs just made into the file directory.

keithrfung · 2020-08-10T20:32:25Z

+READ: str = "r"
 JSON_PARSE_ERROR = '{"error": "Object could not be parsed due to json issue"}'
+# TODO Issue #??: Jsons library incorrectly dumps class method
+FROM_JSON_FILE = '"from_json_file": {}, '


This is handling a bug with jsons that an issue still needs to be made for. I'll update PR accordingly.

Essentially every output file has this classmethod appended.

keithrfung · 2020-08-10T20:32:59Z

        :param strip_privates: strip private variables
        :return: the json representation of this object
        """
+        set_serializers()


These were moved into here to reduce turning these on every time they needed to be used.

keithrfung · 2020-08-11T13:31:24Z

+    :param description_hash: the hash of the election metadata
+    """
+
+    # What's a crypto_base_hash?


This needs to get fixed.

keithrfung · 2020-08-11T13:31:56Z

    you're absolutely, positively, certain the input is in-bounds.
    """
-    return ElementModP(mpz(i))
+    m = mpz(int(i))


Want to create some consistency with these so that all the methods look the same.

keithrfung · 2020-08-11T13:33:30Z

        write_json_file(self.to_json(strip_privates), file_name, file_path)

+    @classmethod
+    def from_json_file(cls, file_name: str, file_path: str = "") -> T:


This could use just the file_path to reduce the length of this method. This concatenation could be moved out. I only kept it because the write had that but it could be said that neither need it.

keithrfung · 2020-08-11T13:34:56Z


    set_deserializer(
-        lambda usage_string, cls, **_: ProofUsage[usage_string], ProofUsage
+        lambda none, cls, **_: None


This deserialization from "None" is critical. Perhaps we could change it to serialize to null as well and have a deserialization from null to None but regardless this needs to exist for Ballots to be deserialized.

keithrfung · 2020-08-11T13:35:17Z


    return ElectionDescription(
-        election_scope_id=draw(languages()),
+        election_scope_id=draw(emails()),


Verify this.

- Create functional style make method for ElectionContext - Integrate it into Tests

- Create make method for CiphertextAcceptedBallot and CiphertextBallot - Update use cases - Remove todo

- Cast str to int ahead of cast

- Add equality statements - Remove frozen - Use unsafe_hash to remove unnecessary hash function

- Ballot store will now always return Ballots for all() method. This is the main use case and reduces the need to check for null around the store. Fixed attached integration test.

- ✨Remove InitVar for Plaintext Tally - Remove InitVar for Selection - Remove PostVar - Plaintext Tally Changes - Sample Generator Fixes

"None" created issues due Optional that also have custom deserialization. The solution for this is ensuring all NoneTypes are checked against a string before running default deserialization. - Suppress Warning in serializable

keithrfung requested a review from AddressXception August 5, 2020 19:47

keithrfung marked this pull request as draft August 5, 2020 19:47

This was referenced Aug 5, 2020

Fix/serialization and equality #135

Closed

deserialization issue with hashed ballot nonce property #133

Closed

rc-ms marked this pull request as ready for review August 6, 2020 18:39

keithrfung force-pushed the fix/deserialization-issues branch from 4ff5bef to d3df1db Compare August 10, 2020 14:14

keithrfung commented Aug 10, 2020

View reviewed changes

keithrfung force-pushed the fix/deserialization-issues branch from e71551c to 798df06 Compare August 10, 2020 20:27

keithrfung commented Aug 10, 2020

View reviewed changes

keithrfung changed the title ~~🚧 Fix/deserialization issues~~ ✨ Fix deserialization issues Aug 11, 2020

keithrfung commented Aug 11, 2020

View reviewed changes

AddressXception suggested changes Aug 11, 2020

View reviewed changes

Comment thread src/electionguard/ballot.py Outdated

Comment thread src/electionguard/publish.py

keithrfung force-pushed the fix/deserialization-issues branch from 798df06 to 7f9b27b Compare August 11, 2020 17:15

keithrfung added 11 commits August 11, 2020 15:58

✨ Create make method for ElectionContext

2d13f0f

- Create functional style make method for ElectionContext - Integrate it into Tests

✨ Create make method for CiphertextBallots

c0965e4

- Create make method for CiphertextAcceptedBallot and CiphertextBallot - Update use cases - Remove todo

✨ Add datetime to serialize / deserialize

7c0ca25

🐛 Remove Proof serialization

951097b

✨ Allow conversion from str for mpz

4f673b9

- Cast str to int ahead of cast

✨ Alteration to dataclass usage

0902b49

- Add equality statements - Remove frozen - Use unsafe_hash to remove unnecessary hash function

♻️ Refactor all() for BallotStore

e97a346

- Ballot store will now always return Ballots for all() method. This is the main use case and reduces the need to check for null around the store. Fixed attached integration test.

✅Deserialization Tests and Fixes

8637125

- ✨Remove InitVar for Plaintext Tally - Remove InitVar for Selection - Remove PostVar - Plaintext Tally Changes - Sample Generator Fixes

🐛 Handle incorrect from_json_file

7d18380

🐛 Handle None deserialization

8ab1462

"None" created issues due Optional that also have custom deserialization. The solution for this is ensuring all NoneTypes are checked against a string before running default deserialization. - Suppress Warning in serializable

📝Publish and Verify documentation

539338e

keithrfung force-pushed the fix/deserialization-issues branch from 7f9b27b to 539338e Compare August 11, 2020 19:58

AddressXception approved these changes Aug 11, 2020

View reviewed changes

keithrfung merged commit 4ce8592 into main Aug 12, 2020

keithrfung deleted the fix/deserialization-issues branch August 12, 2020 13:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Fix deserialization issues#139

✨ Fix deserialization issues#139
keithrfung merged 11 commits intomainfrom
fix/deserialization-issues

keithrfung commented Aug 5, 2020 •

edited

Loading

Uh oh!

keithrfung Aug 10, 2020

Uh oh!

keithrfung Aug 10, 2020

Uh oh!

keithrfung Aug 10, 2020

Uh oh!

keithrfung Aug 10, 2020

Uh oh!

keithrfung Aug 11, 2020

Uh oh!

keithrfung Aug 11, 2020

Uh oh!

keithrfung Aug 11, 2020

Uh oh!

keithrfung Aug 11, 2020

Uh oh!

keithrfung Aug 11, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants



		@dataclass
		class PublishedPlaintextTally(ElectionObjectBase):

Conversation

keithrfung commented Aug 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Description

Testing

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

keithrfung commented Aug 5, 2020 •

edited

Loading