- 
                Notifications
    You must be signed in to change notification settings 
- Fork 86
feat: Adding API for ListParts #3359
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
base: mpu-feature-2
Are you sure you want to change the base?
Conversation
        
          
                google-cloud-storage/src/main/java/com/google/cloud/storage/MultipartUploadClientImpl.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...d-storage/src/main/java/com/google/cloud/storage/multipartupload/model/ListPartsRequest.java
          
            Show resolved
            Hide resolved
        
              
          
                ...-storage/src/main/java/com/google/cloud/storage/multipartupload/model/ListPartsResponse.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...-storage/src/main/java/com/google/cloud/storage/multipartupload/model/ListPartsResponse.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...-storage/src/main/java/com/google/cloud/storage/multipartupload/model/ListPartsResponse.java
          
            Show resolved
            Hide resolved
        
              
          
                google-cloud-storage/src/main/java/com/google/cloud/storage/multipartupload/model/Part.java
          
            Show resolved
            Hide resolved
        
              
          
                google-cloud-storage/src/main/java/com/google/cloud/storage/multipartupload/model/Part.java
          
            Show resolved
            Hide resolved
        
              
          
                google-cloud-storage/src/main/java/com/google/cloud/storage/multipartupload/model/Part.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...-storage/src/test/java/com/google/cloud/storage/ITMultipartUploadHttpRequestManagerTest.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...-storage/src/test/java/com/google/cloud/storage/ITMultipartUploadHttpRequestManagerTest.java
          
            Show resolved
            Hide resolved
        
              
          
                ...-storage/src/main/java/com/google/cloud/storage/multipartupload/model/ListPartsResponse.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | @JsonCreator | ||
| public static StorageClass valueOf(String constant) { | ||
| if (constant == null || constant.isEmpty()) { | ||
| return null; | ||
| } | 
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.
Why is this necessary?
If I remove it and run the following test, the test passes:
  private static final class TestXmlObject2 {
    @JacksonXmlProperty(localName = "storageClass")
    private StorageClass storageClass;
  }
  @Test
  public void testParseStringValueEnum() throws IOException {
    //language=xml
    String xml = "<TestXmlObject2>\n"
        + "  <storageClass>STANDARD</storageClass>"
        + "</TestXmlObject2>";
    InputStream in = new ByteArrayInputStream(xml.getBytes(StandardCharsets.UTF_8));
    TestXmlObject2 expected = new TestXmlObject2(StorageClass.STANDARD);
    TestXmlObject2 actual =
        xmlObjectParser.parseAndClose(in, StandardCharsets.UTF_8, TestXmlObject2.class);
    assertThat(actual).isEqualTo(expected);
  }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.
This is required incase we have a null string. Try to run test https://paste.googleplex.com/5523315682836480 after removing it. You'll get https://paste.googleplex.com/5523315682836480 failure.
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.
Okay, but that is a deserialization issue not an enum resolution issue.
Enums can't have null values, and changing this method breaks that contract.
The exception explicitly states this fact:
Caused by: java.lang.IllegalArgumentException: Empty enum constants not allowed.
        at com.google.cloud.StringEnumType.valueOf(StringEnumType.java:66)
        at com.google.cloud.storage.StorageClass.valueOf(StorageClass.java:116)
Here is a PR that makes enum parsing work for xml without modifying any behavior or adding additional annotations: #3377
| I'll spend some time tomorrow figuring out the dependencies failing build. | 
| Applying the following patch to this branch should fix the failing dependency check Index: google-cloud-storage/pom.xml
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/google-cloud-storage/pom.xml b/google-cloud-storage/pom.xml
--- a/google-cloud-storage/pom.xml	(revision c7549b10d56e40c5576a6a7f471d7cd6f8bba397)
+++ b/google-cloud-storage/pom.xml	(date 1761754153688)
@@ -28,6 +28,14 @@
       <artifactId>jackson-datatype-jsr310</artifactId>
     </dependency>
     <dependency>
+      <groupId>com.fasterxml.jackson.core</groupId>
+      <artifactId>jackson-databind</artifactId>
+    </dependency>
+    <dependency>
+      <groupId>com.fasterxml.jackson.core</groupId>
+      <artifactId>jackson-annotations</artifactId>
+    </dependency>
+    <dependency>
       <groupId>com.google.guava</groupId>
       <artifactId>guava</artifactId>
     </dependency>The fix can be verified locally by running  | 
| Assuming my fixes in #3377 are included in the ultimate merge train, this PR can be considered done. | 
Refer: #3348