feat: BucketInfo conversion for gRPC #1431

frankyn · 2022-06-02T20:55:51Z

Introducing more support for BucketInfo conversions

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

BenWhitehead · 2022-06-03T17:25:29Z

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

+      }
+
+      /**
+       * Returns the date in RFC 3339 format with only the date part (for instance, "2013-01-15").


OffesetDateTime isn't exactly an RFC 3339 date.

I think we'll want to change the comment here to be a bit more explicit. Something like:

Returns the date and offset from UTC for this condition.
If a time other than 00:00:00.000 is present in the value, GCS will truncate to 00:00:00.000.

BenWhitehead · 2022-06-03T17:27:20Z

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

-import net.jqwik.api.Property;

-final class BucketPropertyTest {
+final class BucketInfoPropertyTest extends BaseConvertablePropertyTest<BucketInfo, Bucket> {


BenWhitehead · 2022-06-03T17:29:29Z

google-cloud-storage/src/test/java/com/google/cloud/storage/jqwik/StorageArbitraries.java

+        .offsetBetween(ZoneOffset.UTC, ZoneOffset.UTC)
+        .map(
+            odt -> {
+              OffsetDateTime utc = odt.withOffsetSameInstant(ZoneOffset.UTC);


I don't think you need this transform here, the input has previously been constrained to UTC.

BenWhitehead · 2022-06-03T17:32:45Z

google-cloud-storage/src/test/java/com/google/cloud/storage/jqwik/StorageArbitraries.java

+      return rule()
+          .list()
+          .ofMinSize(0)
+          .ofMaxSize(100)


Is there an actual limit on the gcs side for how many rules can be present? If so it might make sense to use a similar value here.

I thought it was 100 but i can't seem to find a limit. I can remove the upperbound but 100 feels okay to me. What do you think?

100's probably fine in that case. We probably won't want to somehow end up slowing down tests because the generator needs to construct like 700k of these things.

BenWhitehead · 2022-06-03T17:40:32Z

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

+          if (createdBefore == null) return this;
+          return setCreateBeforeOffsetDateTime(
+              millisOffsetDateTimeCodec.encode(createdBefore.getValue()));


Similar to the getter, I think this can be simplified to

Suggested change

if (createdBefore == null) return this;

return setCreateBeforeOffsetDateTime(

millisOffsetDateTimeCodec.encode(createdBefore.getValue()));

return setCustomTimeBeforeOffsetDateTime(

Utils.dateTimeCodec.nullable().decode(customTimeBefore));

BenWhitehead · 2022-06-03T17:40:52Z

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

+          if (noncurrentTimeBefore == null) return this;
+          return setNoncurrentTimeBeforeOffsetDateTime(
+              millisOffsetDateTimeCodec.encode(noncurrentTimeBefore.getValue()));


Same as #setCustomDateTime

BenWhitehead · 2022-06-03T17:41:08Z

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

+          if (customTimeBefore == null) return this;
+          return setCustomTimeBeforeOffsetDateTime(
+              millisOffsetDateTimeCodec.encode(customTimeBefore.getValue()));


Same as #setCustomDateTime

BenWhitehead · 2022-06-03T17:42:08Z

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

+          if (createdBefore == null) return this;
+          return setCreateBeforeOffsetDateTime(
+              millisOffsetDateTimeCodec.encode(createdBefore.getValue()));
+        }


Additionally, when using the Utils.dateTimeCodec.nullable() you'll want to put that in a variable so it doesn't construct a new codec instance each time.

BenWhitehead · 2022-06-03T17:55:49Z

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

+    if (from.getWebsite() != null
+        || from.getWebsite().getMainPageSuffix() != null
+        || from.getWebsite().getNotFoundPage() != null) {
+      to.setIndexPage(from.getWebsite().getMainPageSuffix());
+      to.setNotFoundPage(from.getWebsite().getNotFoundPage());
+    }


According to the proto definition, both of these properties are non-optional.

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

Lines 1520 to 1533 in 1f14acd

message Website {

// If the requested object path is missing, the service will ensure the path

// has a trailing '/', append this suffix, and attempt to retrieve the

// resulting object. This allows the creation of `index.html`

// objects to represent directory pages.

string main_page_suffix = 1;

// If the requested object path is missing, and any

// `mainPageSuffix` object is missing, if applicable, the service

// will return the named object from this bucket as the content for a

// [https://tools.ietf.org/html/rfc7231#section-6.5.4][404 Not Found]

// result.

string not_found_page = 2;

}

This if condition can be simplified to:

if (from.hasWebsite())

BenWhitehead · 2022-06-03T19:06:21Z

google-cloud-storage/src/test/java/com/google/cloud/storage/jqwik/StorageArbitraries.java

+                  actionBuilder.setStorageClass(s);
+                }
+                return actionBuilder.build();
+              });


I found the previous version easier to read, but if you feel this is easier feel free to keep it.

BenWhitehead

A few minor requests to update some of the javadocs. I'll approve so you can merge after updating those items.

BenWhitehead · 2022-06-03T19:11:33Z

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

         * truncated. This condition is satisfied when the custom time on an object is before this
         * date in UTC.
+         *
+         * @deprecated {@link #setCustomTimeBeforeOffsetDateTime(OffsetDateTime)}


Missing the Use on this statement like the others

BenWhitehead · 2022-06-03T19:11:51Z

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

      /**
       * Returns the date in RFC 3339 format with only the date part (for instance, "2013-01-15").
+       *
+       * @deprecated {@link #getNoncurrentTimeBeforeOffsetDateTime()}


Missing the Use on this statement like the others

BenWhitehead · 2022-06-03T19:14:04Z

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

+        /**
+         * Sets the date in RFC 3339 format with only the date part (for instance, "2013-01-15").
+         * Note that only date part will be considered, if the time is specified it will be
+         * truncated. This condition is satisfied when the noncurrent time on an object is before
+         * this date. This condition is relevant only for versioned objects.
+         */


Can we drop the RFC 3339 part of this statement?

BenWhitehead · 2022-06-03T19:14:22Z

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

+        }
+
+        /**
+         * Sets the date in RFC 3339 format with only the date part (for instance, "2013-01-15").


Can we drop the RFC 3339 part of this statement.

frankyn · 2022-06-06T18:01:42Z

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

+        Utils.toImmutableListOf(lifecycleRule()::encode),
+        lifecycleBuilder::addAllRule);
+    /*
+    // Corruption occurs by having deleteRule() values added


@BenWhitehead deleteRule() corrupts the policy and I commented it out. I can do a follow-up PR to better handle this case as this PR is long. WDYT?

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

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

frankyn force-pushed the bucket-info-feat branch 2 times, most recently from daa1619 to de73884 Compare June 3, 2022 16:39

feat: BucketInfo conversion for gRPC

df3df98

frankyn force-pushed the bucket-info-feat branch from de73884 to df3df98 Compare June 3, 2022 17:03

BenWhitehead requested changes Jun 3, 2022

View reviewed changes

address feedback

29d474e

BenWhitehead reviewed Jun 3, 2022

View reviewed changes

BenWhitehead approved these changes Jun 3, 2022

View reviewed changes

Frank Natividad added 5 commits June 6, 2022 09:53

fix retentionPolicy encode

5bc8e01

address feedback

8e86ba5

lint

7481a14

use correct setter

a2fdc31

include proto-google-common-protos

8bff2b9

frankyn marked this pull request as ready for review June 6, 2022 17:55

frankyn requested review from a team as code owners June 6, 2022 17:55

frankyn commented Jun 6, 2022

View reviewed changes

frankyn merged commit 500b73f into feat/grpc-storage Jun 6, 2022

frankyn deleted the bucket-info-feat branch June 6, 2022 20:05

	message Website {
	// If the requested object path is missing, the service will ensure the path
	// has a trailing '/', append this suffix, and attempt to retrieve the
	// resulting object. This allows the creation of `index.html`
	// objects to represent directory pages.
	string main_page_suffix = 1;

	// If the requested object path is missing, and any
	// `mainPageSuffix` object is missing, if applicable, the service
	// will return the named object from this bucket as the content for a
	// [https://tools.ietf.org/html/rfc7231#section-6.5.4][404 Not Found]
	// result.
	string not_found_page = 2;
	}

feat: BucketInfo conversion for gRPC #1431

feat: BucketInfo conversion for gRPC #1431

Uh oh!

Conversation

frankyn commented Jun 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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!

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!

BenWhitehead 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!

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!

2 participants

frankyn commented Jun 2, 2022 •

edited

Loading