atiaomar1978-hub commented on PR #25430: URL: https://github.com/apache/camel/pull/25430#issuecomment-5247825390
## Bugbot / Grok Review — PR #25430 (CAMEL-24347) **Verdict:** ✅ **Approve** — focused bug fix, CI green, already approved by @davsclaus ### Summary Fixes two real issues in `camel-google-firestore`: 1. **`listCollections` ignored endpoint `documentId`** — the only operation that read the id from the header alone, so `?documentId=alice` silently listed root collections instead of sub-collections under `users/alice`. 2. **`queryCollection` / `listDocuments` mutated SDK maps** — `_id` / `_path` were written into the map returned by `QueryDocumentSnapshot.getData()` instead of a copy handed to the route. ### What looks good | Area | Assessment | |------|------------| | **Correctness** | `determineListedDocumentId()` mirrors `determineDocumentId()` header→option fallback but correctly keeps missing id as “list root collections” (no throw). | | **Consistency** | Same `ObjectHelper.isEmpty()` semantics as other operations (blank header falls back to endpoint option). | | **Side-effect fix** | `withDocumentMetadata()` copies via `new HashMap<>(document.getData())` before adding metadata — good hygiene. | | **Tests** | New `GoogleFirestoreProducerDocumentIdTest` covers all three resolution paths; AssertJ, package-private class/methods, no `Thread.sleep`. | | **Scope** | Small, well-scoped diff; PR description honestly scopes out the unbounded consumer queue (needs maintainer policy). | | **CI** | All checks passing (Java 17/25 builds). | ### Non-blocking follow-ups 1. **`withDocumentMetadata` test** — consider a small unit test with a mocked `QueryDocumentSnapshot` to assert the SDK map is never mutated (regression guard). 2. **Upgrade guide** — optional one-liner under 4.22: `listCollections` now honours endpoint `documentId` (behaviour fix; previously listed root collections when id was only configured on the URI). 3. **Consumer queue growth** — already noted on JIRA; future work should pick an overflow policy (`maxMessagesPerPoll` or bounded queue + drop/block strategy). 4. **`getDocumentById`** returns `document.getData()` without copy — fine today (read-only), but if routes mutate the body map, same class of issue could appear; low priority. ### Bugbot automated scan No additional defects flagged by automated review on this changeset. **Recommendation:** Merge when ready — behaviour fix is correct, tests are adequate for the id-resolution logic, and the map-copy change is a clear improvement. _AI-generated review on behalf of atiaomar1978-hub_ -- 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]
