adityamparikh commented on PR #144:
URL: https://github.com/apache/solr-mcp/pull/144#issuecomment-5357777772
Reviewed as part of a sweep over the open PRs. The markdown support is
thoughtfully done — the
input size guard, front-matter extraction with a first-`h1` title fallback,
and especially the
tool description that routes callers *away* to `index-json-documents` /
`-csv` / `-xml` and tells
them only to convert when no dedicated tool exists. Negative routing like
that is exactly what
stops an LLM reaching for the wrong tool.
**Blocking: the branch is currently `CONFLICTING` and can't merge.** It also
overlaps #105, which
adds a shared `SolrDocumentCreator.requireContent(content, format)` helper
for the null/blank
check each creator was doing separately. Worth rebasing *after* #105 lands
and reusing that helper
rather than keeping the local check in
`createSchemalessDocumentsFromMarkdown`, so the error
message stays identical across all four formats.
**Worth a second look: the content-hash id makes edits additive.**
`contentHash` is a SHA-256 of
the entire markdown body, so changing a single character produces a
different `id`. Re-indexing an
edited document therefore creates a *second* document rather than updating
the first, and the
stale copy stays in the index. For a docs collection re-indexed on every
commit, that grows
without bound and silently returns outdated content alongside current
content.
The tool description does say to supply a stable front-matter `id`, but only
in the
format-conversion case. Since the hash is what makes re-indexing idempotent
*only for byte-identical
input*, I'd widen that guidance to say a front-matter `id` is required for
any document that will
be re-indexed, and state plainly that without one, edits accumulate rather
than replace. The
current wording reads as an optimisation rather than a correctness
requirement.
Two minor things:
- `create` measures the input with `markdown.getBytes(UTF_8).length`, which
allocates a full byte
copy of a potentially 10 MB string just to size it. A `markdown.length() >
MAX_INPUT_SIZE_BYTES`
pre-check is free — UTF-8 bytes can never be fewer than chars — and only
needs the exact
computation when that passes.
- `create` returns `List.of()` for blank input, but
`createSchemalessDocumentsFromMarkdown` throws
on blank first, so that branch is unreachable through the tool. Either
drop it or add a
direct-caller test so it isn't dead code.
The id semantics are the only thing I'd call out as needing a decision
rather than a tweak.
--
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]
