gnodet-bot commented on code in PR #27231:
URL: https://github.com/apache/camel/pull/27231#discussion_r4158435444
##########
dsl/camel-yaml-dsl/camel-yaml-dsl-validator/src/main/java/org/apache/camel/dsl/yaml/validator/YamlValidator.java:
##########
@@ -1128,19 +1235,81 @@ private Map<String, OneOfGroup> loadOneOfGroups() {
* {@code BeanConstructorDefinition.index}, which is a map key and is not
reachable from the schema - see
* CAMEL-24696 before exposing it.
*/
+ /**
Review Comment:
💡 **Orphaned Javadoc.** The large Javadoc block ending on the line above
(originally documenting `isRuntimeAcceptedScalar`) is now separated from its
method by `removeSatisfiedAlternatives`, `requiredPropertyOf`, and
`constructOf`. In Java the second `/**` takes precedence, so the first block
becomes dead documentation. Consider moving the original Javadoc down to sit
directly above `isRuntimeAcceptedScalar` (line 1295).
##########
dsl/camel-yaml-dsl/camel-yaml-dsl-validator/src/main/java/org/apache/camel/dsl/yaml/validator/YamlValidator.java:
##########
@@ -1128,19 +1235,81 @@ private Map<String, OneOfGroup> loadOneOfGroups() {
* {@code BeanConstructorDefinition.index}, which is a map key and is not
reachable from the schema - see
* CAMEL-24696 before exposing it.
*/
+ /**
+ * Drops "required property X not found" where one of the alternatives is
in fact there.
+ * <p/>
+ * The branches of a {@code oneOf} each require their own key, and when
all of them are reported the file is told it
+ * needs a key it does not: {@code unmarshal: {fhirXml: ...}} in one when
branch of a choice was told "required
+ * property 'fhirJson' not found", because the winner of the other when
branch was chosen for it as well. Collected
+ * per construct and per place, the required names of the branches are the
alternatives; if the object at that place
+ * has any of them, one branch is satisfied and the rest are noise. When
none of them is there, the file really does
+ * have to pick one and the errors stay (CAMEL-25238).
+ */
+ private static void removeSatisfiedAlternatives(List<Error> errors) {
+ Map<String, List<Error>> groups = new LinkedHashMap<>();
+ for (Error e : errors) {
+ if (!"required".equals(e.getKeyword())) {
+ continue;
+ }
+ String construct =
constructOf(String.valueOf(e.getEvaluationPath()));
+ if (construct != null) {
+ groups.computeIfAbsent(construct + "@" +
e.getInstanceLocation(), k -> new ArrayList<>()).add(e);
+ }
+ }
+ for (List<Error> group : groups.values()) {
+ JsonNode instance = group.get(0).getInstanceNode();
+ if (instance == null || !instance.isObject()) {
+ continue;
+ }
+ // the branches of a pick-one construct each require their own
key, so one error per key the file did not
+ // write. The key it did write is the one missing from them: that
branch got past its required check and
+ // failed, if at all, further in. So a non-empty object means a
branch was chosen and the rest are noise.
+ // An object with nothing in it, or with a key no branch knows,
still gets the construct's own error and
+ // the additionalProperties error, which is what actually tells
the author what to do.
+ if (!instance.isEmpty() && group.size() > 1) {
+ errors.removeAll(group);
+ }
+ }
+ }
+
+ private static final Pattern REQUIRED_PROPERTY = Pattern.compile("required
property '([^']+)' not found");
+
+ /** The property name a "required property 'X' not found" error names, or
null. */
+ private static String requiredPropertyOf(Error error) {
+ Matcher m =
REQUIRED_PROPERTY.matcher(String.valueOf(error.getMessage()));
+ return m.find() ? m.group(1) : null;
+ }
Review Comment:
💡 **Dead code.** `REQUIRED_PROPERTY` and `requiredPropertyOf` are declared
but never called. If they're scaffolding for the per-place-winner improvement
mentioned in the PR description, a `// TODO CAMEL-25238` would make that intent
clear. Otherwise they can be removed.
--
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]
