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

Some dependencies in /core module are in the wrong scope. #5581

Closed
HappyHacker123 opened this issue Apr 8, 2024 · 5 comments · Fixed by #5623
Closed

Some dependencies in /core module are in the wrong scope. #5581

HappyHacker123 opened this issue Apr 8, 2024 · 5 comments · Fixed by #5623
Labels
Milestone

Comments

@HappyHacker123
Copy link

Description

The following dependencies in /core are set to scope implementation, but the expected scope should api.

org.bouncycastle:bcprov-jdk15on 
com.google.guava:guava:32.1.3-jre 
io.netty:netty-transport-native-unix-common

According to gradle offcial doc, if a library contains classes that are exposed in the library binary interface, its dependency scope should be api. The above depedencies all contain classes exposed in binary interface as offical doc goes, I list one example of each dependency below.

  • Class AsymmetricKeyInfoConverter is used as public method parameter in core/src/main/java/com/linecorp/armeria/internal/common/util/MinifiedBouncyCastleProvider.java
  • Class io.netty.channel.unix.DomainSocketAddress is used as public method parameter in core/src/main/java/com/linecorp/armeria/common/util/DomainSocketAddress.java
  • Class ForwardingSet is extended in core/src/main/java/com/linecorp/armeria/common/DefaultCookies.java

Possible Outcome

This can lead to compilation failures in consumers, as mentioned in offical doc.

This keeps the dependencies off of the consumer’s compilation classpath. In addition, the consumers will immediately fail to compile if any implementation types accidentally leak into the public API.

Solution

Change the dependency scope from implementation to api.

@HappyHacker123
Copy link
Author

Could you help me review this issue @ikhoon @minwoox ? Many thanks :)

@ikhoon
Copy link
Contributor

ikhoon commented Apr 8, 2024

Class io.netty.channel.unix.DomainSocketAddress is used as public method parameter in core/src/main/java/com/linecorp/armeria/common/util/DomainSocketAddress.java

Good point. We need to fix it.

Class AsymmetricKeyInfoConverter is used as public method parameter in core/src/main/java/com/linecorp/armeria/internal/common/util/MinifiedBouncyCastleProvider.java

bouncycastle is shaded so it is not part of transitive dependencies and can't be api.
We may remove ConfigurableProvider from the super class or move MinifiedBouncyCastleProvider to internal.

Class ForwardingSet is extended in core/src/main/java/com/linecorp/armeria/common/DefaultCookies.java

DefaultCookies is a package private class so ForwardingSet doesn't have to be api.

@ikhoon ikhoon added the defect label Apr 8, 2024
@ikhoon ikhoon added this to the 1.28.0 milestone Apr 8, 2024
@HappyHacker123
Copy link
Author

@ikhoon Thanks a lot for your prompt response and clarification!

DefaultCookies is a package private class so ForwardingSet doesn't have to be api.

You're right, I'm sorry to give a wrong example. But here's another example regarding exposed classses in guava. Could you please take a look?

  • com.google.common.collect.ImmutableList.Builder in guava is used as public method parameter in core/src/main/java/com/linecorp/armeria/internal/common/metric/DefaultMeterIdPrefixFunction.java

Acually we found another dependency com.github.ben-manes.caffeine:caffeine whose scope may also need to be set to api due to the following usage.

  • com.github.benmanes.caffeine.cache.Cache in caffeine is used as public parameter in core/src/main/java/com/linecorp/armeria/internal/common/metric/CaffeineMetricSupport.java

@ikhoon
Copy link
Contributor

ikhoon commented Apr 9, 2024

Good question.

com.google.common.collect.ImmutableList.Builder in guava is used as public method parameter in

Though they have public visibility, classes under internal packages are internal classes.

* Various classes used internally. Anything in this package can be changed or removed at any time.

In addition to Bouncycastle, we also shadow Guava dependency. If it is exposed in the public API, we should fix it instead of making it api.

armeria/dependencies.toml

Lines 524 to 525 in a13d582

relocations = [
{ from = "com.google.common", to = "com.linecorp.armeria.internal.shaded.guava" },

@ikhoon ikhoon modified the milestones: 1.28.0, 1.29.0 Apr 9, 2024
@HappyHacker123
Copy link
Author

@ikhoon Thank you for your response :). I now have a thorough understanding of the situation, and the problem should be fixed according to armeria's actual implementation. Hope to hear it fixed soon :).

ikhoon pushed a commit that referenced this issue Apr 22, 2024
….unix.common) (#5623)

Motivation:

Explain why you're making this change and what problem you're trying to
solve.

Modifications:

From issue #5581, the class is exposed in the public API.
> Class io.netty.channel.unix.DomainSocketAddress is used as public
method parameter in
core/src/main/java/com/linecorp/armeria/common/util/DomainSocketAddress.java

Result:

- Closes #5581. (If this resolves the issue.)
- User can use [core]
com.linecorp.armeria.common.util.DomainSocketAddress#asNettyAddress
without adding dependency
Dogacel pushed a commit to Dogacel/armeria that referenced this issue Jun 8, 2024
….unix.common) (line#5623)

Motivation:

Explain why you're making this change and what problem you're trying to
solve.

Modifications:

From issue line#5581, the class is exposed in the public API.
> Class io.netty.channel.unix.DomainSocketAddress is used as public
method parameter in
core/src/main/java/com/linecorp/armeria/common/util/DomainSocketAddress.java

Result:

- Closes line#5581. (If this resolves the issue.)
- User can use [core]
com.linecorp.armeria.common.util.DomainSocketAddress#asNettyAddress
without adding dependency
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants