Fix #12609: ProtoSession.Builder.newBuilder() NPE on unset property maps

[elharo]

Fixes #12609

Initialize userProperties and systemProperties to Map.of() in Builder to prevent NullPointerException when build() is called without setting these properties via newBuilder().

Also adds a test that verifies building a session without setting properties does not throw NPE.

[Copilot]

Pull request overview

Fixes a NullPointerException in the public ProtoSession.newBuilder() path by ensuring unset property maps are initialized to empty maps, aligning with the ProtoSession contract that these properties are never null.

Changes:

• Initialize ProtoSession.Builder’s userProperties and systemProperties to Map.of() to avoid Map.copyOf(null) in Impl.
• Add a JUnit test covering building a session from newBuilder() without explicitly setting property maps.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
api/maven-api-core/src/main/java/org/apache/maven/api/ProtoSession.java	Initializes default property maps in Builder to prevent NPE during build().
api/maven-api-core/src/test/java/org/apache/maven/api/ProtoSessionTest.java	Adds regression test ensuring newBuilder() build path works when properties are not explicitly set.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[gnodet]

LGTM ✅

Clean, minimal, and correct fix that initializes userProperties and systemProperties to Map.of() in the Builder, preventing the NullPointerException from Map.copyOf(null) in the Impl constructor and aligning with the @Nonnull contract on both getter methods.

Details:

• No other Builder fields suffer from the same silent-null-to-NPE issue: startTime is always set by newBuilder() via withStartTime(MonotonicClock.now()), topDirectory uses requireNonNull (which gives a clear error message), and rootDirectory is intentionally nullable.
• The effectiveProperties derived field also behaves correctly with empty defaults.
• One minor suggestion below to strengthen the test assertions.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

[gnodet]

Re-reviewed after 3 new commits — still LGTM ✅

The new commits are all test-only changes: added assertTrue(isEmpty()) assertions per Copilot feedback, applied gnodet's suggestion, and cleaned up resulting duplicates. The production code change is unchanged and correct — initializing Builder fields to Map.of() prevents Map.copyOf(null) NPE in the Impl constructor.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet