davsclaus commented on code in PR #23922:
URL: https://github.com/apache/camel/pull/23922#discussion_r3390973726
##########
components/camel-bindy/src/main/java/org/apache/camel/dataformat/bindy/BindyKeyValuePairFactory.java:
##########
@@ -573,18 +581,26 @@ public String unbind(CamelContext camelContext,
Map<String, Object> model) throw
return builder.toString();
}
- private Object formatField(Format<?> format, String value, int tag, int
line) throws Exception {
+ private Object formatField(
+ Format<?> format, String value, int tag, int line, boolean
fieldOpinion, Class<?> fieldType, String defaultValue)
+ throws Exception {
Object obj = null;
if (value != null) {
// Format field value
try {
- obj = format.parse(value);
+ obj = parseField(format, value, fieldOpinion, fieldType,
defaultValue);
} catch (Exception e) {
- throw new IllegalArgumentException(
- "Parsing error detected for field defined at the tag:
" + tag + ", line: " + line, e);
+ if (!fieldOpinion) {
+ throw new IllegalArgumentException(
+ "Parsing error detected for field defined at the
tag: " + tag + ", line: " + line, e);
+ }
+ if (!defaultValue.isEmpty()) {
+ return format.parse(defaultValue);
+ }
Review Comment:
**Duplicated fallback logic** — `parseField()` in `BindyAbstractFactory`
already handles the entire tolerant recovery path internally (try
`defaultValue`, then fall back to primitive default). This outer catch block
duplicates that same logic:
- When `fieldOpinion=true` and the value fails to parse, `parseField`
handles the fallback internally and **never throws** — so lines 596–602 are
dead code in the normal tolerant case.
- When `fieldOpinion=true` and the `defaultValue` itself is malformed,
`parseField` throws after trying `format.parse(defaultValue)`. The outer catch
catches it and tries `format.parse(defaultValue)` a **second time** (which
fails again). The exception eventually propagates, so the behavior is correct
by accident, but the double-parse of a known-bad value is wasteful and
confusing.
The CSV and Fixed-length factories don't have this issue — they call
`parseField` directly and only wrap the rethrown exception. This method should
follow the same pattern:
```suggestion
obj = parseField(format, value, fieldOpinion, fieldType,
defaultValue);
} catch (Exception e) {
throw new IllegalArgumentException(
"Parsing error detected for field defined at the
tag: " + tag + ", line: " + line, e);
```
With this simplification, when `fieldOpinion=true`, `parseField` handles the
recovery and the catch is never reached. When `fieldOpinion=false`,
`parseField` rethrows and the catch wraps it — exactly like CSV and
Fixed-length.
##########
components/camel-bindy/src/main/java/org/apache/camel/dataformat/bindy/BindyKeyValuePairFactory.java:
##########
@@ -573,18 +581,26 @@ public String unbind(CamelContext camelContext,
Map<String, Object> model) throw
return builder.toString();
}
- private Object formatField(Format<?> format, String value, int tag, int
line) throws Exception {
+ private Object formatField(
Review Comment:
Minor: the parameter name `fieldOpinion` is less clear than
`continueOnFailure` used in `parseField`. Using the same name across both
methods would improve readability.
##########
components/camel-bindy/src/main/java/org/apache/camel/dataformat/bindy/BindyAbstractFactory.java:
##########
@@ -71,7 +73,6 @@ protected BindyAbstractFactory(Class<?> type) throws
Exception {
public void initModel() throws Exception {
models = new HashSet<>();
modelClassNames = new HashSet<>();
Review Comment:
Nit: unrelated blank-line removal from `initModel()`. Trivial, but ideally
PRs avoid cosmetic changes outside their scope.
##########
components/camel-bindy/src/main/java/org/apache/camel/dataformat/bindy/annotation/ResumeUnmarshalingState.java:
##########
@@ -0,0 +1,24 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.dataformat.bindy.annotation;
+
Review Comment:
Minor naming observation: the enum name is verbose and slightly misleading —
it's a per-field override policy, not really a "state." The values
`TRUE`/`FALSE`/`INHERIT` are generic. Something like `ContinueOnFailure` with
the same values, or `ParseFailurePolicy` with `TOLERANT`/`STRICT`/`INHERIT`,
would be more self-documenting. Not blocking — just a suggestion for
consideration.
--
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]
