feat(jspecify): complete the @NullMarked opt-in for test packages - #179
Merged
epugh merged 4 commits intoAug 18, 2026
Merged
Conversation
This was referenced Aug 2, 2026
adityamparikh
changed the title
Background: prior to this change only the root package `org.apache.solr.mcp.server` was @NullMarked via package-info.java. All sub-packages — `.collection`, `.config`, `.indexing`, `.indexing.documentcreator`, `.metadata`, `.search`, `.security`, `.util`, plus the test-only `.containerization` and `.observability` — were running unannotated, so NullAway (`OnlyNullMarked=true`) was silently skipping them. Changes: 1. Adds `package-info.java` with `@NullMarked` to every sub-package (10 files: 8 main + 2 test-only). 2. Annotates production code with `@Nullable` where return types, parameters, or record components legitimately admit null. The non-obvious nullable surfaces: - `CollectionUtils.getLong/getFloat/getInteger` return null on missing or unparseable keys. - `CollectionService` cache/handler metrics methods return null when the underlying Solr endpoint is unavailable (graceful degradation by design). - `SolrMetrics.cacheStats` / `handlerStats`, `CacheStats.*`, `CacheInfo.*`, `HandlerStats.*`, `HandlerInfo.*`, `SolrHealthStatus.errorMessage` / `responseTime` / `totalDocuments` / `solrVersion` / `status` — all documented as nullable, now annotated. - `IndexStats.numDocs` / `segmentCount` — Luke can return either field as null. - Optional `@McpToolParam(required = false)` parameters on `CollectionService.createCollection` and `SearchService.search`. - `JsonResponseParser.convertValue` — JSON null inputs. 3. `HttpSecurityConfiguration` Spring `@Value` fields initialised with defaults (`""` for `issuerUrl` matching the property default, `List.of()` for `allowedOrigins`) so the analyser can see they are always non-null after construction. 4. NullAway is explicitly disabled on `compileTestJava` (with an explanatory comment). The @NullMarked rollout exposes ~30 test sites that unbox or dereference values now declared @nullable in production (Map.get returns, metric fields that are null when an endpoint is unavailable, etc.). Each can be tightened by extracting to a local + `assertNotNull`, but the volume makes it a deliberate follow-up. Production is fully enforced. Build verified green (compile + unit + integration). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: adityamparikh <aditya.m.parikh@gmail.com>
The method unconditionally returns `new CacheStats(...)`; null-wrapping happens one level up in `fetchCacheMetrics` via the `isCacheStatsEmpty` check. The @nullable was misleading — readers expect a method marked @nullable to actually have a `return null` path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: adityamparikh <aditya.m.parikh@gmail.com>
@NullMarked on a package-info.java applies to that package only - Java packages are not hierarchical for annotation purposes. Combined with NullAway:OnlyNullMarked=true, that means an unmarked package is exempt from nullness checking even though NullAway is configured at error level. apache#133 marked every main package and two test packages (containerization, observability). This adds the six that were still unmarked, so every main and test package is now null-marked and the opt-in is uniform. NullAway remains disabled on compileTestJava, so this is declarative for now. Enabling it surfaces 48 test sites (the note in build.gradle.kts estimates ~30); that is left as a follow-up rather than folded in here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Aditya Parikh <aditya.m.parikh@gmail.com>
adityamparikh
force-pushed
the
chore/jspecify-test-packages-upstream
branch
from
August 18, 2026 21:23
a3104c6 to
b74f25b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
@NullMarkedon apackage-info.javaapplies to that package only — Javapackages are not hierarchical for annotation purposes. Combined with
NullAway:OnlyNullMarked=true, an unmarked package is exempt from nullnesschecking even though NullAway is configured at
errorlevel.#133 marks every main package plus two test packages (
containerization,observability). This adds the six that are still unmarked, so the opt-inis uniform across main and test:
org.apache.solr.mcp.server(test root)org.apache.solr.mcp.server.collectionorg.apache.solr.mcp.server.configorg.apache.solr.mcp.server.indexingorg.apache.solr.mcp.server.schemaorg.apache.solr.mcp.server.searchThat takes the test tree from 2/8 packages marked to 8/8.
Scope
Purely declarative. NullAway remains disabled on
compileTestJava(see thecomment #133 adds to
build.gradle.kts), so these files change no compilationoutcome today — they make the opt-in complete and consistent, so that enabling
test-side enforcement later is a one-line change rather than a rediscovery of
which packages were missed.
For sizing the follow-up: flipping that one line on this branch surfaces
48 NullAway errors across 14 test files, measured by temporarily replacing
the
compileTestJavadisable and running./gradlew compileTestJava --rerun-tasks. The~30in #133's newbuild.gradle.ktscomment is anunder-estimate — worth correcting there, but left alone here so this PR stays
additive-only.
Each site is a test unboxing or dereferencing a value that production code
declares
@Nullable(metrics fields that are null when a Solr endpoint isunavailable). They're mechanical to fix — extract a local,
assertNotNull—but 48 of them is its own PR, not a rider on this one.
Testing
./gradlew buildpasses on this branch (JDK 25, Testcontainers) — full suitegreen,
spotlessApplyreports no reformatting.