oscerd commented on PR #29067: URL: https://github.com/apache/camel/pull/29067#issuecomment-6626275792
On the review from davsclaus: Thanks Claus. Saving the raw change was not a good default, and the new commit takes the stricter option you describe: fail with a hint to remove the header. When the consumer's `CamelCouchDbMethod` header comes through with its change, the producer now fails with a `CouchDbException`, whether the method would save (`UPDATE`) or delete (`DELETE`): `The body is a change of the couchdb consumer, not a document to save: get the document first, with the CamelCouchDbMethod header set to GET and its id in the CamelCouchDbId header, set the body to the document to save, or remove the CamelCouchDbMethod header to save the change itself as a new document`. For `DELETE` the message gives only the first two remedies. The message does not hold the body. So a route from a couchdb consumer straight to a couchdb producer writes nothing, to another database or to the same one. Removing the header keeps the behaviour from before this PR: the change itself is saved as a new document with a generated id. A change is a JSON object with the fields of a change and no other: `seq`, `id` and `changes`, and `deleted` for a deleted document. These are the fields of the Cloudant `ChangesResultItem` that the consumer emits (`doc` is not used, as the consumer does not ask for documents). A document, which has an `_id` or any other field, is saved as before. So the useful case you describe, a route that turns the change into a document and leaves the `UPDATE` header, still saves; a test covers it. The upgrade guide entry and the component page now say it plainly. A route from a couchdb consumer straight to a couchdb producer fails, because the consumer's header and its change reach the producer. To copy the document, get it first with `GET` and the `CamelCouchDbId` header that the consumer sets, or set the body to the document. To store the change itself, remove the header. In a route back to the database that the consumer reads, each such save is a new change in turn, so that route writes without end. On the review from Croway: Thanks for running both routes against CouchDB 3.5.3. The copy to another database storing the change row under a random `_id` settled it, and the new commit takes the stricter option you proposed. When the body is still the change of the consumer and its `CamelCouchDbMethod` header came through, the producer now fails, to save or to delete, with a `CouchDbException` that says the body is a change of the couchdb consumer and not a document. The hint lists your two remedies: get the document first (the `CamelCouchDbMethod` header set to `GET`, with the `CamelCouchDbId` header the consumer sets) or set the body to the document. For a save it adds removing the `CamelCouchDbMethod` header, which saves the change itself as a new document with a generated id, as before this PR. The check matches the change exactly: a JSON object with `seq`, `id` and `changes` (and `deleted` for a deleted document) and no other field. So a document that happens to have a `seq` or `changes` field, or any document with an `_id`, is still saved. With this, both routes of your table should fail on each change and write nothing: the target stays empty, and the loop database keeps only the document inserted by the test. The tests run the real consumer into the producer with a mocked client: - An update change and a delete change both fail and write nothing. - A change turned into a document, with the `UPDATE` header left, is saved. - A change body without the method header is saved as a new document, as before. On the review from gnodet-bot: Thanks for the review. - `getBodyAsJsonObject` now declares `throws InvalidPayloadException, CouchDbException`. - `initCause` stays. The `InvalidPayloadException` constructor that takes a cause has no hint, and it appends the message of the cause to its own. The parse error of Gson names the path of the bad field in the JSON, and the message should say only that the body is not valid JSON. The hint constructor does not set a cause, so `initCause` is allowed, and the `JsonSyntaxException` stays in the chain for the logs. - Agreed on the `if`/`else if` chain: the old code checked `DELETE` and `GET` in two separate `if` blocks, and any other method fell through both. _Claude Code on behalf of Andrea Cosentino (oscerd)_ -- 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]
