feat: TransformPageDecorator #1439

sydney-munro · 2022-06-07T19:30:39Z

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> ☕️

If you write sample code, please follow the samples format.

google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java

BenWhitehead · 2022-06-10T17:30:15Z

google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java

+    ifNonNull((String) optionsMap.get(StorageRpc.Option.PAGE_TOKEN), builder::setPageToken);
+    ifNonNull((String) optionsMap.get(StorageRpc.Option.PREFIX), builder::setPrefix);
+    // TODO(sydmunro): StorageRpc.Option.Fields
+    // TODO(sydmunro): User Project


According to the protos, project is required to be able to list buckets.

We'll probably need to do a similar thing here as for listHmacKeys with the fallback to options.

User project is different though because it is used for requester pays to refer to the billing project, correct? the required field that you are referring to is the projectid right?

I added the project I am just making sure this comment was for project and not user project.

+1 to @sydney-munro, you're right, UserProject is meant to assign operation cost to a specified project. It's usually used with RequesterPays but may still be used even if requesterPays is not enabled.

According to

java-storage/proto-google-cloud-storage-v2/src/main/proto/google/storage/v2/storage.proto

Line 336 in 8a7755c

string parent = 1 [

a project must be specified in a list buckets request.

If that is incorrect, we should request that the protos be updated.

yeah, what I am referring to in this comment is that UserProject
https://github.com/googleapis/java-storage/blob/main/google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java#L1015-L1017
is a BucketListOption but not supported by the proto. The project parent field for ListBucketsRequest should be fixed now with L263-L265

Ahh, missed that in the diff-diffs.

google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java

Co-authored-by: BenWhitehead <BenWhitehead@users.noreply.github.com>

frankyn

Overall LGTM, just have one q on property tests

frankyn · 2022-06-15T00:08:10Z

google-cloud-storage/src/test/java/com/google/cloud/storage/GrpcTransformPageDecoratorTest.java

+public class GrpcTransformPageDecoratorTest {
+
+  @Test
+  public void valueTranslationTest() {


Will you be adding a property based test in a follow-up PR?

I can do that, I wanted to make a general test because I felt a property test would kind of just be duplicating the conversion tests, the only difference being wrapping the BlobInfo, BucketInfo etc in a Page but the translation logic is the same. Do you think there would be additional value to doing this?

I was thinking you could use property based testing to generate the list of object names of different sizes (for initial Values, pageOne, pageTwo) then make then the test could do the rest that you're doing in this test. That was my only point in this comment.

This test is specifically for the PageDecorator.

As far as testing the list calls themselves is concerned, I think those can be added in a later PR once we are also testing with the testbench.

Given that pages all have the same "shape" the things we care about testing there are:

Do we construct the request accurately

Do we wire in the correct decoder to the constructed TransformingPageDecorator

The decoder wiring part is pretty much verified for us at compile type via type checking.

The request construction part is probably more of a unit test, given how unfriendly the *Options stuff is.

google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java

BenWhitehead · 2022-06-15T17:30:32Z

google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java

+    ifNonNull((String) optionsMap.get(StorageRpc.Option.PAGE_TOKEN), builder::setPageToken);
+    ifNonNull((String) optionsMap.get(StorageRpc.Option.PREFIX), builder::setPrefix);
+    // TODO(sydmunro): StorageRpc.Option.Fields
+    // TODO(sydmunro): User Project


According to

java-storage/proto-google-cloud-storage-v2/src/main/proto/google/storage/v2/storage.proto

Line 336 in 8a7755c

string parent = 1 [

a project must be specified in a list buckets request.

If that is incorrect, we should request that the protos be updated.

google-cloud-storage/src/test/java/com/google/cloud/storage/GrpcTransformPageDecoratorTest.java

BenWhitehead · 2022-06-15T17:49:38Z

google-cloud-storage/src/test/java/com/google/cloud/storage/GrpcTransformPageDecoratorTest.java

+public class GrpcTransformPageDecoratorTest {
+
+  @Test
+  public void valueTranslationTest() {


This test is specifically for the PageDecorator.

As far as testing the list calls themselves is concerned, I think those can be added in a later PR once we are also testing with the testbench.

Given that pages all have the same "shape" the things we care about testing there are:

Do we construct the request accurately

Do we wire in the correct decoder to the constructed TransformingPageDecorator

The decoder wiring part is pretty much verified for us at compile type via type checking.

The request construction part is probably more of a unit test, given how unfriendly the *Options stuff is.

BenWhitehead · 2022-06-15T20:13:31Z

google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java

+    ifNonNull((String) optionsMap.get(StorageRpc.Option.PAGE_TOKEN), builder::setPageToken);
+    ifNonNull((String) optionsMap.get(StorageRpc.Option.PREFIX), builder::setPrefix);
+    // TODO(sydmunro): StorageRpc.Option.Fields
+    // TODO(sydmunro): User Project


Ahh, missed that in the diff-diffs.

sydney-munro added 2 commits June 7, 2022 12:29

feat: Grpc Page implementation and skeleton definitions

7625021

Minor naming tweak

b0fa574

sydney-munro requested a review from a team as a code owner June 7, 2022 19:30

sydney-munro marked this pull request as draft June 7, 2022 19:30

product-auto-label bot added size: m Pull request size is medium. api: storage Issues related to the googleapis/java-storage API. labels Jun 7, 2022

sydney-munro changed the title ~~Page impl~~ feat: TransformPageDecorator Jun 7, 2022

sydney-munro added 2 commits June 9, 2022 12:54

Build up requests

829d0d2

attempting to fix build

89e3b5a

BenWhitehead requested changes Jun 10, 2022

View reviewed changes

sydney-munro and others added 2 commits June 10, 2022 14:16

Apply nit suggestions from code review

8fc2899

Co-authored-by: BenWhitehead <BenWhitehead@users.noreply.github.com>

Address some PR comments

39c6991

BenWhitehead added this to the Storage gRPC implementation milestone Jun 10, 2022

sydney-munro added 4 commits June 14, 2022 09:48

First pass testing value translation

19ec0dd

linter

5e8e822

Second pass test with pseudo pagination

32feeab

Copywrite header

0d8c3f3

product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Jun 14, 2022

Addressing PR comments

21d1e5f

sydney-munro marked this pull request as ready for review June 14, 2022 23:31

frankyn reviewed Jun 15, 2022

View reviewed changes

BenWhitehead requested changes Jun 15, 2022

View reviewed changes

Addressing PR comments

aa5c947

BenWhitehead approved these changes Jun 15, 2022

View reviewed changes

BenWhitehead merged commit 0d564fc into feat/grpc-storage Jun 15, 2022

BenWhitehead deleted the page-impl branch June 15, 2022 20:17

feat: TransformPageDecorator #1439

feat: TransformPageDecorator #1439

Uh oh!

Conversation

sydney-munro commented Jun 7, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sydney-munro Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

sydney-munro Jun 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

frankyn left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sydney-munro Jun 14, 2022 •

edited

Loading

sydney-munro Jun 15, 2022 •

edited

Loading