-
Notifications
You must be signed in to change notification settings - Fork 481
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
Make PortabilityJob an AutoValue class, including Jackson serialization #150
Conversation
…ented (see PortabilityJobTest)
/** | ||
* Tests serialization and deserialization of a {@link PortabilityJob}. | ||
*/ | ||
public class PortabilityJobTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: the rename of this class is because there used to be an extra "main" in the path (src/test/main/java/...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! So much cleaner than before!
objectMapper.readValue(serializedJob, PortabilityJob.class); | ||
assertThat(deserializedJob).isEqualTo(job); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: new line at eof
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
.put(EXPORT_SERVICE_KEY, exportService()) | ||
.put(IMPORT_SERVICE_KEY, importService()) | ||
.put(AUTHORIZATION_STATE, jobAuthorization().state().toString()) | ||
.put(ENCRYPTED_SESSION_KEY, jobAuthorization().encryptedSessionKey()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does AutoValue check if these are null/empty too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point that I hadn't thought about- AutoValue guarantees the fields are non-null (since no @nullable annotation) but does not check for empty. But, there should be no harm in persisting an empty value in the map.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack that makes sense! Thanks!
@@ -0,0 +1,43 @@ | |||
package org.dataportabilityproject.spi.cloud.types; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
License Header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
This removes boilerplate code and makes the class less error prone in regards to final and null variable state
See PortabilityJobTest for proof of serialization/deserialization
See note in slack channel for AutoValue/Gradle/IntelliJ instructions
Also makes PortabilityJob not inherit from EntityType any more. That inheritance broke json serialization, plus we had discussed getting rid of it anyway as EntityType is empty now.
fixes #4