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 2020/09/11 13:37:52 UTC
[GitHub] [jclouds] cstamas opened a new pull request #82: JCLOUDS-1552: Do not attempt to parse empty payload
cstamas opened a new pull request #82:
URL: https://github.com/apache/jclouds/pull/82
----------------------------------------------------------------
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] nacx commented on a change in pull request #82: JCLOUDS-1552: Do not attempt to parse empty payload
Posted by GitBox <gi...@apache.org>.
nacx commented on a change in pull request #82:
URL: https://github.com/apache/jclouds/pull/82#discussion_r487221374
##########
File path: apis/sts/src/test/java/org/jclouds/aws/util/AWSUtilsTest.java
##########
@@ -88,6 +95,20 @@ public void testNoExceptionParsingTextPlain() {
assertNull(utils.parseAWSErrorFromContent(command.getCurrentRequest(), response));
}
+ /**
+ * Do not attempt to parse empty payload.
+ */
+ @Test
+ public void testNoExceptionEmptyPayload() {
+ utils.logger = mock(Logger.class);
+ utils.logger.warn(anyObject(Throwable.class), anyString());
+ expectLastCall().andThrow(new IllegalStateException("log spam"));
+ replay(utils.logger);
+ HttpResponse response = HttpResponse.builder().statusCode(NOT_FOUND.getStatusCode()).payload(new StringPayload("")).build();
+ response.getPayload().getContentMetadata().setContentType("application/unknown");
+ assertNull(utils.parseAWSErrorFromContent(command.getCurrentRequest(), response));
+ }
Review comment:
Is there any way we can better test this? The whole purpose of the change shouldn't be to _not send logs_, but to avoid parsing a body that we know is empty.
Instead of mocking the logger, could we for example mock the response.getPayload to make sure it is not called more than once? Or something similar that helps us verify that the code reached the desired execution paths, without having to mess up with the logger.
----------------------------------------------------------------
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] cstamas commented on a change in pull request #82: JCLOUDS-1552: Do not attempt to parse empty payload
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #82:
URL: https://github.com/apache/jclouds/pull/82#discussion_r489376596
##########
File path: apis/sts/src/test/java/org/jclouds/aws/util/AWSUtilsTest.java
##########
@@ -88,6 +95,20 @@ public void testNoExceptionParsingTextPlain() {
assertNull(utils.parseAWSErrorFromContent(command.getCurrentRequest(), response));
}
+ /**
+ * Do not attempt to parse empty payload.
+ */
+ @Test
+ public void testNoExceptionEmptyPayload() {
+ utils.logger = mock(Logger.class);
+ utils.logger.warn(anyObject(Throwable.class), anyString());
+ expectLastCall().andThrow(new IllegalStateException("log spam"));
+ replay(utils.logger);
+ HttpResponse response = HttpResponse.builder().statusCode(NOT_FOUND.getStatusCode()).payload(new StringPayload("")).build();
+ response.getPayload().getContentMetadata().setContentType("application/unknown");
+ assertNull(utils.parseAWSErrorFromContent(command.getCurrentRequest(), response));
+ }
Review comment:
Hey, sorry for late response. Well yes, that could be made as well, but...
I really wanted to make sure that execution does not end up in `catch` branch, and there logger is the only thing I could get hold of :) I mean, in future the code may change (for example to reuse the `parseAWSErrorFromContent` byte[]), and then the mocked test would may become "false negative". This way, am sure my goal is achieve: there is no WARN log spam :smile:
But in general, am open to both changes.
----------------------------------------------------------------
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] nacx merged pull request #82: JCLOUDS-1552: Do not attempt to parse empty payload
Posted by GitBox <gi...@apache.org>.
nacx merged pull request #82:
URL: https://github.com/apache/jclouds/pull/82
----------------------------------------------------------------
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] nacx commented on pull request #82: JCLOUDS-1552: Do not attempt to parse empty payload
Posted by GitBox <gi...@apache.org>.
nacx commented on pull request #82:
URL: https://github.com/apache/jclouds/pull/82#issuecomment-696127269
Cherry-picked to 2.2.x 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] nacx merged pull request #82: JCLOUDS-1552: Do not attempt to parse empty payload
Posted by GitBox <gi...@apache.org>.
nacx merged pull request #82:
URL: https://github.com/apache/jclouds/pull/82
----------------------------------------------------------------
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] nacx commented on a change in pull request #82: JCLOUDS-1552: Do not attempt to parse empty payload
Posted by GitBox <gi...@apache.org>.
nacx commented on a change in pull request #82:
URL: https://github.com/apache/jclouds/pull/82#discussion_r487221374
##########
File path: apis/sts/src/test/java/org/jclouds/aws/util/AWSUtilsTest.java
##########
@@ -88,6 +95,20 @@ public void testNoExceptionParsingTextPlain() {
assertNull(utils.parseAWSErrorFromContent(command.getCurrentRequest(), response));
}
+ /**
+ * Do not attempt to parse empty payload.
+ */
+ @Test
+ public void testNoExceptionEmptyPayload() {
+ utils.logger = mock(Logger.class);
+ utils.logger.warn(anyObject(Throwable.class), anyString());
+ expectLastCall().andThrow(new IllegalStateException("log spam"));
+ replay(utils.logger);
+ HttpResponse response = HttpResponse.builder().statusCode(NOT_FOUND.getStatusCode()).payload(new StringPayload("")).build();
+ response.getPayload().getContentMetadata().setContentType("application/unknown");
+ assertNull(utils.parseAWSErrorFromContent(command.getCurrentRequest(), response));
+ }
Review comment:
Is there any way we can better test this? The whole purpose of the change shouldn't be to _not send logs_, but to avoid parsing a body that we know is empty.
Instead of mocking the logger, could we for example mock the response.getPayload to make sure it is not called more than once? Or something similar that helps us verify that the code reached the desired execution paths, without having to mess up with the logger.
----------------------------------------------------------------
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] cstamas commented on a change in pull request #82: JCLOUDS-1552: Do not attempt to parse empty payload
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #82:
URL: https://github.com/apache/jclouds/pull/82#discussion_r490751367
##########
File path: apis/sts/src/test/java/org/jclouds/aws/util/AWSUtilsTest.java
##########
@@ -88,6 +95,20 @@ public void testNoExceptionParsingTextPlain() {
assertNull(utils.parseAWSErrorFromContent(command.getCurrentRequest(), response));
}
+ /**
+ * Do not attempt to parse empty payload.
+ */
+ @Test
+ public void testNoExceptionEmptyPayload() {
+ utils.logger = mock(Logger.class);
+ utils.logger.warn(anyObject(Throwable.class), anyString());
+ expectLastCall().andThrow(new IllegalStateException("log spam"));
+ replay(utils.logger);
+ HttpResponse response = HttpResponse.builder().statusCode(NOT_FOUND.getStatusCode()).payload(new StringPayload("")).build();
+ response.getPayload().getContentMetadata().setContentType("application/unknown");
+ assertNull(utils.parseAWSErrorFromContent(command.getCurrentRequest(), response));
+ }
Review comment:
+1 will try to get it later afternoon
----------------------------------------------------------------
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] nacx commented on pull request #82: JCLOUDS-1552: Do not attempt to parse empty payload
Posted by GitBox <gi...@apache.org>.
nacx commented on pull request #82:
URL: https://github.com/apache/jclouds/pull/82#issuecomment-696127269
Cherry-picked to 2.2.x 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] nacx commented on a change in pull request #82: JCLOUDS-1552: Do not attempt to parse empty payload
Posted by GitBox <gi...@apache.org>.
nacx commented on a change in pull request #82:
URL: https://github.com/apache/jclouds/pull/82#discussion_r487221374
##########
File path: apis/sts/src/test/java/org/jclouds/aws/util/AWSUtilsTest.java
##########
@@ -88,6 +95,20 @@ public void testNoExceptionParsingTextPlain() {
assertNull(utils.parseAWSErrorFromContent(command.getCurrentRequest(), response));
}
+ /**
+ * Do not attempt to parse empty payload.
+ */
+ @Test
+ public void testNoExceptionEmptyPayload() {
+ utils.logger = mock(Logger.class);
+ utils.logger.warn(anyObject(Throwable.class), anyString());
+ expectLastCall().andThrow(new IllegalStateException("log spam"));
+ replay(utils.logger);
+ HttpResponse response = HttpResponse.builder().statusCode(NOT_FOUND.getStatusCode()).payload(new StringPayload("")).build();
+ response.getPayload().getContentMetadata().setContentType("application/unknown");
+ assertNull(utils.parseAWSErrorFromContent(command.getCurrentRequest(), response));
+ }
Review comment:
Is there any way we can better test this? The whole purpose of the change shouldn't be to _not send logs_, but to avoid parsing a body that we know is empty.
Instead of mocking the logger, could we for example mock the response.getPayload to make sure it is not called more than once? Or something similar that helps us verify that the code reached the desired execution paths, without having to mess up with the logger.
----------------------------------------------------------------
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] nacx commented on a change in pull request #82: JCLOUDS-1552: Do not attempt to parse empty payload
Posted by GitBox <gi...@apache.org>.
nacx commented on a change in pull request #82:
URL: https://github.com/apache/jclouds/pull/82#discussion_r490171464
##########
File path: apis/sts/src/test/java/org/jclouds/aws/util/AWSUtilsTest.java
##########
@@ -88,6 +95,20 @@ public void testNoExceptionParsingTextPlain() {
assertNull(utils.parseAWSErrorFromContent(command.getCurrentRequest(), response));
}
+ /**
+ * Do not attempt to parse empty payload.
+ */
+ @Test
+ public void testNoExceptionEmptyPayload() {
+ utils.logger = mock(Logger.class);
+ utils.logger.warn(anyObject(Throwable.class), anyString());
+ expectLastCall().andThrow(new IllegalStateException("log spam"));
+ replay(utils.logger);
+ HttpResponse response = HttpResponse.builder().statusCode(NOT_FOUND.getStatusCode()).payload(new StringPayload("")).build();
+ response.getPayload().getContentMetadata().setContentType("application/unknown");
+ assertNull(utils.parseAWSErrorFromContent(command.getCurrentRequest(), response));
+ }
Review comment:
So, I think the right way to approach this is a bit different. You don't wanna check that the code does not reach the `catch` branch. You wanna verify that it does not try to even parse the response payload. Having a try/catch there is anecdotical; the logic you wanna test is that if the payload is present but empty, the code doesn't attempt to parse it.
For that, looking at the code you could for example mock the factory that is used to get the apply function, or even provide a mock apply function and fail if it is called. I think this approach would much better test the behaviour we want to fix here. WDYT?
----------------------------------------------------------------
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] cstamas commented on a change in pull request #82: JCLOUDS-1552: Do not attempt to parse empty payload
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #82:
URL: https://github.com/apache/jclouds/pull/82#discussion_r489376596
##########
File path: apis/sts/src/test/java/org/jclouds/aws/util/AWSUtilsTest.java
##########
@@ -88,6 +95,20 @@ public void testNoExceptionParsingTextPlain() {
assertNull(utils.parseAWSErrorFromContent(command.getCurrentRequest(), response));
}
+ /**
+ * Do not attempt to parse empty payload.
+ */
+ @Test
+ public void testNoExceptionEmptyPayload() {
+ utils.logger = mock(Logger.class);
+ utils.logger.warn(anyObject(Throwable.class), anyString());
+ expectLastCall().andThrow(new IllegalStateException("log spam"));
+ replay(utils.logger);
+ HttpResponse response = HttpResponse.builder().statusCode(NOT_FOUND.getStatusCode()).payload(new StringPayload("")).build();
+ response.getPayload().getContentMetadata().setContentType("application/unknown");
+ assertNull(utils.parseAWSErrorFromContent(command.getCurrentRequest(), response));
+ }
Review comment:
Hey, sorry for late response. Well yes, that could be made as well, but...
I really wanted to make sure that execution does not end up in `catch` branch, and there logger is the only thing I could get hold of :) I mean, in future the code may change (for example the method `parseAWSErrorFromContent` may choose to reuse `actualPayload` byte[]), and then the mocked test would may become "false negative". This way, am sure my goal is achieve: there is no WARN log spam :smile:
But in general, am open to both changes.
----------------------------------------------------------------
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] nacx commented on a change in pull request #82: JCLOUDS-1552: Do not attempt to parse empty payload
Posted by GitBox <gi...@apache.org>.
nacx commented on a change in pull request #82:
URL: https://github.com/apache/jclouds/pull/82#discussion_r487221374
##########
File path: apis/sts/src/test/java/org/jclouds/aws/util/AWSUtilsTest.java
##########
@@ -88,6 +95,20 @@ public void testNoExceptionParsingTextPlain() {
assertNull(utils.parseAWSErrorFromContent(command.getCurrentRequest(), response));
}
+ /**
+ * Do not attempt to parse empty payload.
+ */
+ @Test
+ public void testNoExceptionEmptyPayload() {
+ utils.logger = mock(Logger.class);
+ utils.logger.warn(anyObject(Throwable.class), anyString());
+ expectLastCall().andThrow(new IllegalStateException("log spam"));
+ replay(utils.logger);
+ HttpResponse response = HttpResponse.builder().statusCode(NOT_FOUND.getStatusCode()).payload(new StringPayload("")).build();
+ response.getPayload().getContentMetadata().setContentType("application/unknown");
+ assertNull(utils.parseAWSErrorFromContent(command.getCurrentRequest(), response));
+ }
Review comment:
Is there any way we can better test this? The whole purpose of the change shouldn't be to _not send logs_, but to avoid parsing a body that we know is empty.
Instead of mocking the logger, could we for example mock the response.getPayload to make sure it is not called more than once? Or something similar that helps us verify that the code reached the desired execution paths, without having to mess up with the logger.
##########
File path: apis/sts/src/test/java/org/jclouds/aws/util/AWSUtilsTest.java
##########
@@ -88,6 +95,20 @@ public void testNoExceptionParsingTextPlain() {
assertNull(utils.parseAWSErrorFromContent(command.getCurrentRequest(), response));
}
+ /**
+ * Do not attempt to parse empty payload.
+ */
+ @Test
+ public void testNoExceptionEmptyPayload() {
+ utils.logger = mock(Logger.class);
+ utils.logger.warn(anyObject(Throwable.class), anyString());
+ expectLastCall().andThrow(new IllegalStateException("log spam"));
+ replay(utils.logger);
+ HttpResponse response = HttpResponse.builder().statusCode(NOT_FOUND.getStatusCode()).payload(new StringPayload("")).build();
+ response.getPayload().getContentMetadata().setContentType("application/unknown");
+ assertNull(utils.parseAWSErrorFromContent(command.getCurrentRequest(), response));
+ }
Review comment:
Is there any way we can better test this? The whole purpose of the change shouldn't be to _not send logs_, but to avoid parsing a body that we know is empty.
Instead of mocking the logger, could we for example mock the response.getPayload to make sure it is not called more than once? Or something similar that helps us verify that the code reached the desired execution paths, without having to mess up with the logger.
##########
File path: apis/sts/src/test/java/org/jclouds/aws/util/AWSUtilsTest.java
##########
@@ -88,6 +95,20 @@ public void testNoExceptionParsingTextPlain() {
assertNull(utils.parseAWSErrorFromContent(command.getCurrentRequest(), response));
}
+ /**
+ * Do not attempt to parse empty payload.
+ */
+ @Test
+ public void testNoExceptionEmptyPayload() {
+ utils.logger = mock(Logger.class);
+ utils.logger.warn(anyObject(Throwable.class), anyString());
+ expectLastCall().andThrow(new IllegalStateException("log spam"));
+ replay(utils.logger);
+ HttpResponse response = HttpResponse.builder().statusCode(NOT_FOUND.getStatusCode()).payload(new StringPayload("")).build();
+ response.getPayload().getContentMetadata().setContentType("application/unknown");
+ assertNull(utils.parseAWSErrorFromContent(command.getCurrentRequest(), response));
+ }
Review comment:
Is there any way we can better test this? The whole purpose of the change shouldn't be to _not send logs_, but to avoid parsing a body that we know is empty.
Instead of mocking the logger, could we for example mock the response.getPayload to make sure it is not called more than once? Or something similar that helps us verify that the code reached the desired execution paths, without having to mess up with the logger.
##########
File path: apis/sts/src/test/java/org/jclouds/aws/util/AWSUtilsTest.java
##########
@@ -88,6 +95,20 @@ public void testNoExceptionParsingTextPlain() {
assertNull(utils.parseAWSErrorFromContent(command.getCurrentRequest(), response));
}
+ /**
+ * Do not attempt to parse empty payload.
+ */
+ @Test
+ public void testNoExceptionEmptyPayload() {
+ utils.logger = mock(Logger.class);
+ utils.logger.warn(anyObject(Throwable.class), anyString());
+ expectLastCall().andThrow(new IllegalStateException("log spam"));
+ replay(utils.logger);
+ HttpResponse response = HttpResponse.builder().statusCode(NOT_FOUND.getStatusCode()).payload(new StringPayload("")).build();
+ response.getPayload().getContentMetadata().setContentType("application/unknown");
+ assertNull(utils.parseAWSErrorFromContent(command.getCurrentRequest(), response));
+ }
Review comment:
Is there any way we can better test this? The whole purpose of the change shouldn't be to _not send logs_, but to avoid parsing a body that we know is empty.
Instead of mocking the logger, could we for example mock the response.getPayload to make sure it is not called more than once? Or something similar that helps us verify that the code reached the desired execution paths, without having to mess up with the logger.
##########
File path: apis/sts/src/test/java/org/jclouds/aws/util/AWSUtilsTest.java
##########
@@ -88,6 +95,20 @@ public void testNoExceptionParsingTextPlain() {
assertNull(utils.parseAWSErrorFromContent(command.getCurrentRequest(), response));
}
+ /**
+ * Do not attempt to parse empty payload.
+ */
+ @Test
+ public void testNoExceptionEmptyPayload() {
+ utils.logger = mock(Logger.class);
+ utils.logger.warn(anyObject(Throwable.class), anyString());
+ expectLastCall().andThrow(new IllegalStateException("log spam"));
+ replay(utils.logger);
+ HttpResponse response = HttpResponse.builder().statusCode(NOT_FOUND.getStatusCode()).payload(new StringPayload("")).build();
+ response.getPayload().getContentMetadata().setContentType("application/unknown");
+ assertNull(utils.parseAWSErrorFromContent(command.getCurrentRequest(), response));
+ }
Review comment:
Is there any way we can better test this? The whole purpose of the change shouldn't be to _not send logs_, but to avoid parsing a body that we know is empty.
Instead of mocking the logger, could we for example mock the response.getPayload to make sure it is not called more than once? Or something similar that helps us verify that the code reached the desired execution paths, without having to mess up with the logger.
##########
File path: apis/sts/src/test/java/org/jclouds/aws/util/AWSUtilsTest.java
##########
@@ -88,6 +95,20 @@ public void testNoExceptionParsingTextPlain() {
assertNull(utils.parseAWSErrorFromContent(command.getCurrentRequest(), response));
}
+ /**
+ * Do not attempt to parse empty payload.
+ */
+ @Test
+ public void testNoExceptionEmptyPayload() {
+ utils.logger = mock(Logger.class);
+ utils.logger.warn(anyObject(Throwable.class), anyString());
+ expectLastCall().andThrow(new IllegalStateException("log spam"));
+ replay(utils.logger);
+ HttpResponse response = HttpResponse.builder().statusCode(NOT_FOUND.getStatusCode()).payload(new StringPayload("")).build();
+ response.getPayload().getContentMetadata().setContentType("application/unknown");
+ assertNull(utils.parseAWSErrorFromContent(command.getCurrentRequest(), response));
+ }
Review comment:
Is there any way we can better test this? The whole purpose of the change shouldn't be to _not send logs_, but to avoid parsing a body that we know is empty.
Instead of mocking the logger, could we for example mock the response.getPayload to make sure it is not called more than once? Or something similar that helps us verify that the code reached the desired execution paths, without having to mess up with the logger.
##########
File path: apis/sts/src/test/java/org/jclouds/aws/util/AWSUtilsTest.java
##########
@@ -88,6 +95,20 @@ public void testNoExceptionParsingTextPlain() {
assertNull(utils.parseAWSErrorFromContent(command.getCurrentRequest(), response));
}
+ /**
+ * Do not attempt to parse empty payload.
+ */
+ @Test
+ public void testNoExceptionEmptyPayload() {
+ utils.logger = mock(Logger.class);
+ utils.logger.warn(anyObject(Throwable.class), anyString());
+ expectLastCall().andThrow(new IllegalStateException("log spam"));
+ replay(utils.logger);
+ HttpResponse response = HttpResponse.builder().statusCode(NOT_FOUND.getStatusCode()).payload(new StringPayload("")).build();
+ response.getPayload().getContentMetadata().setContentType("application/unknown");
+ assertNull(utils.parseAWSErrorFromContent(command.getCurrentRequest(), response));
+ }
Review comment:
Is there any way we can better test this? The whole purpose of the change shouldn't be to _not send logs_, but to avoid parsing a body that we know is empty.
Instead of mocking the logger, could we for example mock the response.getPayload to make sure it is not called more than once? Or something similar that helps us verify that the code reached the desired execution paths, without having to mess up with the logger.
----------------------------------------------------------------
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