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

Excluding servlet-related dependencies (#1912) #2287

Merged

Conversation

pmauduit
Copy link
Contributor

@pmauduit pmauduit commented Nov 3, 2017

These jars should be provided by the servlet container.

Note: If Hadoop transitively requires jetty + jetty:jsp-api + servlet-api (in wfsfeature-harvester subproject), there is probably a good reason for this. I preferred to exclude the dependencies from the web/pom.xml instead of acting on each maven subprojects.

Note 2: some classes actually require the servlet-api jar, so I added it as a dependency, but keeping it away from the resulting artifact using the 'provided' scope.

Tests: compilation + runtime on web, no issue so far.

@fxprunayre fxprunayre added this to the 3.2.3 milestone Dec 6, 2017
@Delawen
Copy link
Contributor

Delawen commented Jan 11, 2018

Makes sense to remove dependencies that may break some containers.

@Delawen Delawen modified the milestones: 3.2.3, 3.4.3 Jun 27, 2018
@josegar74
Copy link
Member

@pmauduit would you mind to upgrade the PR for master branch?

Apologies about the late response.

These jars should be provided by the servlet container.

Note: If Hadoop transitively requires jetty + jetty:jsp-api +
servlet-api (in wfsfeature-harvester subproject), there is probably a
good reason for this. I preferred to exclude the dependencies from the
web/pom.xml instead of acting on each maven subprojects.

Note 2: some classes actually require the servlet-api jar, so I added it
as a dependency, but keeping it away from the resulting artifact using
the 'provided' scope.

Tests: compilation + runtime on web, no issue so far.
@pmauduit pmauduit force-pushed the fix-webapp-classloader-pollution-1912 branch from 833fafe to e82f7d5 Compare June 29, 2018 09:47
@pmauduit pmauduit changed the base branch from 3.2.x to master June 29, 2018 09:47
@pmauduit
Copy link
Contributor Author

Apologies about the late response.

No worries, done ;-)

Copy link
Contributor

@juanluisrp juanluisrp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking for javax.servlet.Servlet.class inside WEB-INF/lib/*.jar files and I can't find any occurrence, so it is not included anymore in the libraries distributed in the WAR.
Tested in Tomcat 9.0.8.0 and no warning about Servlet API is thrown.

@juanluisrp juanluisrp added this to the 3.6.0 milestone Jul 25, 2018
@juanluisrp juanluisrp merged commit 3bba23e into geonetwork:master Jul 25, 2018
@pmauduit pmauduit deleted the fix-webapp-classloader-pollution-1912 branch July 26, 2018 14:54
juanluisrp pushed a commit that referenced this pull request Jul 27, 2018
Cherry-picked 3bba23e.

These jars should be provided by the servlet container.

Note: If Hadoop transitively requires jetty + jetty:jsp-api +
servlet-api (in wfsfeature-harvester subproject), there is probably a
good reason for this. I preferred to exclude the dependencies from the
web/pom.xml instead of acting on each maven subprojects.

Note 2: some classes actually require the servlet-api jar, so I added it
as a dependency, but keeping it away from the resulting artifact using
the 'provided' scope.
@juanluisrp juanluisrp added enhancement backport 3.4.4 Apply when a feature has been backported from master branch to 3.4.4 release labels Jul 27, 2018
@juanluisrp
Copy link
Contributor

Backported to 3.4.x branch.

@etj
Copy link
Member

etj commented Aug 2, 2018

@juanluisrp wouldnt it be enough to set the javax.servlet.api as <scope>provided</scope>? It will not include the jar in the war file, nor it will be transitive, so it wouldnt need to be excluded.

Here an excerpt from the doc:

provided

This is much like compile, but indicates you expect the JDK or a container to provide the dependency > at runtime. For example, when building a web application for the Java Enterprise Edition, you would
set the dependency on the Servlet API and related Java EE APIs to scope provided because the web
container provides those classes. This scope is only available on the compilation and test classpath,
and is not transitive.

@juanluisrp
Copy link
Contributor

juanluisrp commented Aug 3, 2018

I've checked it in a commit previous to this PR and adding

    <dependency>
      <groupId>javax.servlet</groupId>
      <artifactId>javax.servlet-api</artifactId>
      <version>3.1.0</version>
      <scope>provided</scope>
    </dependency>
    <dependency>
      <groupId>org.mortbay.jetty</groupId>
      <artifactId>servlet-api</artifactId>
      <version>2.5-20081211</version>
      <scope>provided</scope>
    </dependency>
    <dependency>
      <groupId>org.mortbay.jetty</groupId>
      <artifactId>servlet-api-2.5</artifactId>
      <version>6.1.14</version>
      <scope>provided</scope>
    </dependency>
    <dependency>
      <groupId>org.mortbay.jetty</groupId>
      <artifactId>jsp-api-2.1</artifactId>
      <version>6.1.14</version>
      <scope>provided</scope>
    </dependency>

to web/pom.xml makes no servlet-api*.jar and jsp-api-2.1.jar be present in WEB-INF/lib.

I don't know which is the best approach, include as provided or use excludes.

@pmauduit
Copy link
Contributor Author

pmauduit commented Aug 3, 2018

In the pom snippet above, you seem to "exclude" a specific version of the unwanted jars ; the "exclude" approach makes sure that the dependency won't be included in the WAR regardless of the version. But feel free to adapt if you prefer using the provided scope, I have no objection on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 3.4.4 Apply when a feature has been backported from master branch to 3.4.4 release enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants