adityamparikh commented on PR #86: URL: https://github.com/apache/solr-mcp/pull/86#issuecomment-5650648106
Design review of the current head, since the branch has drifted (81 commits behind, two conflicts: the `SolrConfig` import block and `McpToolRegistrationTest`'s `metadata.SchemaService` import, which moved to `schema`). Blocking: - `RefGuideService.java:66` fetches `https://solr.apache.org/guide/sitemap.xml` on every tool call and regex-scans it. That sitemap covers every version, so each query is a multi-megabyte download with no cache and no timeouts on the `RestClient`. Cache the parsed `<loc>` list with a TTL and set connect/read timeouts. - `RefGuideService.java:74` calls `query.toLowerCase()`; the MCP SDK on `main` does not validate required inputs, so an omitted `query` is an NPE. Blank-guard it the way `AliasService` does. - `RefGuideServiceTest` uses `MockitoExtension` with deep stubs and no `@DisabledInNativeImage`, so `./gradlew nativeTest -Pnative` will fail on it. Non-blocking: the tool name `searchRefGuide` is camelCase where every other tool is kebab-case; it has no `readOnlyHint` and no `@Observed`; the pre-9 branch returns a hard-coded archive PDF URL without checking it exists (`5.0` yields a 404); and the bare `RestClient` bean sits in `SolrConfig` rather than its own configuration with timeouts. @jzonthemtn, do you still plan to finish this? If not, I would close the draft and keep #85 open for a cached-sitemap version. -- 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]
