You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by GitBox <gi...@apache.org> on 2021/04/04 01:40:53 UTC
[GitHub] [jclouds] timuralp opened a new pull request #103: Do not assume Owner ID comes first in XML for S3
timuralp opened a new pull request #103:
URL: https://github.com/apache/jclouds/pull/103
When parsing S3 ListBuckets responses, jclouds should not implicitly
assume that the owner ID comes first when parsing the XML document. The
assumption triggers NullPointerException against Digital Ocean Spaces,
which returns the Owner information as
<Owner><DisplayName>value</DisplayName><ID>value</ID></Owner>.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jclouds] timuralp commented on a change in pull request #103: Do not assume Owner ID comes first in XML for S3
Posted by GitBox <gi...@apache.org>.
timuralp commented on a change in pull request #103:
URL: https://github.com/apache/jclouds/pull/103#discussion_r606745485
##########
File path: apis/s3/src/main/java/org/jclouds/s3/xml/ListAllMyBucketsHandler.java
##########
@@ -56,9 +57,18 @@ public ListAllMyBucketsHandler(DateService dateParser) {
public void endElement(String uri, String name, String qName) {
if (qName.equals("ID")) { // owner stuff
- currentOwner = new CanonicalUser(currentOrNull(currentText));
+ if (currentDisplayName == null) {
+ currentOwner = new CanonicalUser(currentOrNull(currentText));
+ } else {
+ currentOwner = new CanonicalUser(
+ currentOrNull(currentText), currentDisplayName);
+ }
} else if (qName.equals("DisplayName")) {
- currentOwner.setDisplayName(currentOrNull(currentText));
+ if (currentOwner == null) {
+ currentDisplayName = currentOrNull(currentText);
+ } else {
+ currentOwner.setDisplayName(currentOrNull(currentText));
Review comment:
I see, yes, that's better, although in the current patch and suggested patch there is the latent bug of having the `<Owner>` element appear _after_ the bucket listing. What do you think about adding an empty constructor for the `CanonicalUser` class and setting the values once they are parsed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jclouds] timuralp commented on a change in pull request #103: Do not assume Owner ID comes first in XML for S3
Posted by GitBox <gi...@apache.org>.
timuralp commented on a change in pull request #103:
URL: https://github.com/apache/jclouds/pull/103#discussion_r606752482
##########
File path: apis/s3/src/main/java/org/jclouds/s3/xml/ListAllMyBucketsHandler.java
##########
@@ -56,9 +57,18 @@ public ListAllMyBucketsHandler(DateService dateParser) {
public void endElement(String uri, String name, String qName) {
if (qName.equals("ID")) { // owner stuff
- currentOwner = new CanonicalUser(currentOrNull(currentText));
+ if (currentDisplayName == null) {
+ currentOwner = new CanonicalUser(currentOrNull(currentText));
+ } else {
+ currentOwner = new CanonicalUser(
+ currentOrNull(currentText), currentDisplayName);
+ }
} else if (qName.equals("DisplayName")) {
- currentOwner.setDisplayName(currentOrNull(currentText));
+ if (currentOwner == null) {
+ currentDisplayName = currentOrNull(currentText);
+ } else {
+ currentOwner.setDisplayName(currentOrNull(currentText));
Review comment:
I updated the patch with a test for a reverse order of <Buckets> and <Owner> tags and expanded the CanonicalUser class to allow setting the ID and a public constructor. This allowed me to change the parser to avoid relying on the order of the elements when processing the XML.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jclouds] gaul commented on a change in pull request #103: Do not assume Owner ID comes first in XML for S3
Posted by GitBox <gi...@apache.org>.
gaul commented on a change in pull request #103:
URL: https://github.com/apache/jclouds/pull/103#discussion_r606733441
##########
File path: apis/s3/src/main/java/org/jclouds/s3/xml/ListAllMyBucketsHandler.java
##########
@@ -56,9 +57,18 @@ public ListAllMyBucketsHandler(DateService dateParser) {
public void endElement(String uri, String name, String qName) {
if (qName.equals("ID")) { // owner stuff
- currentOwner = new CanonicalUser(currentOrNull(currentText));
+ if (currentDisplayName == null) {
+ currentOwner = new CanonicalUser(currentOrNull(currentText));
+ } else {
+ currentOwner = new CanonicalUser(
+ currentOrNull(currentText), currentDisplayName);
+ }
} else if (qName.equals("DisplayName")) {
- currentOwner.setDisplayName(currentOrNull(currentText));
+ if (currentOwner == null) {
+ currentDisplayName = currentOrNull(currentText);
+ } else {
+ currentOwner.setDisplayName(currentOrNull(currentText));
Review comment:
The original code is wrong here. Instead what I think would improve it is storing `id` and `displayName` as `String`s then putting this conditional in the "Bucket" clause. Note that you will want to set these fields to `null` as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jclouds] gaul merged pull request #103: Do not assume Owner ID comes first in XML for S3
Posted by GitBox <gi...@apache.org>.
gaul merged pull request #103:
URL: https://github.com/apache/jclouds/pull/103
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jclouds] timuralp commented on a change in pull request #103: Do not assume Owner ID comes first in XML for S3
Posted by GitBox <gi...@apache.org>.
timuralp commented on a change in pull request #103:
URL: https://github.com/apache/jclouds/pull/103#discussion_r606745485
##########
File path: apis/s3/src/main/java/org/jclouds/s3/xml/ListAllMyBucketsHandler.java
##########
@@ -56,9 +57,18 @@ public ListAllMyBucketsHandler(DateService dateParser) {
public void endElement(String uri, String name, String qName) {
if (qName.equals("ID")) { // owner stuff
- currentOwner = new CanonicalUser(currentOrNull(currentText));
+ if (currentDisplayName == null) {
+ currentOwner = new CanonicalUser(currentOrNull(currentText));
+ } else {
+ currentOwner = new CanonicalUser(
+ currentOrNull(currentText), currentDisplayName);
+ }
} else if (qName.equals("DisplayName")) {
- currentOwner.setDisplayName(currentOrNull(currentText));
+ if (currentOwner == null) {
+ currentDisplayName = currentOrNull(currentText);
+ } else {
+ currentOwner.setDisplayName(currentOrNull(currentText));
Review comment:
I see, yes, that's better, although in the current patch and suggested patch there is the latent bug of having the `<Owner>` element appear _after_ the `<Buckets>` list. What do you think about adding an empty constructor for the `CanonicalUser` class and setting the values once they are parsed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jclouds] gaul commented on pull request #103: Do not assume Owner ID comes first in XML for S3
Posted by GitBox <gi...@apache.org>.
gaul commented on pull request #103:
URL: https://github.com/apache/jclouds/pull/103#issuecomment-813014357
Thank you for your contribution @timuralp!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jclouds] gaul commented on a change in pull request #103: Do not assume Owner ID comes first in XML for S3
Posted by GitBox <gi...@apache.org>.
gaul commented on a change in pull request #103:
URL: https://github.com/apache/jclouds/pull/103#discussion_r606733441
##########
File path: apis/s3/src/main/java/org/jclouds/s3/xml/ListAllMyBucketsHandler.java
##########
@@ -56,9 +57,18 @@ public ListAllMyBucketsHandler(DateService dateParser) {
public void endElement(String uri, String name, String qName) {
if (qName.equals("ID")) { // owner stuff
- currentOwner = new CanonicalUser(currentOrNull(currentText));
+ if (currentDisplayName == null) {
+ currentOwner = new CanonicalUser(currentOrNull(currentText));
+ } else {
+ currentOwner = new CanonicalUser(
+ currentOrNull(currentText), currentDisplayName);
+ }
} else if (qName.equals("DisplayName")) {
- currentOwner.setDisplayName(currentOrNull(currentText));
+ if (currentOwner == null) {
+ currentDisplayName = currentOrNull(currentText);
+ } else {
+ currentOwner.setDisplayName(currentOrNull(currentText));
Review comment:
The original code is wrong here. Instead what I think would improve it is storing `id` and `displayName` as `String`s then putting this conditional in the Bucket clause. Note that you will want to set these fields to `null` as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org