The service file parsing is incomplete #5203
Conversation
Added tests for service file parsing.
|
Nice work. Can you open a Ticket please so they can link the issue with this PR? |
| return null; | ||
| } else { | ||
| return line; | ||
| } |
There was a problem hiding this comment.
Here is what MyFaces does it might look a little cleaner...
int idx = serviceImplName.indexOf('#');
if (idx >= 0)
{
serviceImplName = serviceImplName.substring(0, idx);
}
serviceImplName = serviceImplName.trim();
if (serviceImplName.length() != 0)
{
results.add(serviceImplName);
}There was a problem hiding this comment.
I appreciate your effort, but MyFaces is under the Apache 2 License, which is to my understanding not really compatible with the EPL. I don't want to copy their code (even if they probably don't care).
There was a problem hiding this comment.
You don't need to quite copy it but you can see its a much simpler version. And Apache 2 is basically the most friendly open source license there is...
There was a problem hiding this comment.
Sure, the MyFaces code is simpler, it saves code by being worse and having some problems:
- It could immediately stop all processing, after knowing that the whole line is one single comment (
idx == 0), which happens very often, and is also the case in this project (e.g. in all #-comments in the Messages.properties). But instead it carries on, creates a substring copy to create an empty string, trims that empty string and afterwards checks if that empty string isn't empty. - Its complexity is according to SonarLint too high (like in Mojarra), but it still doesn't use an extra method and puts all code in the loop.
- It doesn't use the dedicated
isEmpty()method from Java 6, instead it uses the outdated.length != 0comparison.
In conclusion I think the MyFaces code isn't that good in comparison.
Thanks.
Done. Link: #5204 |
|
Thank you! |
|
Tried to fix the Eclipse verification, so I recreated the branch. |
Issue #5204.
According to the official documentation of Javas Service Loader it may contain comments. Everything which comes after the '#' is considered a comment. Reference: https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/ServiceLoader.html.
But Mojarras own Service Loader implementation doesn't support comments. It even doesn't skip blank lines. This leads to unexpected behaviour when using Mojarras Service Loader. When files under META-INF/services are loaded from Mojarras Service Loader it may lead to exceptions, while these service files under any other META-INF/services directory may perform perfect when they're loaded by the JDK Service Loader.
Therefore I implemented a Pull-Request which fixes that, so comments and blank lines don't lead to crashes when loading service files.
I also added unit tests to test the new functionality. To an area which had previously no test coverage at all.
I also replaced the magic string
"UTF-8"withStandardCharsets.UTF_8in the method I updated. It was introduced with Java 7 and can therefore also be used in a project where downwards compability is important.P.S.: I read, understood and accepted the ECA.