Repository navigation
CAMEL-25361: camel-couchbase - use the view value as the body and document the fullDocument default - #27588
Conversation
…ument the fullDocument default With useView=true and fullDocument=false the consumer set the message body to row.valueAs(Object.class), which the Couchbase SDK 3 returns as an Optional, so the body was Optional[value] (Optional.empty when the view emitted null) instead of the value. The body is now the value, or null when the view emitted none. The SQL++ path is not affected: it uses the query row as a JSON string. The fullDocument option was documented with defaultValue false since it was added (CAMEL-15792), but the field has always defaulted to true, which keeps the behaviour from before the option existed (the consumer always fetched the document). The annotation now says true, and the component JSON, the catalog and the endpoint DSL are regenerated. No change at runtime. Upgrade guide: the body change, and, as asked in the review of apache#27347 (CAMEL-25221), that a view or SQL++ query returning several rows for one document delivers only the first of them with consumerProcessedStrategy=delete. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 9 of 704 tested, 24 compile-only — current: 9 all testedMaveniverse Scalpel detected 9 affected modules (current approach: 9). Skip-tests mode would test 9 modules (4 direct + 8 downstream), skip tests for 24 (generated code, meta-modules) Modules Scalpel would test (9)
Modules with tests skipped (24)
All tested modules (36 modules, 5m 58s total)Total reactor time: 5m 58s
Top 20 slowest modules:
|
davsclaus
left a comment
There was a problem hiding this comment.
Thanks, the fix is right: ViewRow.valueAs(Class) returns Optional<T> in SDK 3, so with useView=true&fullDocument=false the body has been an Optional since CAMEL-15792. The fullDocument annotation also contradicted the field default (true) and the SQL++ docs. Both upgrade-guide entries are accurate: the body change is a real change for routes that unwrapped the Optional, and the multi-row note matches the in-flight skip added in CAMEL-25221. On the in-flight set: agreed, stopRoute/startRoute builds a new consumer, so leaving it as it is makes sense. The tests follow the module's mock-based style. One optional nit inline.
This review was generated by an AI agent (Claude Code on behalf of davsclaus) and may contain inaccuracies. Please verify all suggestions before applying.
…nd the fullDocument default Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
oscerd
left a comment
There was a problem hiding this comment.
Approving. Both defects independently confirmed.
1. The Optional body is real. I decompiled the SDK actually on the classpath (couchbase-client-version = 3.12.3, parent/pom.xml:140):
public <T> java.util.Optional<T> valueAs(java.lang.Class<T>);
So doc = row.valueAs(Object.class) on main (CouchbaseConsumer.java:256) genuinely set the body to an Optional. The .orElse(null) fix is correct — and the surrounding code already used the right idiom two lines later (row.keyAs(String.class).orElse(null)), which makes this an obvious copy-paste miss rather than intent. git log confirms: the call dates from the SDK-3 migration and no commit has touched it since.
2. The defaultValue mismatch is real. CouchbaseEndpoint.java:152-153 had @UriParam(defaultValue = "false") over private boolean fullDocument = true. The component page already contradicted the annotation — couchbase-component.adoc:119 says "When fullDocument is true (the default)". The annotation was the outlier; changing it to true is documentation-only with no runtime effect, and does not relax a default.
Generated files are complete. All three mirrors of the default are regenerated: component JSON, catalog JSON, endpoint DSL Javadoc. I checked the .adoc pair separately — the prose at :119 and the example at :164 already said true, so no .adoc edit (and therefore no catalog-doc mirror) is due. CI's uncommitted-changes check passing agrees.
Test review. CouchbaseConsumerBodyTest is well-constructed: it drives poll() directly with startScheduler=false instead of racing a scheduler, so no Thread.sleep and no Awaitility needed. theFullDocumentIsTheBodyByDefault is a good regression pin — I verified CouchbaseCollectionOperation.getDocument(...) returns collection.get(id, options), i.e. the GetResult itself (CouchbaseCollectionOperation.java:79-85), so assertSame(document, bodies.get(0)) is sound. JUnit style matches the module (7 test files use org.junit.jupiter.api.Assertions, zero AssertJ).
Non-blocking
[Suggestion] The mocked-ViewRow helper stubs valueAs(Object.class) only. If someone later switches the consumer to valueAs(TypeRef), the mock returns null and the test NPEs rather than failing with a clear message. A lenient() default or a comment pinning the overload would make the intent explicit.
[Question] The PR deliberately does not clear the in-flight set in doStart/doStop, contrary to what was said in #27347. The reasoning (route restart creates a fresh consumer via DefaultRoute.initializeServices → endpoint.createConsumer, so the set is already empty) matches the isInFlight Javadoc at CouchbaseConsumer.java:288+, and clearing it would reintroduce the duplicate-delivery window CAMEL-25221 closed. I agree with leaving it — and it's spelled out in the PR description, so this is already handled transparently. Flagging only so @davsclaus can ack it explicitly.
Checklist: tests ✅ · docs ✅ already correct, verified · upgrade guide ✅ ==== useView=true with fullDocument=false correctly nested under === camel-couchbase · generated catalog ✅ all 3 · commit convention ✅ · public API
Reviewed with Claude Code (Claude Opus 5) on behalf of @oscerd. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying. It is a rules-and-conventions review and does not replace CodeRabbit, Sourcery, or SonarCloud.
… overloads in a comment Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks @oscerd. Addressed in a048d0b:
Tests: camel-couchbase Claude Code on behalf of allthingssecurity |
Description
CAMEL-25361
Follow-up to #27347 (CAMEL-25221), with two small defects found while preparing it.
useView=true&fullDocument=falsethe body wasrow.valueAs(Object.class), which the Couchbase SDK 3 returns as anOptional, so routes receivedOptional[value](orOptional.emptywhen the view emittednull). The body is now the value, ornullwhen the view emitted none. The SQL++ path is not affected (it uses the row as a JSON string). The code dates from CAMEL-15792 (3.7), so 4.14.x and 4.18.x have it too; as it changes what routes receive, the upgrade guide mentions it (a new==== useView=true with fullDocument=falsesubsection of the existing=== camel-couchbasesection).fullDocumentdefault. The field defaults totrue, but@UriParamsaiddefaultValue = "false", so the component JSON, catalog and endpoint DSL documented the wrong default.trueis the intended default: it keeps the behaviour from before the option existed (the consumer always fetched the document), and the SQL++ section of the component page already says so. Only the annotation changes; the generated files are regenerated. No runtime change.==== consumerProcessedStrategy=delete: a view or SQL++ query that returns several rows for one document now delivers only the first of these rows withdelete. I checked that against the code: the first row marks the document in progress, the later rows are skipped, and the completed exchange removes the document. In 4.22 the next row of the already removed document failed the whole poll.Not included: the other suggestion from that review, clearing the in-flight set in
doStop/doStart. I said in #27347 that I would add it, butstopRoute/startRoutedoes not reuse the consumer:RouteServicesets the route up again andDefaultRoute.initializeServicescallsendpoint.createConsumer, so the set starts empty after a route restart. The set survives only a stop and start of the same consumer instance (for example a route policy that stops and starts the consumer). There, clearing it would let the next poll read again a document whose exchange is still being processed, so I left it as is; happy to clear it indoStartif you prefer bounding the set over that duplicate.Tests: new
CouchbaseConsumerBodyTest(mocked cluster).theValueEmittedByTheViewIsTheBodyWithoutFullDocumentandtheBodyIsNullWhenTheViewEmitsNoValuefail without the change (two runs;expected: <[{name=Alice}, Bob]> but was: <[Optional[{name=Alice}], Optional[Bob]]>). Two control tests pass with and without it:theQueryRowIsTheBodyWithoutFullDocument(the SQL++ row as a JSON string) andtheFullDocumentIsTheBodyByDefault(without the option the consumer fetches the document, which pins thefullDocument=truedefault). Module: 61 tests, 0 failures (the Docker ITs are skipped).Target
mainbranch)Tracking
Apache Camel coding standards and style
mvn clean install -DskipTestslocally from root folder and I have committed all auto-generated changes.(I built and tested the affected module, including the formatter and import-sort plugins, and regenerated the catalog and endpoint DSL files for camel-couchbase. I did not run the full root build.)
AI-assisted contributions
Co-authored-bytrailers) and the PR description identifies the AI tool used.This PR was prepared with Claude Code (Claude Opus 5.5). The commit carries a
Co-Authored-Bytrailer.Claude Code on behalf of allthingssecurity
🤖 Generated with Claude Code