gnodet-bot commented on code in PR #26619: URL: https://github.com/apache/camel/pull/26619#discussion_r4056383667
########## dsl/camel-yaml-dsl/camel-yaml-dsl-common/src/main/java/org/apache/camel/dsl/yaml/common/DataFormatKeyHints.java: ########## @@ -0,0 +1,145 @@ +/* + * 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.dsl.yaml.common; + +import java.util.Collection; +import java.util.List; +import java.util.Locale; +import java.util.Map; + +/** + * The hint for a {@code marshal}/{@code unmarshal} key that names a data format the way its artifact or the catalog + * does (jackson, json-jackson, jackson-xml, snake-yaml) instead of by its YAML key (json with library Jackson, + * jacksonXml, yaml). + * <p> + * The YAML key is the data format's model name. Ten data formats in the catalog have a name of their own that differs + * from it, because one model serves several libraries or types: {@link #ALIASES} lists them with the option that + * selects the library or type. The table mirrors the catalog ({@code name} and {@code modelName} of each data format, + * and the enum of the {@code library} or {@code type} option of the model); the validator's tests check the two agree. + * The deserializer cannot read the catalog at runtime, which is why the table is here rather than derived. + */ +public final class DataFormatKeyHints { + + /** The YAML key of an aliased data format and the option that selects it: json with library Jackson. */ + public record Alias(String key, String option, String value) { + + /** The key with the option, as it is written in YAML: json: {library: Jackson}, or yaml: {...}. */ + public String form() { + return option != null ? key + ": {" + option + ": " + value + "}" : key + ": {...}"; + } + } + + /** + * The catalog data formats whose name is not their YAML key, by name in lower case with no separators, so that + * jackson, Jackson, snake-yaml and snakeYaml all match. + */ + public static final Map<String, Alias> ALIASES = Map.ofEntries( + Map.entry("jackson", new Alias("json", "library", "Jackson")), + Map.entry("gson", new Alias("json", "library", "Gson")), + Map.entry("jsonb", new Alias("json", "library", "Jsonb")), + Map.entry("fastjson", new Alias("json", "library", "Fastjson")), + Map.entry("avrojackson", new Alias("avro", "library", "Jackson")), + Map.entry("protobufjackson", new Alias("protobuf", "library", "Jackson")), + Map.entry("snakeyaml", new Alias("yaml", null, null)), + Map.entry("bindycsv", new Alias("bindy", "type", "Csv")), + Map.entry("bindyfixed", new Alias("bindy", "type", "Fixed")), + Map.entry("bindykvp", new Alias("bindy", "type", "KeyValue"))); + + /** The models an alias belongs to, which people prefix or suffix the library with: json-jackson, jackson-json. */ + private static final List<String> MODELS = List.of("json", "avro", "protobuf", "yaml", "bindy"); + + private DataFormatKeyHints() { + } + + /** + * The hint for a key that is not a data format key, or null when the key is neither a spelling of one of the known + * keys nor an alias. + * + * @param key the key as written: jackson, json-jackson, jackson-xml, JSON + * @param knownKeys the data format keys of marshal/unmarshal: json, jacksonXml, yaml... + * @return the hint: did you mean 'jacksonXml'? for a spelling of a key, the data format is json, Jackson + * is its library: write json: {library: Jackson} for an alias, or null + */ + public static String hint(String key, Collection<String> knownKeys) { + String normalized = normalize(key); + // jackson-xml, JSON, base-64: the key itself, spelled differently + for (String known : knownKeys) { + if (!known.equals(key) && normalized.equals(normalize(known))) { + return "did you mean '" + known + "'?"; + } + } + Alias alias = alias(normalized); Review Comment: **[low] `hint()` passes a pre-normalized string to `alias()`, which normalizes again** `alias()` is a public method whose Javadoc says it takes any "spelling of a data format name" and normalizes it internally. `hint()` passes `normalized` (already lowercased, separators stripped), so `alias()` double-normalizes — harmless because `normalize` is idempotent, but the internal contract is misleading. Either pass the raw `key` here and let `alias()` own normalization: ```suggestion Alias alias = alias(key); ``` or clarify in `alias()`'s Javadoc that it accepts both raw and pre-normalized input. The current state creates a false impression that callers must pre-normalize before calling `alias()`. ########## dsl/camel-yaml-dsl/camel-yaml-dsl-validator/src/test/java/org/apache/camel/dsl/yaml/validator/DataFormatKeyHintsCatalogTest.java: ########## @@ -0,0 +1,85 @@ +/* + * 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.dsl.yaml.validator; + +import java.util.ArrayList; +import java.util.List; + +import org.apache.camel.catalog.CamelCatalog; +import org.apache.camel.catalog.DefaultCamelCatalog; +import org.apache.camel.dsl.yaml.common.DataFormatKeyHints; +import org.apache.camel.tooling.model.DataFormatModel; +import org.apache.camel.tooling.model.EipModel; +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * CAMEL-24847: the alias table the runtime deserializer uses without a catalog must agree with the catalog: every data + * format whose name differs from its model name is in it, with the model name as the key and a value of the option that + * selects the library or type. + */ +public class DataFormatKeyHintsCatalogTest { + + private final CamelCatalog catalog = new DefaultCamelCatalog(); + + @Test + public void testEveryAliasedDataFormatOfTheCatalogIsInTheTable() { + List<String> aliased = new ArrayList<>(); + for (String name : catalog.findDataFormatNames()) { + DataFormatModel df = catalog.dataFormatModel(name); + if (df.getModelName().equals(name)) { + assertThat(DataFormatKeyHints.alias(name)).as(name + " is its own key").isNull(); + continue; + } + aliased.add(name); + DataFormatKeyHints.Alias alias = DataFormatKeyHints.alias(name); + assertThat(alias).as(name + " maps to " + df.getModelName()).isNotNull(); + assertThat(alias.key()).as(name).isEqualTo(df.getModelName()); + EipModel model = catalog.eipModel(df.getModelName()); + EipModel.EipOptionModel selector = model.getOptions().stream() + .filter(o -> o.getEnums() != null && o.getEnums().size() > 1 + && (o.getName().equals("library") || o.getName().equals("type"))) + .findFirst().orElse(null); + if (selector == null) { + assertThat(alias.option()).as(name + " has no library or type to select").isNull(); + } else { + assertThat(alias.option()).as(name).isEqualTo(selector.getName()); + assertThat(selector.getEnums()).as(name + " " + selector.getName()).contains(alias.value()); + } + } + assertThat(DataFormatKeyHints.ALIASES).as("aliases without a catalog data format: " + aliased) + .hasSize(aliased.size()); Review Comment: **[low] Assertion catches missing ALIASES entries but not stale ones** `ALIASES.hasSize(aliased.size())` verifies the counts match, but doesn't prove every key in `ALIASES` corresponds to a real catalog data format. A typo in an ALIASES key (e.g. `"avrojackon"` instead of `"avrojackson"`) would keep the sizes equal while silently registering a dead entry. Consider also asserting that every normalized ALIASES key is found as a catalog data format name: ```java Set<String> normalizedAliased = aliased.stream() .map(DataFormatKeyHints::normalize) .collect(java.util.stream.Collectors.toSet()); assertThat(DataFormatKeyHints.ALIASES.keySet()) .as("every ALIASES key must be a normalized data format name") .isSubsetOf(normalizedAliased); ``` This catches stale entries in addition to missing ones. ########## dsl/camel-yaml-dsl/camel-yaml-dsl/src/test/groovy/org/apache/camel/dsl/yaml/UnmarshalTest.groovy: ########## @@ -128,4 +129,31 @@ class UnmarshalTest extends YamlTestSupport { 'true', 'false', null ] } + + // CAMEL-24847: a data format named as its artifact or catalog entry says which key and option to write + def "unmarshal with #key fails with a message naming the data format key"(String key, String hint) { + when: + loadRoutes([ResourceHelper.fromString("route-1.yaml", """ + - from: + uri: timer:tick + steps: + - unmarshal: + ${key}: {} + """.stripIndent())], false) + then: + def e = thrown(Exception) + def messages = [] + for (Throwable t = e; t != null; t = t.cause) { + messages << t.message + } + messages.any { it != null && it.contains("Error constructing YAML node id: unmarshal: unsupported field: ${key}") && it.contains(hint) } + where: + key | hint + 'jackson' | 'the data format is json, Jackson is its library: write json: {library: Jackson}' + 'json-jackson' | 'write json: {library: Jackson}' + 'gson' | 'write json: {library: Gson}' + 'bindy-csv' | 'the data format is bindy, Csv is its type: write bindy: {type: Csv}' + 'snake-yaml' | 'the data format is yaml: write yaml: {...}' + 'JSON' | "did you mean 'json'?" Review Comment: **[low] No `marshal:` integration test row** `YamlDeserializationContext` now handles both `"marshal"` and `"unmarshal"` symmetrically, but the Groovy `where:` table only covers `unmarshal:`. Adding one `marshal:` row would give the runtime path a complete end-to-end smoke test: ```suggestion 'JSON' | "did you mean 'json'?" 'jackson' | 'the data format is json, Jackson is its library: write json: {library: Jackson}' ``` (The second row uses `marshal` — you'd need to parametrize the EIP name too, or add a separate `then:` block for `marshal`.) -- 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]
