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

Wildcard ban does not work? #76

Closed
yeikel opened this issue May 30, 2023 · 6 comments
Closed

Wildcard ban does not work? #76

yeikel opened this issue May 30, 2023 · 6 comments

Comments

@yeikel
Copy link
Contributor

yeikel commented May 30, 2023

Hello there,

This does not seem to work

<bannedImports>
<!---
Matches multiple dependencies that shade/relocate Google annotations
Examples : com.couchbase.client.core.deps.com.google.common.annotations.VisibleForTesting
org.testcontainers.shaded.com.google.common.annotations
-->
<bannedImport>*.com.google.common.annotations.VisibleForTesting</bannedImport>
</bannedImports>

As described by the comment, the goal is to match multiple dependencies that shade/relocate Google annotations like com.couchbase.client.core.deps.com.google.common.annotations.VisibleForTesting

@skuzzle
Copy link
Owner

skuzzle commented May 30, 2023

You probably need to prefix the pattern with **. A single * only matches a single package level.

@yeikel
Copy link
Contributor Author

yeikel commented May 30, 2023

Thank you for your help, but that does not seem to work unless I am missing something:

<groups>
<group>
<reason>Use org.jetbrains.annotations.VisibleForTesting</reason>
<basePackage>**</basePackage>
<bannedImports>
<!---
 Matches multiple dependencies that shade/relocate Google annotations
Examples : 

    com.couchbase.client.core.deps.com.google.common.annotations.VisibleForTesting
    org.testcontainers.shaded.com.google.common.annotations

-->
<bannedImport>**.com.google.common.annotations.VisibleForTesting</bannedImport>
</bannedImport>
</bannedImports>
</group>
</groups>
+import com.couchbase.client.core.deps.com.google.common.annotations.VisibleForTesting;
 import com.github.jasync.sql.db.pool.PooledObject;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
+
 import pgnio.QueryBuildConnection.Prepared;
 import pgnio.QueryMessage.Row;
 import pgnio.QueryReadyConnection.AutoCommit;
@@ -42,6 +44,7 @@ public class AutoCommitPooledObject implements PooledObject {
     // return autoCommit.ctx.io.isReadWrite();
     // }
 
+    @VisibleForTesting
     pu

This works though :

  <bannedImport>com.couchbase.client.core.deps.com.google.common.annotations.VisibleForTesting</bannedImport>
[ERROR] Reason: Use org.jetbrains.annotations.VisibleForTesting
[ERROR]         in file: pgnio/AutoCommitPooledObject.java
[ERROR]                 com.couchbase.client.core.deps.com.google.common.annotations.VisibleForTesting  (Line: 3, Matched by: com.couchbase.client.core.deps.com.google.common.annotations.VisibleForTesting)

@skuzzle
Copy link
Owner

skuzzle commented May 31, 2023

You are right, this is a bug. According to the documentation, **should match at least one package part. In this case however, it matches none because the package to match starts with com and com is also the first part in the pattern after the **.

There might be further issues with the way we currently implemented the matching algorithm. I created a reminder ticket (which needs some more specification) to improve this in the future: #78

@yeikel
Copy link
Contributor Author

yeikel commented May 31, 2023

Thank you for the fix. That did it

@yeikel
Copy link
Contributor Author

yeikel commented May 31, 2023

It seems that

<bannedImport>**.com.google.common.annotations.VisibleForTesting</bannedImport>

Does not match com.google.common.annotations.VisibleForTesting but I believe that this is expected, right?

@skuzzle
Copy link
Owner

skuzzle commented May 31, 2023

Yes, for the time being that is expected because ** will consume at least one package part while matching.
I guess the whole matching semantics should be reconsidered for the next major release. There are quite a few use cases that currently can not be expressed in a user friendly manor with the current implementation

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

2 participants