deps: do not re-declare grpc dependencies as test dependencies

[olavloite]

Removes the re-declaration of grpc-google-cloud-spanner-... as test dependencies as they are already transitive compile-time dependencies.

Fixes the failure in GoogleCloudPlatform/java-docs-samples#4278

[elharo]

I don't think this is correct. If they're not needed by this project's compile classpath, they should be added in the downstream project; not here.

[codecov]

Codecov Report

Merging #278 (8acd358) into master (825f186) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #278   +/-   ##
=========================================
  Coverage     71.40%   71.40%           
  Complexity     1104     1104           
=========================================
  Files            24       24           
  Lines          3396     3396           
  Branches        525      525           
=========================================
  Hits           2425     2425           
  Misses          756      756           
  Partials        215      215           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 825f186...8acd358. Read the comment docs.

[olavloite]

I don't think this is correct. If they're not needed by this project's compile classpath, they should be added in the downstream project; not here.

@elharo
Hmmm... That is true. It should however be a compile time dependency that should be included automatically as a transitive dependency from google-cloud-spanner. So I agree, adding it here is not the real solution.

It seems that there are a couple of problems here:

1. The reason that the grpc-google-cloud-spanner-admin-... dependencies were not considered transitive, was that they were added explicitly as test dependencies. Removing those as test dependencies is enough to solve this specific problem.
2. All other compile time dependencies currently declared in the JDBC pom can now also be removed, except for the dependency on google-cloud-spanner.
3. The JDBC pom imports a specific version of google-cloud-spanner-bom, which determines the version of the Spanner client library to use. That version will be used if the application only adds a specific version of google-cloud-spanner-jdbc as a dependency. That seems good to me.
4. An application that imports the libraries-bom and adds google-cloud-spanner-jdbc as a dependency will get the versions of google-cloud-spanner and google-cloud-spanner-jdbc that are defined in libraries-bom. The version of google-cloud-spanner in libraries-bom can be different from the one defined in google-cloud-spanner-jdbc. This is currently also the case, as google-cloud-spanner-jdbc imports version 2.0.2, while the libraries-bom specifies version 3.0.4. The latter version will be added as a compile time dependency for an application that defines google-cloud-spanner-jdbc as a dependency. That could be a problem.

@suztomo Any ideas what the best approach would be for the last point?

[olavloite]

The version that is used here will be determined by the version defined at line 72 for the import of google-cloud-spanner-bom (currently 2.0.2). That is the version of google-cloud-spanner that anyone using google-cloud-spanner-jdbc should be using. But:

• Any application that adds only google-cloud-spanner-jdbc (version 1.17.3) as a dependency without importing the libraries-bom will get google-cloud-spanner version 2.0.2.
• Any application that adds google-cloud-spanner-jdbc as a dependency and imports the libraries-bom, will get the versions of google-cloud-spanner-jdbc and google-cloud-spanner that are defined in the libraries-bom. The version of google-cloud-spanner in the BOM can be different from the one defined in google-cloud-spanner-jdbc.

[suztomo]

I agree with Elliotte's point. If some artifacts are missing proper dependencies, we should fix that there.

Let me dig your finding further. GoogleCloudPlatform/java-docs-samples#4278 (comment)

[olavloite]

I agree with Elliotte's point. If some artifacts are missing proper dependencies, we should fix that there.

Yes, I agree with that. But in this case it should be a transitive dependency, but seems to have lost that status. And that seems to be caused by the fact that it was added as a test dependency in the pom of this project, while it was already a compile-time dependency. I have not been able to find a satisfactory explanation as to why the dependency was still transitive in earlier versions of the JDBC driver, but not anymore.

[elharo]

I'm a little surprised Maven doesn't flag this issue (declaring the same dependency in multiple scopes) as an error.