adityamparikh opened a new pull request, #267:
URL: https://github.com/apache/solr-mcp/pull/267
Stacked on #266 (google-java-format): the first commit here is #266, and
only the top commit (`fix: remove unreachable null checks and declare real
nullability`) is new.
## What
Fixes IntelliJ's nullness warnings across `src/main` and `src/test`, the
conditions it reports as always true or always false, and the dead code next to
them. Every warning was one of two things, and each kind gets its own fix:
- **A null check on an input that can't be null:** the check is removed.
- **A type claiming non-null for a value that can be null:** the type is
corrected and the check stays.
### Null checks removed
Required `@McpToolParam`/`@McpArg` parameters no longer null-check. MCP SDK
2.0 validates tool arguments against the input schema before dispatch.
`CompleteRequest` and `CompleteArgument` also reject a null argument or value
in their constructors. The blank and empty checks stay.
Affected:
- `AliasService`: create and delete alias
- `CollectionService`: `createCollection`, `completeCollection`,
`extractCollectionName`, `isHandlerStatsEmpty`
- `SchemaService`: collection and list validation
- `IndexingService`: Markdown documents, prompt format
- `JsonDocumentCreator` and `IndexingDocumentCreator`
- `SearchService`: sort clauses
### Types corrected; checks kept
| Where | Why it can be null |
|---|---|
| `purpose`, `sample`, `sampleDocument` prompt arguments;
`PromptText.optionalCodeBlock(content)` | optional `@McpArg`s |
| `CollectionUtils.getLong/getFloat/getInteger` now take `NamedList<?>`;
`extractSingleCacheInfo`, `fetchMetrics` | `NamedList.get` returns null for a
missing key |
| `JsonResponseParser` containers are `@Nullable Object` | JSON `null`
values |
| `MarkdownDocumentCreator.addValue(@Nullable Object value)` | YAML front
matter such as `key:` with no value |
| `SearchResponse.maxScore`, `QueryStats.maxScore` | null when scores
weren't requested |
| `SortClause.order` | optional; see below |
For `NamedList`, declaring `NamedList<Object>` told IntelliJ that values
can't be null. Removing the check, as the quick-fix suggests, would turn a
missing key into an NPE. In `getFloat`, the next line is
`Float.parseFloat(value.toString())`. In `extractSingleCacheInfo`, a cache
entry is always absent on Solr 10.
**`SortClause.order`:** the `search` tool tells clients `order` is optional
and defaults to `asc`. The generated input schema listed it as required,
though, so the SDK rejected a sort clause without an order before the default
was ever applied. With `@Nullable`, only `field` is required:
```
before: sortClauses.items.required = ["field", "order"]
after: sortClauses.items.required = ["field"]
```
### Tests
- **Removed:** the 10 tests that only passed null to parameters that are now
required.
- **Kept on purpose:** two tests pass null to pin a contract: the SDK
rejecting a null completion value, and Spring's binder passing null when
`solr.url` is absent. They keep the null and suppress `DataFlowIssue` on that
method only.
- **Real nullability:** captured system properties are `@Nullable`, since
`System.getProperty` returns null when unset. Test lists built with a null
value are `NamedList<@Nullable Object>`.
- **No more unboxing nullable counters:** the cache and handler assertions
use AssertJ `isPositive()`, which also fails clearly on null. Float comparisons
use `isCloseTo`.
- **Assertions that couldn't fail now test something:**
- `required() || !required()` now checks that `collection` is required and
the other `search` parameters are optional.
- `size() >= 0` now checks that the `_default` configset has dynamic
fields.
- The span timestamps check now requires every span to have started and to
have ended no earlier than it started. `SimpleSpan` never returns null
timestamps.
- **Lifecycle fields:** `McpClientIntegrationTestBase` uses `@AutoClose`
instead of a manual null-guarded `afterAll`. `DockerImageHttpIntegrationTest`
builds its `HttpClient` inline and reads the base URL from the container.
- **Dead code removed:** unused fields and mocks, unused `throws Exception`
clauses (37, two of them in `HttpSecurityConfiguration`), redundant casts, an
empty `else`, and two Javadoc links to test classes that aren't accessible from
where they're used.
### Not changed
- **`@McpArg(required = false)`:** IntelliJ calls these redundant defaults.
The repo writes `required` explicitly on every parameter, because
`@McpToolParam` and `@McpArg` have opposite defaults.
- **Other warning types are separate changes:**
- deprecated-API migrations: Jackson 3 text accessors, MCP request
constructors, Testcontainers `SolrContainer`
- record-component Javadoc (`DanglingJavadoc`)
- package-private DTOs returned from public methods
- unused public test helpers
- style hints: method references, `getFirst()`, expression lambdas
## Verification
- `./gradlew build` passes, with all 455 tests run, none skipped or failed.
That's #266's 465 minus the 10 removed null tests. NullAway, which fails the
build on `src/main`, accepts every new `@Nullable`.
- IntelliJ's `ConstantValue` inspection run on all 37 changed files reports
nothing. The same run flags the original `CollectionUtils`.
- `DataFlowIssue` and `NullableProblems` reproduce every original warning on
the unedited files. On the changed files, the only hits are calls into
signatures this PR changes, checked against their old declarations.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
