Skip to content

fix: surface ServiceResponseError outside of RuntimeError #83

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 29, 2020
Merged

fix: surface ServiceResponseError outside of RuntimeError #83

merged 1 commit into from
Jul 29, 2020

Conversation

christiancompton
Copy link

@christiancompton christiancompton commented Jul 23, 2020 •
edited
Loading

This PR will allow ServiceResponseErrors to be returned directly, as opposed to in the causedBy: of a RuntimeException.

@CLAassistant
Copy link

CLAassistant commented Jul 23, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@christiancompton christiancompton changed the title [WIP] fix: surface ServiceResponseError outside of RuntimeError fix: surface ServiceResponseError outside of RuntimeError Jul 28, 2020
padamstx
padamstx suggested changes Jul 29, 2020
View reviewed changes
Copy link
Contributor

@padamstx padamstx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just had some suggested improvements to the test cases.

src/test/java/com/ibm/cloud/sdk/core/test/security/Cp4dAuthenticatorTest.java Outdated
// Calling authenticate should result in an exception.
try {
authenticator.authenticate(requestBuilder);
fail("Expected authenticate() to result in exception!");
} catch (RuntimeException excp) {
Throwable causedBy = excp.getCause();
assertNotNull(causedBy);
assertTrue(causedBy instanceof ServiceResponseException);
assertFalse(causedBy instanceof ServiceResponseException);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to check for the specific exception type that we'd expect in this "bad json response" scenario (i.e. a JsonParseException), rather than just make sure it's not a ServiceResponseException.

src/test/java/com/ibm/cloud/sdk/core/test/security/Cp4dAuthenticatorTest.java
try {
authenticator.authenticate(requestBuilder);
fail("Expected authenticate() to result in exception!");
} catch (RuntimeException excp) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
} catch (RuntimeException excp) {
} catch (ServiceResponseException excp) {

You could just catch the ServiceResponseException directly and call that a success, while anything else (the Throwable catch clause) would be an error.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should remain RuntimeExcpetion here, since the underlying error is IllegalStateException from the bad JSON. The ServiceResponse Exception changes have been made in both the testApiErrorBadRequest() locations where it is now surfaced.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just testing you :) HAHAHA

src/test/java/com/ibm/cloud/sdk/core/test/security/IamAuthenticatorTest.java Outdated
try {
authenticator.authenticate(requestBuilder);
fail("Expected authenticate() to result in exception!");
} catch (RuntimeException excp) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
} catch (RuntimeException excp) {
} catch (ServiceResponseException excp) {

Same comment as above.

src/test/java/com/ibm/cloud/sdk/core/test/security/IamAuthenticatorTest.java Outdated
@@ -348,7 +373,8 @@ public void testApiErrorBadRequest() throws Throwable {
} catch (RuntimeException excp) {
Throwable causedBy = excp.getCause();
assertNotNull(causedBy);
assertTrue(causedBy instanceof ServiceResponseException);
assertFalse(causedBy instanceof ServiceResponseException);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assertFalse(causedBy instanceof ServiceResponseException);
assertTrue(causedBy instanceof JsonParseException);

src/test/java/com/ibm/cloud/sdk/core/test/security/IamAuthenticatorTest.java Outdated
@@ -348,7 +373,8 @@ public void testApiErrorBadRequest() throws Throwable {
} catch (RuntimeException excp) {
Throwable causedBy = excp.getCause();
assertNotNull(causedBy);
assertTrue(causedBy instanceof ServiceResponseException);
assertFalse(causedBy instanceof ServiceResponseException);
assertFalse(excp instanceof ServiceResponseException);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assertFalse(excp instanceof ServiceResponseException);

I don't think this is needed if we correctly test the happy path.

padamstx
padamstx approved these changes Jul 29, 2020
View reviewed changes
Copy link
Contributor

@padamstx padamstx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

src/test/java/com/ibm/cloud/sdk/core/test/security/Cp4dAuthenticatorTest.java
try {
authenticator.authenticate(requestBuilder);
fail("Expected authenticate() to result in exception!");
} catch (RuntimeException excp) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just testing you :) HAHAHA

@christiancompton christiancompton merged commit 349ca1a into master Jul 29, 2020
@christiancompton christiancompton deleted the invalid_IAM branch July 29, 2020 21:08
ibm-devx-automation pushed a commit that referenced this pull request Jul 29, 2020
@semantic-release-bot
chore(release): 8.4.3 release notes
b32ee5c 
## [8.4.3](8.4.2...8.4.3) (2020-07-29)

### Bug Fixes

* surface ServiceResponseError outside of RuntimeError ([#83](#83)) ([349ca1a](349ca1a))
@ibm-devx-automation
Copy link

🎉 This PR is included in version 8.4.3 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@padamstx padamstx Awaiting requested review from padamstx

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@christiancompton @CLAassistant @ibm-devx-automation @padamstx