-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
REST client with native and SSL #806
Conversation
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.
Seems OK but we'd want to ensure that the order of classes is preserved or otherwise stable in the ServiceUtil
change.
final Enumeration<URL> resources = classLoader.getResources(fileName); | ||
final Set<Class<?>> found = new HashSet<>(); | ||
|
||
final Set<String> classNames = new HashSet<>(); |
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 should either be a LinkedHashSet
(preferred) or a TreeSet
. Otherwise the build results could vary from build to build based on HashSet
's capacity and hashing algorithm of the day.
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.
@dmlloyd done.
@@ -29,17 +39,23 @@ private ServiceUtil() {} | |||
try (BufferedReader br = new BufferedReader(isr)) { | |||
String line; | |||
while ((line = br.readLine()) != null) { | |||
if (line.contains("#")) { |
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.
If we're changing this anyway... using indexOf
here and checking for -1
would mean the string is only traversed one time. Obviously not a big deal though.
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.
while at it .. Prefer indexOf('#') over indexOf("'#") :)
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.
Yeah, to be honest, I just centralized the best implementation we had without thinking twice about it.
I’ll improve it first thing tomorrow.
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.
ServiceUtil was very naive, not filtering empty lines and not taking into account comments. I moved the best implementation of that we had in the code base to ServiceUtil and it is now used consistently. Returning a Set as more flexible than an Iterable.
It initializes some stuff we don't want to initialize in the native image.
It's not the correct solution and we will fix that later but let's not invest too much time in it as the SmallRye REST client is coming.
This extension is not designed to be used externally and will probably go away once we migrate the REST client part to the reactive client. It will then be incorporated again in the RESTEasy (server) extension.
I just wanted to write the SSL guide and thought the REST client quickstart would be nice to base this guide on.
Unfortunately, it was a rabbit hole.
This PR changes a few various things:
ServiceUtil
a bit as I needed it, @dmlloyd could you take a look at this particular commit: ba3f1be ?Best reviewed commit per commit as they are not directly related even if they have the same goal: being able to use the REST client extension in native mode to access SSL services.
Hopefully, once we have moved to the SmallRye REST client, we will work a bit more on it to better optimize it.