Add support for ServiceFinder on PackageNamesScanner #3818
Conversation
|
ECA signed @jansupol |
Signed-off-by: Luís Duarte <[email protected]>
|
Is Eclipse going to leave this project on the oven for too long? Seems to me that there is no interest in continuing with Jersey.... Should we change to RestEasy or some other JAX-RS implementation? It seems to be that Eclipse "saving" Java EE is being seen as free publicity because i see no activity in these projects. |
|
Please see our wiki, what to do with missing sign-off label. |
|
Can you elaborate on what does your change do? You put in a possibility to replace internal UriSchemeResourceFinderFactories with some other factories provided by SpringBoot? |
|
@jansupol. There is a PR in the previous Jersey Repo regarding this issue. Nevertheless, this PR allows you to override the Class Factories within jersey. Since Spring Boot v1.6 if I'm not mistaken, they've changed the way it's packaged, using BOOT-INF folder. And jersey even if you register new factories, it still forces you to use the old ones, causing spring boot to throw exceptions after jar packaging. This change is only to clear the default factories if new ones are provided. |
| @@ -316,4 +323,13 @@ private String toExternalForm(final URL u) { | |||
| } | |||
| return result.toString(); | |||
| } | |||
|
|
|||
| private static final Collection<UriSchemeResourceFinderFactory> defaultFinderFactories() { | |||
There was a problem hiding this comment.
I suppose here you were going to declare a constant but ended-up with private static final method instead. Please decide whether that shall be a constant (private static final) or method (private).
There was a problem hiding this comment.
I've declared this into a method because the Data Structure might change. Personally i don't like having for example new LinkedList() in the constant field.
| ServiceFinder<UriSchemeResourceFinderFactory> servicesFound = ServiceFinder.find(UriSchemeResourceFinderFactory.class); | ||
|
|
||
| // TODO: ServiceFinder should return an Optional | ||
| if (servicesFound.toArray().length == 0) { |
There was a problem hiding this comment.
I would write the whole construction shorter, like:
for (UriSchemeResourceFinderFactory s : ((servicesFound.toArray().length == 0) ?
defaultFinderFactories : servicesFound)) {
add(s);
}
but here could be objections from people who likes IFs...
There was a problem hiding this comment.
Either way for me it's fine. Personally, i think the code will be more readable with the If Statement there.
There was a problem hiding this comment.
Do you want me to apply your suggestion?
|
Pasting Jersey Legacy Repo PR for tracking purposes. https://github.com/jersey/jersey/pull/250 Pasting Spring issues: https://github.com/jersey/jersey/pull/196 |
| // for (UriSchemeResourceFinderFactory s : ServiceFinder.find(UriSchemeResourceFinderFactory.class)) { | ||
| // add(s); | ||
| // } | ||
| ServiceFinder<UriSchemeResourceFinderFactory> servicesFound = ServiceFinder.find(UriSchemeResourceFinderFactory.class); |
There was a problem hiding this comment.
This is dangerous. Suppose an environment with multiple JAX-RS applications. If one app adds a jar with UriSchemeResourceFinderFactory, it potentially breaks all other applications.
I believe this should be allowed on an application level only. Fortunately, PackageNamesScanner is instantiated from the ResourceConfig, so it would be possible to pass a boolean argument whether the feature is allowed or not. Something like:
new PackageNamesScanner(packages, recursive, isProperty(ServerProperties.URI_SCHEME_RESOURCE_FINDER_FACTORY_SERVICE_ENABLED) && !isProperty(ServerProperties.METAINF_SERVICES_LOOKUP_DISABLE))
There was a problem hiding this comment.
The dangerous thing here is that the PR implies the OR logic which excludes default resource finder factories when at least one custom factory is found.
The documentation at the top of the PackageNameScanner states that default resource finders shall not be excluded, but only custom resource finder factories could be added if any are found.
The whole algorithm of addition of such a factories works in the way that default factories come (are added) first, and then on top are added custom ones (if any). The add(..) method works so, that if new factory comes with existing scheme it will be used. So if custom factory comes with the scheme similar to the one in default implementation, the default will be replaced by a custom.
Thus the whole PR shall bring the only change
for (UriSchemeResourceFinderFactory s : ServiceFinder.find(UriSchemeResourceFinderFactory.class)) { add(s); }
it will be less dangerous at least.
And for sure tests shall be added for this modification.
There was a problem hiding this comment.
@senivam , so you're basically telling me that i cannot have any custom jar packaging?
Like i said, this PR is because Spring Boot uses BOOT-INF directory to store the App Logic, so it's isolated from Spring Boot's base classes see here.
While this isn't fixed, i've already migrated all the App to Spring MVC, and since JAX-RS is a "standard" i would prefer to use it.
There was a problem hiding this comment.
@driverpt nope, what I'm telling is when you are having your custom jar packaging it has priority over default. However default shall be included BEFORE your custom. Which is commented below the TODO (lines 125 - 127) in the original PackageNamedScanner . So, when you uncomment it (lines 125 -127), it will work as described in the documentation and as you are requiring - default jar resource factory finder will come first and afterwards it will be overwritten by your custom (when it's found by service finder).
There was a problem hiding this comment.
But that way the JarZipSchemeResourceFinderFactory will always run and cause an Exception to be Thrown
There was a problem hiding this comment.
The whole sequence of the PackageNameScanner looks like:
- Initialize default resource finders
- (not implemented, the aim of the current PR) initialize custom resource finders
- run (create(...) method of) initialized resource finders.
points 1 and 2 are related. Initialization of default and custom resource finders is mutually exclusive.
So yes, JarZipSchemeResourceFinderFactory will be initialized but it won't be run (it's create(...) method won't be called) if there would be found a custom resource finder which implements "jar", "zip", "wsjar" schemes. Because custom resource finder would remove default one from the finderFactories hash map.
No description provided.