chore(grpc): refactor model to/from Pb methods to be abstracted by Codec #1344
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
frankyn
reviewed
Apr 6, 2022
| Conversions.apiary().blobInfo().encode(this), | ||
| Conversions.apiary() | ||
| .blobInfo() | ||
| .encode(((BlobInfo) obj))); // TODO: remove this excessive allocation |
Contributor
There was a problem hiding this comment.
When you say remove excessive allocation, are you saying the Blob should retain a reference to it's encoding so you can don't have to do .encode(this)?
Collaborator
Author
There was a problem hiding this comment.
No, I think we should implement equals & hash code in terms of the properties of the class and not in terms of it's converted result.
| @Test | ||
| public void testCreateBucket() { | ||
| doReturn(BUCKET_INFO1.toPb()) | ||
| doReturn(Conversions.apiary().bucketInfo().encode(BUCKET_INFO1)) |
Contributor
There was a problem hiding this comment.
Do we want to complete moving to mockito from easymock in main branch?
Collaborator
Author
There was a problem hiding this comment.
Eventually yeah, though I'm not sure it's an immediate need compared to features for gRPC.
3729d4f to
eca9821
Compare
* Introduce new internal API Codec<From, To> which provides a symmetric pair of conversion functions between two types * Add ApiaryConversions.java to contain all the new Codecs for each of the existing apiary toPb/fromPb methods * Update StorageImpl and associated classes to use new codecs rather than old model methods * Add clirr rule to allow removing accidental public HmacKeyMetadata#toPb * Add BucketInfo#asBucket to convert from a BucketInfo to the syntax class Bucket with a specified instance of Storage * Add BlobInfo#asBlob to convert from a BlobInfo to the syntax class Blob with a specified instance of Storage * Add Utils#ifNonNull to help tersify a lot of the conversion code in ApiaryConversions
eca9821 to
0701d61
Compare
frankyn
suggested changes
May 2, 2022
Contributor
frankyn
left a comment
There was a problem hiding this comment.
LGTM, missing codec for Notification:
google-cloud-storage/src/main/java/com/google/cloud/storage/Notification.java: static Notification fromPb(
google-cloud-storage/src/main/java/com/google/cloud/storage/Notification.java: storage, new NotificationInfo.BuilderImpl(NotificationInfo.fromPb(notificationPb)));
google-cloud-storage/src/main/java/com/google/cloud/storage/NotificationInfo.java: return NotificationInfo.fromPb(pb);
google-cloud-storage/src/main/java/com/google/cloud/storage/NotificationInfo.java: static NotificationInfo fromPb(
google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java: return Notification.fromPb(
google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java: return answer == null ? null : Notification.fromPb(this, answer);
google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java: return Notification.fromPb(getOptions().getService(), notificationPb);
google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java: IamConfiguration fromPb = Conversions.apiary().iamConfiguration().decode(iamConfiguration);
google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java: assertEquals(PublicAccessPrevention.UNKNOWN, fromPb.getPublicAccessPrevention());
google-cloud-storage/src/test/java/com/google/cloud/storage/NotificationInfoTest.java: NOTIFICATION_INFO, NotificationInfo.fromPb(NOTIFICATION_INFO.toPb()));
google-cloud-storage/src/test/java/com/google/cloud/storage/NotificationInfoTest.java: compareBucketsNotification(notificationInfo, Notification.fromPb(notificationInfo.toPb()));
google-cloud-storage/src/test/java/com/google/cloud/storage/NotificationTest.java: NOTIFICATION_INFO, Notification.fromPb(storage, NOTIFICATION_INFO.toPb()));
google-cloud-storage/src/test/java/com/google/cloud/storage/AclTest.java
Outdated
Show resolved
Hide resolved
Collaborator
Author
|
Refactored the notification conversions as well, thanks for the reminder. Ready for re-review. |
frankyn
approved these changes
May 4, 2022
| } | ||
|
|
||
| private static void checkTopicFormat(String topic) { | ||
| private static void checkTopicFormat(String topic) { // todo: why does this exist? |
Contributor
There was a problem hiding this comment.
Accidentally added I'm assuming, but we can remove it.
sydney-munro
pushed a commit
that referenced
this pull request
Jun 7, 2022
…dec (#1344) * Introduce new internal API Codec<From, To> which provides a symmetric pair of conversion functions between two types * Add ApiaryConversions.java to contain all the new Codecs for each of the existing apiary toPb/fromPb methods * Update StorageImpl and associated classes to use new codecs rather than old model methods * Add clirr rule to allow removing accidental public HmacKeyMetadata#toPb * Add BucketInfo#asBucket to convert from a BucketInfo to the syntax class Bucket with a specified instance of Storage * Add BlobInfo#asBlob to convert from a BlobInfo to the syntax class Blob with a specified instance of Storage * Add Utils#ifNonNull to help tersify a lot of the conversion code in ApiaryConversions
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.