refactor (jspecify): roll @NullMarked out to all sub-packages#133
Open
adityamparikh wants to merge 1 commit into
Open
refactor (jspecify): roll @NullMarked out to all sub-packages#133adityamparikh wants to merge 1 commit into
adityamparikh wants to merge 1 commit into
Conversation
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>
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
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.
Summary
Extends JSpecify null-safety enforcement from the single root package
org.apache.solr.mcp.serverto every sub-package in the codebase — main and test alike.Before this change,
NullAway:OnlyNullMarked=truewas silently skipping all production code except the (mostly empty) root package. So the project advertised null-safety but only enforced it onMain.java.Changes
package-info.javawith@NullMarkedadded to 10 packages (8 main + 2 test-only):.collection,.config,.indexing,.indexing.documentcreator,.metadata,.search,.security,.util.containerization,.observabilityProduction code annotated with
@Nullablewhere appropriate, derived from running NullAway:CollectionUtils.getLong/getFloat/getInteger— return null on missing/unparseableCollectionServicecache/handler metrics methods — return null when the Solr endpoint is unavailable (graceful degradation by design)Dtos.javarecord components:SolrMetrics.{cacheStats,handlerStats}, all ofCacheStats/CacheInfo/HandlerStats/HandlerInfo,SolrHealthStatus.{errorMessage,responseTime,totalDocuments,solrVersion,status},IndexStats.{numDocs,segmentCount}@McpToolParam(required = false)parameters onCollectionService.createCollectionandSearchService.searchJsonResponseParser.convertValue— JSON null inputsCollectionService.isCacheStatsEmpty— accepts @nullableHttpSecurityConfigurationSpring@Valuefields initialised with defaults (""forissuerUrlmatching the property default,List.of()forallowedOrigins) so the analyser can see they are always non-null after construction.NullAway is explicitly disabled on
compileTestJavafor this PR (one-line inbuild.gradle.ktswith explanatory comment).@Nullablein production (Map.get returns, optional metric fields, etc.).assertNotNull, but the volume makes test-side cleanup a deliberate follow-up.Why draft
Draft because the test-side cleanup is intentionally deferred. Two reasonable paths forward:
@Nullableflow analysis requires extracting to locals +assertNotNull). Roughly 30 sites acrossCollectionServiceTest,CollectionServiceIntegrationTest,CollectionUtilsTest,SearchServiceIntegrationTest,ConferenceEndToEndIntegrationTest,IndexingServiceIntegrationTest,XmlIndexingTest,SchemaServiceTest,SchemaServiceIntegrationTest,TraceAssertions,DistributedTracingTest.Happy to do either.
Test plan
./gradlew build— full build green (compile + unit + integration)🤖 Generated with Claude Code