Skip to content
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

Address review comments from Pull Request 25148 #25201

Closed
sawadood opened this issue May 4, 2023 · 8 comments · Fixed by #25205
Closed

Address review comments from Pull Request 25148 #25201

sawadood opened this issue May 4, 2023 · 8 comments · Fixed by #25205
Assignees

Comments

@sawadood
Copy link
Contributor

sawadood commented May 4, 2023

This issue is opened to address review comments from JAX-RS team from pull request: #25148

@sawadood sawadood self-assigned this May 4, 2023
@sawadood
Copy link
Contributor Author

sawadood commented May 4, 2023

Comment 1 from WhiteCat22:

@sawadood
Copy link
Contributor Author

sawadood commented May 4, 2023

Comment 1 from Whitecate22:
Can you confirm that org.apache.cxf.rest.message is present on MP Rest Client requests in addition to vanilla JAX-RS request please?
Yes, I have tested that org.apache.cxf.rest.message is present on MP REST client requests.

@sawadood
Copy link
Contributor Author

sawadood commented May 4, 2023

Comment 2 from Whitecat22:
Move "// Liberty Change End" from Line 160 to after line 178 of HttpsURLConnectionFactory.java.
OK, will make this change using this issue.

@sawadood
Copy link
Contributor Author

sawadood commented May 4, 2023

Comment 3 from Whitecat22:
Remove TS012061109 on line 77 of SSLUtils.java
OK, will make this update with this issue.

@sawadood
Copy link
Contributor Author

sawadood commented May 4, 2023

Comment 4 from WhiteCat22:
remove TS012061109 from Line 56 of SSLUtils.java.
OK< will make this update.

@sawadood
Copy link
Contributor Author

sawadood commented May 4, 2023

Comment # 5 from jim-krueger, related to (URLConnectionHTTPConduit.java):
It seems to me that this code section might be incorrect (but would not cause a problem in most cases). They are attempting to get the "Error Stream". Shouldn't the if check here be if (in != null)?
Also, I'd suggest changing your log message to "Ignoring unexpected exception in getInputStream() when an error stream is present: "
The code looks o to me. In the normal case, when there is no error, it's calling connection.getInputStream(). In the error case, it tries to call connection.getErrorStream() (which also returns the same type - InputStream). The code is checking if it cannot get error stream by checking if (in == null) and tries to get input stream again. I am not comfortable changing this code as it may cause regression.
The trace statement also looks ok to me. I don't think adding "when an error stream is present:" would add anything additional debug information.

@sawadood
Copy link
Contributor Author

sawadood commented May 4, 2023

Comment #6 from jim-krueger, related to HttpsURLConnectionFactory.java
Minor: Change get to create in Line 99 in trace.
OK, will make this change.

@sawadood
Copy link
Contributor Author

sawadood commented May 4, 2023

Comment #7 about line 191 in HttpsURLConnectionFactory:
if (verifier != null) {
LOG.fine("Hostname verifier obtained from SSLUtils.getHostnameVerifier: " + verifier.getClass().getCanonicalName());
}
jim-krueger's suggestion:
Suggestion: Remove the if check and change the line to the following so you'll know if it is null or not more easily:
LOG.fine(""Hostname verifier obtained from SSLUtils.getHostnameVerifier: " + (verifier==null? "null": verifier.getClass().getCanonicalName()));
WhiteCat22's suggestion:
I think we should leave the if check, but only print a message if the verifier is null.
I like jim-krueger's suggestion because it will log verifier for all cases, whether it's null or has a valid class name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant