-
Notifications
You must be signed in to change notification settings - Fork 304
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
PAYARA-3723 Fixing sonar blocker bugs with unclosed resources and potential NPE #3868
Conversation
Jenkins test please |
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.
Looks fine to me, just some formatting niggles
import org.glassfish.appclient.client.acc.JWSACCClassLoader; | ||
import org.glassfish.appclient.common.Util; | ||
|
||
import javax.swing.*; |
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.
Minor point, but is this necessary? Aren't we only using one class from here?
|
||
private byte[] serialize(Object obj) throws IOException { | ||
try(ByteArrayOutputStream bos = new ByteArrayOutputStream(); | ||
ObjectOutputStream oos = getFactory().createObjectOutputStream(bos)) { |
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.
Formatting is a little off here, makes this a bit hard to read as this line looks like it belongs with the ones below.
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 actually 2 Closables in one try-with-resources block. To have them both automatically closed. I have made now 2 try blocks to make clearer.
return JsonbBuilder | ||
.create() | ||
.fromJson(objectValue.toString(), type); | ||
try(Jsonb jsonb = JsonbBuilder.create()) { |
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.
Super minor, you missed a space between try
and (
@@ -245,6 +243,12 @@ private AuthenticationStatus validateAuthorizationCode(HttpMessageContext httpCo | |||
} | |||
} | |||
|
|||
private JsonObject readJsonObject(String tokensBody) { | |||
try(JsonReader reader = Json.createReader(new StringReader(tokensBody))) { |
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.
Same as other comment, just nitpicking formatting here
@Pandrex247 I have made the requested code changes. |
Jenkins test please |
No description provided.