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

Make c3p0 and HarikiCP dependency as provided scope #306

Closed
zemian opened this issue Dec 28, 2018 · 24 comments
Closed

Make c3p0 and HarikiCP dependency as provided scope #306

zemian opened this issue Dec 28, 2018 · 24 comments
Labels
is:feature New feature

Comments

@zemian
Copy link
Contributor

zemian commented Dec 28, 2018

See #294 for more discussion as well.

@markkolich
Copy link

markkolich commented Dec 28, 2018

+1, thanks @zemian.

As discussed in #294 (comment) ...

HikariCP and c3p0 are currently in the compile scope. These are not "required" to use Quartz. They are effectively optional runtime dependencies if I understand their usage correctly. That said, I think it's a much cleaner approach to tell the users of this library "if you need to interact with a database, then you should add HikariCP to your pom". Or, "add c3p0 to your pom".

Then, in the Quartz pom, put these database dependencies in the provided scope with a version identifier that enforces a minimum provided version:

<dependency>
  <groupId>com.zaxxer</groupId>
  <artifactId>HikariCP</artifactId>
  <version>[2.3.0)</version>
  <scope>provided</scope>
</dependency>

In other words, users of Quartz will "provide" a compatible version of these dependencies if they need to interact with a database. Otherwise, these dependencies are kept local to Quartz, and won't transitively pollute any downstream dependency trees.

@wrwatson
Copy link

wrwatson commented Mar 1, 2019

This would be especially useful when vulnerabilities are reported against
HikariCP or c3p0. (e.g. https://nvd.nist.gov/vuln/detail/CVE-2018-20433)

We could avoid needing to investigate vulnerabilities in libraries we have as dependencies but are not actually using.

@ilgrosso
Copy link
Contributor

Big +1 for this

@PawelLipski
Copy link

PawelLipski commented Mar 27, 2019

@ilgrosso Correct me if I'm wrong... but the problem I can see with this approach: what if someone:

  1. uses HikariCP 3.x in the app (e.g. b/c they use Play Framework 2.7.x)
  2. pulls in Quartz that has been compiled against HikariCP 2.x but has HikariCP as provided dependency
  3. and then actually uses a Quartz data source backed by an SQL database?

It'll lead to classpath conflict in runtime IIUC.

@PawelLipski
Copy link

I'd even say that the better solution would be to emulate a Java 9/OSGi non-exported module by shading the HikariCP dependency into some obscure package like com.quartz.shaded.hikaricp... WDYT?

@ilgrosso
Copy link
Contributor

@PawelLipski not sure to follow, but I would presume that Quartz's code is using HikariCP only through standard's java.sql / javax.sql interfaces hence no conflicts at all.

Consider that in Apache Syncope we are simply excluding this transitive dependency and carrying over: https://github.com/apache/syncope/blob/master/pom.xml#L1284-L1294

If this current issue is fixed, we'll remove the <exclusions> element, and live happily.

@PawelLipski
Copy link

Looks there's at least one explicit reference to Hikari classes in non-test code: https://github.com/quartz-scheduler/quartz/blob/master/quartz-core/src/main/java/org/quartz/utils/HikariCpPoolingConnectionProvider.java#L20

As mentioned in the comment above... in the Apache Syncope project, are you using the Hikari-dependent functionality of Quartz? In our project we're just using akka-quartz-scheduler which AFAIK doesn't need anything related to SQL-backed Quartz data sources, so most likely the exclusion you recommend should be enough (let me check actually...).

@markkolich
Copy link

shading the HikariCP dependency into some obscure package like com.quartz.shaded.hikaricp

Instead of shading, perhaps a better split is to create a new quartz-db module. Then, move the HikariCP (and c3p0?) dependencies and related code (e.g., HikariCpPoolingConnectionProvider.java) into that new module.

When a user needs DB connectivity in conjunction with Quartz, they can explicitly add a dependency on quartz-db to pull in the required classes and dependencies. Other users, who don't need a Quartz data source backed by an SQL database, can ignore quartz-db entirely and avoid polluting their dependency trees.

@zemian
Copy link
Contributor Author

zemian commented Apr 1, 2019

I think for now, the easiest solution is simply make HikariCP as provided scope. For other suggestions, we might consider them as part of #416.

@zemian zemian added this to the Quartz 2.4.0 Release milestone Apr 1, 2019
zemian added a commit to zemian/quartz that referenced this issue Apr 1, 2019
@zemian
Copy link
Contributor Author

zemian commented Apr 2, 2019

The HarikiCP has been made "provided" scope with #418, but to make C3P0 also provided scope then it be more intrusive. We would need to at least provide a local provider that use direct JDBC connection as default. Then we can make C3P0 as "provided" scope.

@zemian
Copy link
Contributor Author

zemian commented Apr 2, 2019

Also, I have now added a doc on how to setup these connection provider properly, including the workaround of setup HarikiCP with maven exclude with the latest release. https://github.com/quartz-scheduler/quartz/wiki/How-to-Use-DB-Connection-Pool

@zemian zemian removed the medium label Apr 2, 2019
@markkolich
Copy link

markkolich commented Apr 2, 2019

but to make C3P0 also provided scope then it be more intrusive.

How so?

I quickly grep'ed the source tree and it looks like c3p0 is only referenced in a small handful of files, no more than HikariCP afaict.

A good first step is putting both HikariCP and c3p0 in the provided scope, then work on breaking out the DB scheduler related bits into a separate module, quartz-db, as discussed above.

@zemian
Copy link
Contributor Author

zemian commented Apr 2, 2019

What I mean "intrusive" is that "c3p0" has been part of quartz dep and package for a long time. Folks just change their quartz.properties and they get the db conn pull right a way. Making it provided scope will break people's upgrade unless they add c3p0 explicitly. Hence I say "intrusive". In fact, if we don't include c3p0, Quartz won't able to get any connection since there is no default iml.

I don't disagree with the request. Just that prob need a better doc and provide something default.

As the need for "quartz-db" is not 100% necessary if we going to make the lib in "provided" scope already.

@gavenkoa
Copy link

gavenkoa commented Dec 7, 2019

HikariCP-java7 & Spring Actuator depend on incompatible version of Micrometer. Just disabled HikariCP-java7:

api("org.quartz-scheduler:quartz:${projQuartzVersion}") {
    exclude group: "com.zaxxer", module: 'HikariCP-java6'
    exclude group: "com.zaxxer", module: 'HikariCP-java7'
    exclude group: "com.mchange"
}

@stale
Copy link

stale bot commented Aug 2, 2021

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale Inactive items that will be automatically closed if not resurrected label Aug 2, 2021
@markkolich
Copy link

This is definitely still relevant as far as I can tell.

@stale stale bot removed the stale Inactive items that will be automatically closed if not resurrected label Aug 3, 2021
@mathieucarbou mathieucarbou added is:feature New feature and removed feature request labels Sep 8, 2021
@chrisrueger
Copy link

we have a related issue:
We are using OSGI. We want to use Quartz v2.3.2 and latest hikaryCP 5.0.1

Unfortunatelly Quartz 2.3.2 has requires hikariCP in a lower version range, than we have.

Import-Package: com.zaxxer.hikari;version="[2.4,3)

Thus, quartz does not start in an OSGI container anymore.

Question:
Is it possible to declare the dependecy to HikariCP as optional?
We are not using quartz with a database (just Java), so for us it is purely optional for quartz.

In OSGI this would be achieved with:

Import-Package: com.zaxxer.hikari;resolution:=optional

Or could you just support latest hikariCP versions >=5.0.0?

@jhouserizer
Copy link
Contributor

Build system is being replaced with gradle, and have included generation of maven poms. the dependencies (such as Hikari) are ending up declared as "runtime" scope in the pom.

@gavenkoa
Copy link

Seems runtime dependency is not a solution as it is "hard" dependency:

https://maven.apache.org/guides/introduction/introduction-to-dependency-mechanism.html

This scope indicates that the dependency is not required for compilation, but is for execution. Maven includes a dependency with this scope in the runtime and test classpaths, but not the compile classpath.

So when you publish artifacts to Maven Central we still will suffer from unnecessary dependencies!

People asked for provided scope instead! Which is (according to the above link):

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. A dependency with this scope is added to the classpath used for compilation and test, but not the runtime classpath. It is not transitive.

@gavenkoa
Copy link

gavenkoa commented Apr 19, 2023

If I understand the solution correctly upstream just said: the feature won't be implemented. Nothing wrong, just state it clearly ))

Probably FAQ should have some example about listing & excluding extra dependencies, and how their presence or absence influences functionality.

@jhouserizer
Copy link
Contributor

woah, just realized when opening PR that I accidentally closed this a few hours ago. Intention is to properly resolve this but looking for validation from others around expectations.

This is what currently gets produced:

  <dependencies>
    <dependency>
      <groupId>org.slf4j</groupId>
      <artifactId>slf4j-api</artifactId>
      <version>1.7.36</version>
      <scope>runtime</scope>
    </dependency>
    <dependency>
      <groupId>com.mchange</groupId>
      <artifactId>c3p0</artifactId>
      <version>0.9.5.5</version>
      <scope>provided</scope>
    </dependency>
    <dependency>
      <groupId>com.zaxxer</groupId>
      <artifactId>HikariCP</artifactId>
      <version>5.0.1</version>
      <scope>provided</scope>
    </dependency>
    <dependency>
      <groupId>javax.xml</groupId>
      <artifactId>jaxb-api</artifactId>
      <version>2.1</version>
      <scope>runtime</scope>
    </dependency>
    <dependency>
      <groupId>org.slf4j</groupId>
      <artifactId>slf4j-log4j12</artifactId>
      <version>1.7.36</version>
      <scope>provided</scope>
    </dependency>
  </dependencies>

@jhouserizer jhouserizer reopened this Apr 19, 2023
@gavenkoa
Copy link

There are exclusions for SQL pool managers - what was asked by participants.

As for Slf4j it seems OK to have runtime scope. It was discussed in their FAQ:

https://www.slf4j.org/faq.html

In order to reduce the number of dependencies of our software we would like to make SLF4J an optional dependency. Is that a good idea?

I'm not sure why jaxb-api is here, hope renaming of lots XML related packages to Jakarta causes no harm/conflicts.

@jhouserizer
Copy link
Contributor

Thanks. I think the XML stuff might be able to be "provided" - it's only used if you use certain features of Quartz. Regarding Jakarta, there has to be another release of quartz (2.5.0) in the very near future to deal with all the jakarta renaming - affects JTA, Java Mail, and other things that Quartz uses.

@jhouserizer
Copy link
Contributor

Quartz 2.4.0 RC1 is released to maven central and on GitHub.

It would be nice if a few users test out the new pom which declares several dependencies differently than in past releases (using "provided" scope).

Before making the full/official release it would be good to know this is working as expected for users.

You can add the dependency with this version number:

org.quartz-scheduler quartz 2.4.0-rc1

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

Successfully merging a pull request may close this issue.

9 participants