Skip to content

fix: expand wildcard imports (AvoidStarImport, 79 violations) - #171

Merged
sspaink merged 2 commits into
open-policy-agent:mainfrom
arimu1:fix/90-avoid-star-import
Jul 21, 2026
Merged

fix: expand wildcard imports (AvoidStarImport, 79 violations)#171
sspaink merged 2 commits into
open-policy-agent:mainfrom
arimu1:fix/90-avoid-star-import

Conversation

@arimu1

@arimu1 arimu1 commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Closes #90

Summary

Mechanically expands all import x.y.*; wildcard imports to explicit
imports (51 wildcard import lines across 35 files).

Per the issue's own stated resolution, two static wildcard imports are
kept as-is (idiomatic JUnit/Mockito style) and are now excluded from
AvoidStarImport via config/checkstyle/checkstyle.xml, so the check
reflects the intended scope going forward instead of silently ignoring
them:

  • org.junit.jupiter.api.Assertions.* (22 sites)
  • org.mockito.Mockito.* (6 sites)

org.mockito.ArgumentMatchers.* and java.lang.Math.* were not
called out in the issue as exceptions, so those were expanded like
everything else.

One dead wildcard import (io.github.open_policy_agent.opa.ast.builtin.impls.*
in CapabilitiesGenerator.java) was simply unused and removed rather
than expanded.

No reordering of unrelated imports, no reformatting of unrelated code
— only import lines and the one checkstyle config change.

Test plan

  • ./gradlew compileJava compileTestJavaBUILD SUCCESSFUL
  • ./gradlew checkstyleMain checkstyleTest — 0 AvoidStarImport /
    UnusedImports / RedundantImport violations (was 79
    AvoidStarImport violations before). Total checkstyle warning
    count: 514 (main) → 435 (branch), exactly the 79 removed
    star-import warnings, confirming no other checkstyle regressions.
  • ./gradlew buildBUILD SUCCESSFUL (full build incl. tests, PMD)

Expand all `import x.y.*;` wildcard imports to explicit imports
(51 wildcard import lines across 35 files). Two static wildcard
imports are kept as-is per idiomatic JUnit/Mockito style
(org.junit.jupiter.api.Assertions.* and org.mockito.Mockito.*, 28
sites) and are excluded from AvoidStarImport via checkstyle.xml so
the rule reflects the intended scope going forward.

Closes open-policy-agent#90

Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com>
@arimu1
arimu1 requested a review from a team as a code owner July 21, 2026 02:44
….random from ArithmeticBuiltins

The wildcard-expansion script that generated the explicit static imports
pulled in java.lang.Math.random alongside abs/ceil/floor/round, but
ArithmeticBuiltins never calls a bare random(); rand.intn() uses a local
java.util.Random instance instead. checkstyle's UnusedImports check
cannot catch this because the local variable is also named "random",
which fools its textual reference matching.

Manually audited the other 34 files touched by the wildcard-import fix
for the same pattern (named static import shadowed by a same-named
local/field): all remaining static imports (getArg across 8 builtin
files, ceil/floor/round/abs, assertArgType, assertEquals/assertTrue,
assertThat/assertThatThrownBy, any/anyString/contains/eq) have genuine
bare call sites and are not shadowed.

Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com>
@arimu1

arimu1 commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Fixed the one real defect I found on audit:

ArithmeticBuiltins.java:20import static java.lang.Math.random; was unused. The original java.lang.Math.* wildcard here only actually supplied abs (4 call sites), ceil (:125), floor (:170), and round (:147); there's no bare random() call anywhere in the file — rand.intn() uses a local java.util.Random random = new Random(seed) instead. checkstyle's UnusedImports can't catch this because it treats the local variable named random as satisfying the reference check on the import.

Manually audited the other 34 files this PR touches for the same shadow pattern (a newly-expanded static import matching a same-named local var/field with zero real call sites). Of those, 16 files actually contain any import static line; the rest have only regular type imports, which don't have this blind spot. Across the 16:

  • getArg (8 builtin files) — real call sites in every one.
  • abs/ceil/floor/round (ArithmeticBuiltins, TimeBuiltins) — real call sites (minus the random fix above).
  • assertArgType (ObjectBuiltins) — real call sites.
  • assertEquals/assertTrue (SprintfUtilTest), assertThat/assertThatThrownBy (RegoMapperTest) — real call sites.
  • any/anyString/contains/eq (DiscoveryPluginTest, explicit Mockito matcher imports alongside the intended Assertions.*/Mockito.* wildcards) — real call sites.

No other unused statics found.

Style note considered, not changed: the expansion script hoisted several files' new import static lines into the first regular-import block rather than grouping them together (e.g. ArithmeticBuiltins.java originally had statics sandwiched between two ordinary-import blocks). No ImportOrder/CustomImportOrder checkstyle rule exists in this repo, so it's silent either way, and the messiness isn't even consistent across files (ObjectBuiltins.java, for instance, already groups its statics cleanly at the end). Regrouping all 16 files mechanically would add diff noise for a purely cosmetic, unenforced concern, so I left it — happy to do it if a reviewer wants it.

Local verification (./gradlew compileJava compileTestJava checkstyleMain checkstyleTest build): BUILD SUCCESSFUL, 2838 tests / 0 failures / 0 errors (unchanged from before this fix), 0 duplicate imports, 0 remaining wildcards beyond the 2 intended (Assertions.* ×22, Mockito.* ×6). The only checkstyle findings anywhere in the touched modules are 17 pre-existing MethodNameCheck warnings on ceil_f/round_f/floor_f/abs_f (severity warning, non-blocking, unrelated to this change) plus similar pre-existing warnings elsewhere — nothing new from this diff.

CI caveat: only the DCO check has run so far (mergeStateStatus: BLOCKED, awaiting maintainer approval to run workflows for a first-time contributor) — no upstream CI has actually compiled or tested this yet. The build/test output above is from my local run standing in for that; please don't read the green DCO check as green CI.

Commit is signed off (DCO) as arimu1 <19286898+arimu1@users.noreply.github.com>.

@sspaink sspaink left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@sspaink
sspaink merged commit c75bba7 into open-policy-agent:main Jul 21, 2026
19 checks passed
sspaink added a commit that referenced this pull request Aug 7, 2026
main does not compile: BUILTIN_CLASSES references UriBuiltins, which
lives in the impls subpackage and so needs an explicit import.

#156 branched before #171 replaced the wildcard impls import with
explicit ones, so adding UriBuiltins.class compiled on that branch.
The two merged without a textual conflict, leaving the reference with
no import.

Signed-off-by: Sebastian Spaink <sebastianspaink@gmail.com>
sspaink added a commit to ume3445/java-opa-sdk that referenced this pull request Aug 11, 2026
The OKP branch added in 4fb00ae compiled on this branch's base, where
TokenBuiltins used a wildcard `com.nimbusds.jose.jwk.*` import. Main
has since expanded that wildcard (c75bba7, open-policy-agent#171) into explicit imports
that do not cover Curve or OctetKeyPair, so the merge builds cleanly in
git but fails javac. Add the two imports.

The compliance ratchet added on main also needs io.jwt.verify_eddsa
removed from known-missing-builtins.txt, since the builtin now resolves
and the suite fails on stale entries.

Squash before merge.

Signed-off-by: Sebastian Spaink <sebastianspaink@gmail.com>
sspaink added a commit that referenced this pull request Aug 11, 2026
* Implement io.jwt.verify_eddsa builtin (closes #145)

Signed-off-by: Muhammad Umer Hammad <umerhammad010@gmail.com>

* Address review: support Ed25519 (OKP) JWKs in extractPublicKey

Signed-off-by: Muhammad Umer Hammad <umerhammad010@gmail.com>

* Fixup into 4fb00ae: import Curve/OctetKeyPair, drop ratchet entry

The OKP branch added in 4fb00ae compiled on this branch's base, where
TokenBuiltins used a wildcard `com.nimbusds.jose.jwk.*` import. Main
has since expanded that wildcard (c75bba7, #171) into explicit imports
that do not cover Curve or OctetKeyPair, so the merge builds cleanly in
git but fails javac. Add the two imports.

The compliance ratchet added on main also needs io.jwt.verify_eddsa
removed from known-missing-builtins.txt, since the builtin now resolves
and the suite fails on stale entries.

Squash before merge.

Signed-off-by: Sebastian Spaink <sebastianspaink@gmail.com>

---------

Signed-off-by: Muhammad Umer Hammad <umerhammad010@gmail.com>
Signed-off-by: Sebastian Spaink <sebastianspaink@gmail.com>
Co-authored-by: Sebastian Spaink <sebastianspaink@gmail.com>
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

Successfully merging this pull request may close these issues.

Expand wildcard imports (AvoidStarImport, 79 violations)

2 participants