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

azure-keyvault (mssql-jdbc's dependencies) #17

Closed
martinm1000 opened this issue Nov 21, 2016 · 20 comments
Closed

azure-keyvault (mssql-jdbc's dependencies) #17

martinm1000 opened this issue Nov 21, 2016 · 20 comments
Assignees
Milestone

Comments

@martinm1000
Copy link

martinm1000 commented Nov 21, 2016

Not sure if this is alright, but I needed to exclude azure-keyvault since I do not use it, and this dependency was adding a lot more of its own deps to my final build.

Can you clarify those dependencies ? Maybe add this to the project README ? Thanks !

<dependency>
    <groupId>com.microsoft.sqlserver</groupId>
    <artifactId>mssql-jdbc</artifactId>
    <version>6.1.0.jre8</version>
    <exclusions>
        <exclusion>
            <groupId>com.microsoft.azure</groupId>
            <artifactId>azure-keyvault</artifactId>
        </exclusion>
    </exclusions>
</dependency>

Dependency tree report:

[INFO] | +- com.microsoft.sqlserver:mssql-jdbc:jar:6.1.0.jre8:compile
[INFO] |  \- com.microsoft.azure:azure-keyvault:jar:0.9.3:compile
[INFO] |     +- com.microsoft.azure:azure-core:jar:0.9.3:compile
[INFO] |     |  +- commons-codec:commons-codec:jar:1.10:compile
[INFO] |     |  +- commons-lang:commons-lang:jar:2.6:compile
[INFO] |     |  +- javax.mail:mail:jar:1.4.5:compile
[INFO] |     |  |  \- javax.activation:activation:jar:1.1:compile
[INFO] |     |  +- com.sun.jersey:jersey-client:jar:1.13:compile
[INFO] |     |  |  \- com.sun.jersey:jersey-core:jar:1.13:compile
[INFO] |     |  \- com.sun.jersey:jersey-json:jar:1.13:compile
[INFO] |     |     +- org.codehaus.jettison:jettison:jar:1.1:compile
[INFO] |     |     |  \- stax:stax-api:jar:1.0.1:compile
[INFO] |     |     +- com.sun.xml.bind:jaxb-impl:jar:2.2.3-1:compile
[INFO] |     |     |  \- javax.xml.bind:jaxb-api:jar:2.2.2:compile
[INFO] |     |     |     \- javax.xml.stream:stax-api:jar:1.0-2:compile
[INFO] |     |     +- org.codehaus.jackson:jackson-core-asl:jar:1.9.2:compile
[INFO] |     |     +- org.codehaus.jackson:jackson-mapper-asl:jar:1.9.2:compile
[INFO] |     |     +- org.codehaus.jackson:jackson-jaxrs:jar:1.9.2:compile
[INFO] |     |     \- org.codehaus.jackson:jackson-xc:jar:1.9.2:compile
[INFO] |     +- org.apache.httpcomponents:httpclient:jar:4.3.6:compile
[INFO] |     |  +- org.apache.httpcomponents:httpcore:jar:4.3.3:compile
[INFO] |     |  \- commons-logging:commons-logging:jar:1.1.3:compile
[INFO] |     +- javax.inject:javax.inject:jar:1:compile
[INFO] |     \- com.microsoft.azure:adal4j:jar:1.0.0:compile
[INFO] |        +- com.nimbusds:oauth2-oidc-sdk:jar:4.5:compile
[INFO] |        |  +- net.jcip:jcip-annotations:jar:1.0:compile
[INFO] |        |  +- org.apache.commons:commons-lang3:jar:3.3.1:compile
[INFO] |        |  +- net.minidev:json-smart:jar:1.1.1:compile
[INFO] |        |  +- com.nimbusds:lang-tag:jar:1.4:compile
[INFO] |        |  \- com.nimbusds:nimbus-jose-jwt:jar:3.1.2:compile
[INFO] |        +- com.google.code.gson:gson:jar:2.2.4:compile
[INFO] |        \- org.slf4j:slf4j-api:jar:1.7.5:compile
@nsidhaye
Copy link
Contributor

For Building mssql-jdbc we need azure-core and azure-keyvault for compiling purpose.

Are you suggesting adding this exclusion will remove transitive dependency tree ?

@martinm1000
Copy link
Author

Well yes, for me over here its just the jdbc driver jar file that I need to use, at least for now, not sure in the future what I'll need to connect though Azure SQL Database.

@marschall
Copy link
Contributor

marschall commented Nov 22, 2016

May I suggest Maven optional dependencies, they seem like a perfect fit for your problem.

@martinm1000
Copy link
Author

Well exclusion work for now; I just don't know exactly what I excluded myself from using from mssql-jdbc ;-)

@simdevmon
Copy link

I had the same issue and needed to exclude azure-keyvault, since it is using Jersey client 1.13, which is conflicting with the latest Jersey 2.x dependencies in my project.

A note in the README would be helpful.

@v-nisidh
Copy link
Contributor

@simdevmon , @martinm1000 : I completely agree with you. We will add one section in README related to dependencies and how to exclude those dependencies.

@v-nisidh v-nisidh assigned v-nisidh and unassigned v-nisidh Nov 22, 2016
@xiangyushawn
Copy link
Contributor

xiangyushawn commented Nov 22, 2016

Thank you for filling this issue. We will definitely add a section in README to explain this.

this dependency azure-keyvault is for Always Encrypted Azure Key Vault feature, if you do not need this feature, you can exclude KeyVaultCredential.java and SQLServerColumnEncryptionAzureKeyVaultProvider.java from your classpath, then you will be able to exclude this dependency.

@martinm1000
Copy link
Author

@v-xiangs : I'm only using the jars, I'm not sure how one could exclude 1 class or 2; my main issue here is that unless its officially documented, I can't be sure that I won't get ClassNotFound exceptions in runtime while using the jdbc driver (for SQL Server). But looks like I've been doing it without its deps before maven with no problems.

@v-nisidh v-nisidh self-assigned this Nov 22, 2016
v-nisidh added a commit to v-nisidh/mssql-jdbc that referenced this issue Nov 22, 2016
azure-keyvault (mssql-jdbc's dependencies) microsoft#17 : Adding section in
README related to dependency
@vyazelenko
Copy link

@v-xiangs Why not split a project into two modules: core and azure? Then people who don't use extra features would simply use core only.

@xiangyushawn
Copy link
Contributor

@vyazelenko thank you for your brilliant idea, we will definitely think about this.

@gstojsic
Copy link
Contributor

gstojsic commented Nov 27, 2016

I believe that exclusion is the wrong way to go. Forcing everybody that does not use azure (which I believe is the majority of developers out there) to write those ugly exclusions in the pom is just wrong. You should go with the optional dependency solution @marschall suggested. The fix is relatively simple. You just have to write this in the driver pom:
<dependency>
<groupId>com.microsoft.azure</groupId>
<artifactId>azure-keyvault</artifactId>
<version>0.9.3</version>
<optional>true</optional>
</dependency>
This way there is way less bloat (the difference is about 10Mb), and only those who want to use the azure features have to specify this in their pom. Also, say that there is a bug in the azure jar. The driver references the buggy version and you want to use the fixed one. Now you have to exclude the driver version and add your own all the same.
I can make a pull request if You like.
For some reason @marschall 's link does not work for me. Here is the proper Maven optional dependancy explanation.

@v-nisidh
Copy link
Contributor

@gstojsic : Thanks for your suggestion. It's really simple and straightforward. Let me test how it works if any project having dependency on azure-keyvault (Project using keyvault related features)
I believe in that case project should add azure-keyvault dependency specifically.

@gstojsic
Copy link
Contributor

@v-nisidh Yes. Projects using azure-keyvault features will have to specifically add the dependency. But that is much better than forcing 10 Mb of jars on everyone else or forcing them to exclude dependencies. Of course there should be a readme section explaining people to add the dependency in their project (pom) if they wish to use azure-keyvault features.

@ajlam
Copy link
Member

ajlam commented Nov 29, 2016

@gstojsic - before we proceed with deciding to exclude these dependencies from the jar, we are working through the use cases of Azure Key Vault features with the JDBC driver. We'll continue weighing the different alternatives of including/excluding these dependencies and provide an update.

@gstojsic
Copy link
Contributor

@ajlam Of course you have to weigh the pros and cons. It would be nice to hear from other developers reading this. What do they think? Which solution do they prefer?

@martinm1000
Copy link
Author

@gstojsic Well if those Azure deps were already existing before this lib was under maven, most developers already didn't use or even know about those dependencies, so I would vote for the Optional option.

@ajlam ajlam self-assigned this Dec 5, 2016
@v-nisidh v-nisidh modified the milestone: Long Term Goals Feb 6, 2017
@basil
Copy link

basil commented Feb 7, 2017

We just hit this problem. The Azure dependencies transitively depend on Jersey 1, which is classpath-incompatible with Jersey 2. Since our application relies on Jersey 2, we had to exclude Azure dependencies. We'd definitely appreciate splitting the Azure support into a separate package. Another option to consider is whether it's possible to bundle the required Jersey and/or JAX-RS classes into a private namespace, much like what was done in the Testcontainers project.

@v-nisidh v-nisidh modified the milestones: 6.1.5, Long Term Goals Feb 14, 2017
@xiangyushawn
Copy link
Contributor

@martinm1000 We revisited this issue and really liked @gstojsic 's idea to make the dependency optional. We submitted a PR #148 for this and also updated the ReadMe file in the PR. Please let us know if it works for you.

@v-nisidh
Copy link
Contributor

Thanks @marschall @martinm1000 . Closing this issue with #148

@ajlam
Copy link
Member

ajlam commented Feb 24, 2017

The JDBC team would like to learn more about the Java & SQL Server configuration you're using to help guide priorities going forward. If you have a few minutes, would you please take this survey?

rene-ye added a commit that referenced this issue Mar 6, 2018
VeryVerySpicy pushed a commit to VeryVerySpicy/mssql-jdbc that referenced this issue Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants