adityamparikh commented on PR #105: URL: https://github.com/apache/solr-mcp/pull/105#issuecomment-5698716821
Closing as superseded. This PR consolidates four scattered blank-input checks into one `SolrDocumentCreator.requireContent(content, format)` helper. Three things have since overtaken it. **#205 deletes half the call sites.** It removes `CsvDocumentCreator`, `XmlDocumentCreator`, `XmlIndexingTest` and the orchestrator's XML path, because Solr's own `/update` handlers parse CSV and update XML. That is 3 of the 9 files here deleted outright, plus one hunk of a fourth. What remains is JSON and Markdown — two call sites, each a one-line `if (x.isBlank()) throw`. A static helper on the interface, parameterised by a format-name string, is indirection at that size rather than consolidation. **#202 and #207 rewrite the two survivors.** #202 reworks `JsonDocumentCreator` (+66/-28) and #207 reworks `MarkdownDocumentCreator.create` (+165/-50), so both files this PR edits are being rebuilt by branches already in flight. **The signature no longer covers the shape.** #202 adds `JsonDocumentCreator.create(List<Map<String, Object>>)`, whose emptiness check is `documents == null || documents.isEmpty()` — not a `String`, so `requireContent(String, String)` cannot serve it. The one duplication worth fixing is the `"JSON input cannot be empty"` literal appearing in two methods of that class after #202; that belongs in #202 as a named constant, not here. **No behaviour is lost by closing.** Blank input is still rejected on every format: `IndexingDocumentCreator` throws `"Markdown input cannot be null or empty"` on `main` today and #207 keeps that guard, and the JSON/CSV/XML creators keep their own checks. This PR was a refactor, not a fix. Separately, its own rationale — "the creators are `@NullMarked`, so a null argument is a caller's contract violation, not an input to validate" — is being applied repo-wide rather than in this one package, alongside making `@McpToolParam`/`@McpArg` `required` explicit and dropping the null checks that become dead as a result. Landing this now would adopt the narrower earlier framing. Thanks — the blank-input inconsistency it identified was real, and the repo-wide version of the fix carries it forward. -- 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]
