Skip to content

style: format imports across multiple files - #5200

Open
robfrank wants to merge 1 commit into
mainfrom
add-openrewrite-maven-plugin
Open

style: format imports across multiple files#5200
robfrank wants to merge 1 commit into
mainfrom
add-openrewrite-maven-plugin

Conversation

@robfrank

Copy link
Copy Markdown
Collaborator

What does this PR do?

A brief description of the change being made with this pull request.

Motivation

What inspired you to submit this pull request?

Related issues

A list of issues either fixed, containing architectural discussions, otherwise relevant
for this Pull Request.

Additional Notes

Anything else we should know when reviewing?

Checklist

  • I have run the build using mvn clean package command
  • My unit tests cover both failure and success scenarios

@mergify

mergify Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

@codacy-production

codacy-production Bot commented Jul 10, 2026

Copy link
Copy Markdown

Not up to standards ⛔

🔴 Issues 1 minor

Alerts:
⚠ 1 issue (≤ 0 issues of at least minor severity)

Results:
1 new issue

Category Results
CodeStyle 1 minor

View in Codacy

🟢 Metrics 10 complexity

Metric Results
Complexity 10

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

Comment thread engine/pom.xml
Comment on lines +152 to +157
<dependency>
<groupId>jakarta.annotation</groupId>
<artifactId>jakarta.annotation-api</artifactId>
<version>1.3.5</version>
<scope>provided</scope>
</dependency>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request modernizes the codebase by adopting Java 21 features, such as pattern matching for instanceof, List.getFirst(), and List.getLast(), alongside refactoring collection initializations to use Map.of() and List.of(). While these changes improve readability, the code review correctly identifies several critical issues where replacing HashMap or LinkedHashMap with Map.of() breaks insertion-order guarantees required for wire equality, or risks throwing NullPointerException due to potential null values. Additionally, some refactorings introduce unnecessary allocation overhead in performance-sensitive paths.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +509 to +511
final Map<String, Object> metadata = Map.of(
"server", "Neo4j/5.26.0 compatible (ArcadeDB " + Constants.getRawVersion() + ")",
"connection_id", "bolt-" + Thread.currentThread().threadId());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

critical

The comment explicitly states that the insertion order of the metadata is significant for wire equality. Replacing the LinkedHashMap with Map.of() breaks this guarantee because Map.of() has a randomized iteration order in Java. This will cause random failures in wire equality checks.

Suggested change
final Map<String, Object> metadata = Map.of(
"server", "Neo4j/5.26.0 compatible (ArcadeDB " + Constants.getRawVersion() + ")",
"connection_id", "bolt-" + Thread.currentThread().threadId());
final Map<String, Object> metadata = new LinkedHashMap<>();
metadata.put("server", "Neo4j/5.26.0 compatible (ArcadeDB " + Constants.getRawVersion() + ")");
metadata.put("connection_id", "bolt-" + Thread.currentThread().threadId());

Comment on lines +83 to +85
final Map<String, Object> result = new HashMap<>(Map.of(
"value", maxValue,
"items", maxItems));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

critical

Using Map.of() here will throw a NullPointerException if maxValue is null. In this method, maxValue is initialized to null and can remain null if no non-null items are processed. Map.of() does not permit null values.

Suggested change
final Map<String, Object> result = new HashMap<>(Map.of(
"value", maxValue,
"items", maxItems));
final Map<String, Object> result = new HashMap<>();
result.put("value", maxValue);
result.put("items", maxItems);

Comment on lines +83 to +85
final Map<String, Object> result = new HashMap<>(Map.of(
"value", minValue,
"items", minItems));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

critical

Using Map.of() here will throw a NullPointerException if minValue is null. In this method, minValue is initialized to null and can remain null if no non-null items are processed. Map.of() does not permit null values.

Suggested change
final Map<String, Object> result = new HashMap<>(Map.of(
"value", minValue,
"items", minItems));
final Map<String, Object> result = new HashMap<>();
result.put("value", minValue);
result.put("items", minItems);

Comment on lines +71 to +76
final Map<String, Object> vRel = new HashMap<>(Map.of(
"_type", "vRelationship",
"_id", "vRel:" + (++virtualIdCounter),
"_relType", type,
"_start", getNodeId(fromNode),
"_end", getNodeId(toNode)));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

critical

Using Map.of() here will throw a NullPointerException if getNodeId(fromNode) or getNodeId(toNode) returns null. getNodeId() explicitly returns null if the input node is null or does not match any expected type. Map.of() does not permit null values.

Suggested change
final Map<String, Object> vRel = new HashMap<>(Map.of(
"_type", "vRelationship",
"_id", "vRel:" + (++virtualIdCounter),
"_relType", type,
"_start", getNodeId(fromNode),
"_end", getNodeId(toNode)));
final Map<String, Object> vRel = new HashMap<>();
vRel.put("_type", "vRelationship");
vRel.put("_id", "vRel:" + (++virtualIdCounter));
vRel.put("_relType", type);
vRel.put("_start", getNodeId(fromNode));
vRel.put("_end", getNodeId(toNode));

Comment on lines +44 to +60
final Map<String, Object> map = new HashMap<>(Map.ofEntries(
Map.entry("writeTx", writeTx.get()),
Map.entry("readTx", readTx.get()),
Map.entry("txRollbacks", txRollbacks.get()),
Map.entry("createRecord", createRecord.get()),
Map.entry("readRecord", readRecord.get()),
Map.entry("updateRecord", updateRecord.get()),
Map.entry("deleteRecord", deleteRecord.get()),
Map.entry("existsRecord", existsRecord.get()),
Map.entry("queries", queries.get()),
Map.entry("commands", commands.get()),
Map.entry("scanType", scanType.get()),
Map.entry("scanBucket", scanBucket.get()),
Map.entry("iterateType", iterateType.get()),
Map.entry("iterateBucket", iterateBucket.get()),
Map.entry("countType", countType.get()),
Map.entry("countBucket", countBucket.get())));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Replacing the direct HashMap population with new HashMap<>(Map.ofEntries(...)) introduces significant performance and memory overhead. It allocates 16 KeyValueHolder objects, an array of size 16, an immutable map, and then copies them into a new HashMap. Reverting to new HashMap<>() and calling put() avoids all these intermediate allocations, which is crucial for a stats-gathering method that may be called frequently.

Suggested change
final Map<String, Object> map = new HashMap<>(Map.ofEntries(
Map.entry("writeTx", writeTx.get()),
Map.entry("readTx", readTx.get()),
Map.entry("txRollbacks", txRollbacks.get()),
Map.entry("createRecord", createRecord.get()),
Map.entry("readRecord", readRecord.get()),
Map.entry("updateRecord", updateRecord.get()),
Map.entry("deleteRecord", deleteRecord.get()),
Map.entry("existsRecord", existsRecord.get()),
Map.entry("queries", queries.get()),
Map.entry("commands", commands.get()),
Map.entry("scanType", scanType.get()),
Map.entry("scanBucket", scanBucket.get()),
Map.entry("iterateType", iterateType.get()),
Map.entry("iterateBucket", iterateBucket.get()),
Map.entry("countType", countType.get()),
Map.entry("countBucket", countBucket.get())));
final Map<String, Object> map = new HashMap<>();
map.put("writeTx", writeTx.get());
map.put("readTx", readTx.get());
map.put("txRollbacks", txRollbacks.get());
map.put("createRecord", createRecord.get());
map.put("readRecord", readRecord.get());
map.put("updateRecord", updateRecord.get());
map.put("deleteRecord", deleteRecord.get());
map.put("existsRecord", existsRecord.get());
map.put("queries", queries.get());
map.put("commands", commands.get());
map.put("scanType", scanType.get());
map.put("scanBucket", scanBucket.get());
map.put("iterateType", iterateType.get());
map.put("iterateBucket", iterateBucket.get());
map.put("countType", countType.get());
map.put("countBucket", countBucket.get());

@robfrank
robfrank force-pushed the add-openrewrite-maven-plugin branch from ba6bb22 to fb56988 Compare July 10, 2026 09:45
@claude

claude Bot commented Jul 10, 2026

Copy link
Copy Markdown

Automated review: OpenRewrite cleanup PR

Thanks for driving a codebase-wide cleanup - centralizing this in an OpenRewrite recipe is a nice, repeatable approach. That said, the size and true scope of this PR raise several concerns worth addressing before merge.

1. CI is currently red (blocker)
build-and-package, Check License Compliance, and Meterian client scan are all FAILURE, and unit-tests is SKIPPED. Given the blast radius (1,952 files, +9,541/-9,182), correctness rests almost entirely on the existing test suite passing. The build must be green before this can be safely merged.

2. Title/scope understate what this does
The title is style: format imports, but rewrite.yml runs semantic recipes, not just import ordering: UpgradeToJava21, JavaUtilAPIs, JavaConcurrentAPIs, UseTextBlocks, JUnitToAssertj, ShortenFullyQualifiedTypeReferences, FinalizePrivateFields, EqualsAvoidsNull, RemoveMethodsOnlyCallSuper, RemoveRedundantNullCheckBeforeInstanceof, LambdaBlockToExpression.
Please fill in the (currently empty) PR description and rename to reflect the real change. These recipes can alter behavior:

  • FinalizePrivateFields can break frameworks that mutate private fields via reflection/serialization.
  • RemoveMethodsOnlyCallSuper can drop overrides that exist for widened visibility, annotations, or Javadoc.
  • EqualsAvoidsNull flips the receiver (x.equals(k) becomes k.equals(x)), only safe when the constant equals semantics match.
    I also noticed test signatures widened from throws IOException to throws Exception - harmless but another sign this is more than formatting.

3. Please split this PR
9.5k lines across ~2k files in a single PR is effectively unreviewable, and it mixes mechanical and behavioral changes. Suggested split into individually reviewable PRs:

  1. Add the plugin + rewrite.yml + deps only.
  2. Pure mechanical import/format changes.
  3. Each semantic recipe (Java 21 migration, JUnitToAssertj, static-analysis rules) as its own PR, so a regression can be bisected to one recipe.

4. Non-reproducible plugin version
rewrite-migrate-java is declared with <version>RELEASE</version>, which makes builds non-deterministic. A rewrite-migrate-java.version property (3.39.0) is even declared in the parent pom but left unused - please pin to that property instead.

5. Wildcard imports introduced (~130 net new)
The OrderImports recipe collapsed explicit imports into wildcards (java.util.*, java.io.*, import static ...Mockito.*, etc.). This runs against the codebase convention of explicit imports (CLAUDE.md emphasizes explicit class names) and risks future hidden name conflicts. Consider configuring OrderImports with a high star threshold (e.g. 999 / 999 static) so imports stay explicit.

6. jakarta.annotation-api and the license failure

  • The javax.annotation to jakarta.annotation groupId swap is pinned at 1.3.5, which still ships the javax.annotation.* package (the real jakarta namespace begins at 2.x). So this is cosmetic; verify the generated gRPC code still resolves javax.annotation.Generated.
  • jakarta.annotation-api is dual-licensed (EPL/CDDL + GPLv2-with-classpath-exception). The root pom license-maven-plugin excludes GPL, which is a likely cause of the License Compliance failure - please check the plugin report. If you add any runtime-scoped dependency, remember CLAUDE.md requires updating ATTRIBUTIONS.md/NOTICE.

7. Minor pom nits

  • New lines in pom.xml, engine/pom.xml, e2e/pom.xml use inconsistent indentation (4 spaces vs the surrounding 8-16).
  • e2e/pom.xml hardcodes maven-compiler-plugin version 3.15.0, diverging from the centrally managed version - prefer managing it via parent pluginManagement/property.

Summary
The direction is good, but as-is this is hard to review, mislabeled, and failing CI. Getting the build green, pinning the plugin versions, deciding the wildcard-import policy, and splitting the semantic recipes into separate PRs would make this much safer to land.

Automated review; a human maintainer should confirm before merging.

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.

2 participants