Croway commented on code in PR #29080:
URL: https://github.com/apache/camel/pull/29080#discussion_r4453750413
##########
components/camel-state-store/camel-state-store/src/main/java/org/apache/camel/component/statestore/StateStoreProducer.java:
##########
@@ -127,6 +132,22 @@ private long determineTtl(Exchange exchange) {
return endpoint.getTtl();
}
+ /**
+ * The value to store: the body as it is, except a stream, which can be
read only once and would be shared by every
+ * exchange that gets the key. A stream is read and stored as its bytes,
which keep binary content as it is; a
+ * character stream (also a cached one) as its text in the charset of the
exchange.
+ */
+ private static Object valueToStore(Exchange exchange, Message message)
throws InvalidPayloadException {
+ Object body = message.getBody();
+ if (body instanceof Reader) {
+ return
message.getMandatoryBody(String.class).getBytes(ExchangeHelper.getCharset(exchange));
+ }
+ if (body instanceof StreamCache || body instanceof InputStream) {
+ return message.getMandatoryBody(byte[].class);
Review Comment:
Two small things here:
- The body is converted, but the message keeps the stream, which is now
consumed. On a `noStreamCaching()` route with a remote backend,
`backend.get(key)` or `backend.put(...)` may throw a transient error. If the
error handler then redelivers, the retry reads the consumed `InputStream` and
stores `byte[0]` with no error. In a test with a backend whose first `get`
throws and `maximumRedeliveries(1)`, the stored value is `byte[0]` and the
exchange completes without an exception. Calling `message.setBody(bytes)`
before the backend calls makes a retry store the same bytes.
- A `StreamCache` is not reset before it is converted. The `StreamCache` to
`byte[]` converter uses `writeTo`, and `InputStreamCache.writeTo` writes `buf,
pos, count - pos`, so a cache that an earlier processor read part of is stored
cut short. In a test, an `InputStreamCache` of 7 bytes with 2 already read is
stored as its last 5 bytes. With stream caching on, the channel resets the
cache before the producer, so all 7 bytes are stored there, but on a
`noStreamCaching()` route nothing resets it. Calling `cache.reset()` first
covers that case.
The `direct:putNoStreamCaching` route in the test could cover both cases.
##########
components/camel-state-store/camel-state-store/src/main/java/org/apache/camel/component/statestore/StateStoreProducer.java:
##########
@@ -127,6 +132,22 @@ private long determineTtl(Exchange exchange) {
return endpoint.getTtl();
}
+ /**
+ * The value to store: the body as it is, except a stream, which can be
read only once and would be shared by every
+ * exchange that gets the key. A stream is read and stored as its bytes,
which keep binary content as it is; a
+ * character stream (also a cached one) as its text in the charset of the
exchange.
+ */
+ private static Object valueToStore(Exchange exchange, Message message)
throws InvalidPayloadException {
+ Object body = message.getBody();
+ if (body instanceof Reader) {
+ return
message.getMandatoryBody(String.class).getBytes(ExchangeHelper.getCharset(exchange));
Review Comment:
The text is encoded in the charset of the exchange that does the `put`, but
the charset is not stored with the bytes, so a `get` on another exchange cannot
know it. Two consequences:
- platform-http sets the `CamelCharsetName` exchange property from the
charset of the request `Content-Type`, for example `ISO-8859-1`. A later get
route that runs `convertBodyTo(String.class)` decodes as UTF-8, so non-ASCII
text comes back as replacement characters. In a test, `"café"` put as a
`StringReader` with `CamelCharsetName=ISO-8859-1` comes back from `get` plus
`convertBodyTo(String.class)` as `"caf�"`.
- `ExchangeHelper.getCharset` calls `Charset.forName`. If `CamelCharsetName`
holds a name it rejects, the `put` of a `Reader` now throws, where before it
stored the value. For example, with `CamelCharsetName=utf8mb4` it throws
`UnsupportedCharsetException: utf8mb4`. On main the same `put` succeeds, and a
`String` body is still stored on the PR head.
A `Reader` already holds decoded text, so always encoding it in UTF-8 would
avoid both problems. If keeping the charset of the exchange is intended, the
doc and the upgrade guide should say that the charset is not stored with the
bytes.
##########
docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_24.adoc:
##########
@@ -4742,6 +4742,15 @@ jsonata read the text as one JSON string, refused bytes
and failed on a stream.
the exchange when one is set; otherwise the JSON parser detects the encoding.
Such text that is not valid JSON now fails
with the parse error. Other text, a `Map`, a `List` and other objects are used
as before.
+=== camel-state-store - a stream body is stored as its bytes
+
+The `put` and `putIfAbsent` operations of the `state-store` component now read
a stream body (an `InputStream`, a
+`Reader` or the stream cache of Camel) and store its bytes, so a later `get`
gives a `byte[]`. A `Reader` is stored as
+its text in the charset of the exchange, UTF-8 by default. Before, the stream
object itself was stored: the first `get`
+could read it, later ones got an empty stream, and every exchange that got the
key shared the same stream object. A
+backend that serializes its values could not store such a stream at all.
Convert the `byte[]` to the type you need,
+for example with `convertBodyTo(String.class)` for text. Other bodies are
still stored as they are.
Review Comment:
`convertBodyTo(String.class)` decodes with the charset of the `get`
exchange, UTF-8 by default, not the charset the bytes were stored in. It is
worth saying that the charset is not stored, and to use
`convertBodyTo(String.class, "<charset>")` when the bytes are not UTF-8.
##########
docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_24.adoc:
##########
@@ -4742,6 +4742,15 @@ jsonata read the text as one JSON string, refused bytes
and failed on a stream.
the exchange when one is set; otherwise the JSON parser detects the encoding.
Such text that is not valid JSON now fails
with the parse error. Other text, a `Map`, a `List` and other objects are used
as before.
+=== camel-state-store - a stream body is stored as its bytes
Review Comment:
Nit: this section sits between the two camel-jsonata sections ("camel-jolt,
camel-jsonata - JSON text...", line 4736, and "camel-jsonata - the endpoint
options of an http resource", line 4754), so it splits them. Could it move
after the second one?
--
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]
