mattcasters commented on code in PR #9078:
URL: https://github.com/apache/hop/pull/9078#discussion_r4341272130
##########
plugins/transforms/metainject/src/main/java/org/apache/hop/pipeline/transforms/metainject/MetaInjectMeta.java:
##########
@@ -105,10 +107,11 @@ public class MetaInjectMeta extends
BaseTransformMeta<MetaInject, MetaInjectData
private boolean noExecution;
@HopMetadataProperty(
- key = "allow_empty_stream_on_execution",
- injectionKey = "ALLOW_EMPTY_STREAM_ON_EXECUTION",
- injectionKeyDescription =
"MetaInject.Injection.ALLOW_EMPTY_STREAM_ON_EXECUTION")
- private boolean allowEmptyStreamOnExecution;
+ key = "condition_stream_behavior",
+ injectionKey = "CONDITION_STREAM_BEHAVIOR",
+ injectionKeyDescription =
"MetaInject.Injection.CONDITION_STREAM_BEHAVIOR",
Review Comment:
**[bug]** `injectionKeyDescription` refers to
`MetaInject.Injection.CONDITION_STREAM_BEHAVIOR`, which is not defined in
`messages_en_US.properties`. `messages_pt_BR.properties` still only has the old
`ALLOW_EMPTY_STREAM_ON_EXECUTION` string. `BeanInjectionInfo.getDescription`
leaves a key with no spaces as
`!MetaInject.Injection.CONDITION_STREAM_BEHAVIOR!`, so the Inject Metadata tree
shows that marker instead of a label.
**Suggestion:** Add `MetaInject.Injection.CONDITION_STREAM_BEHAVIOR` next to
the other `MetaInject.Injection.*` entries, and translate it in the locales
that carried the old string.
##########
plugins/transforms/metainject/src/main/java/org/apache/hop/pipeline/transforms/metainject/MetaInjectMeta.java:
##########
@@ -105,10 +107,11 @@ public class MetaInjectMeta extends
BaseTransformMeta<MetaInject, MetaInjectData
private boolean noExecution;
@HopMetadataProperty(
- key = "allow_empty_stream_on_execution",
- injectionKey = "ALLOW_EMPTY_STREAM_ON_EXECUTION",
- injectionKeyDescription =
"MetaInject.Injection.ALLOW_EMPTY_STREAM_ON_EXECUTION")
- private boolean allowEmptyStreamOnExecution;
+ key = "condition_stream_behavior",
+ injectionKey = "CONDITION_STREAM_BEHAVIOR",
Review Comment:
**[bug]** Replacing `ALLOW_EMPTY_STREAM_ON_EXECUTION` drops the injection
key stored in existing metadata-injection mappings. `MetaInject.newInjection`
only logs `TargetKeyIsNotDefined` and keeps the template value, so a parent
that injects `Y`/`true` no longer selects "at least one non-empty". Those
`Y`/`N` values would also be rejected by `convertCodeToEnumeration` if aimed at
the new enum. The 0045 integration test was only half updated:
`golden-meta-inject.csv`, the dataset, and `0045-meta-inject-valiate UNIT.json`
expect `condition_stream_behavior`, but
`integration-tests/mdi/0045-meta-inject.hpl` still injects
`ALLOW_EMPTY_STREAM_ON_EXECUTION` and `0045-meta-inject-valiate.hpl` still
reads xpath `allow_empty_stream_on_execution`, so the check will not produce
the renamed field.
**Suggestion:** Keep accepting the old injection key and map `Y`/`true` to
`AT_LEAST_ONE_NON_EMPTY` and `N`/`false` to `ALL_NON_EMPTY`, the same way the
deserialization callback does. Point the 0045 mapping and Get XML Data xpath at
`condition_stream_behavior` and the new codes.
##########
plugins/transforms/metainject/src/main/java/org/apache/hop/pipeline/transforms/metainject/MetaInjectMeta.java:
##########
@@ -169,6 +172,26 @@ public void convertLegacyXml(Node node) throws
HopException {
}
}
+ /**
+ * Reads the legacy {@code allow_empty_stream_on_execution} boolean property.
+ *
+ * <p>Before the {@link ConditionStreamBehavior} enum existed, this
transform stored a plain
+ * boolean under the tag {@code allow_empty_stream_on_execution} (as {@code
Y}/{@code N} in XML
+ * and {@code true}/{@code false} in JSON metadata). That tag no longer maps
to an annotated
+ * field, so this callback is fired by the serializer whenever it is still
found, translating the
+ * old value into the equivalent {@link ConditionStreamBehavior}.
+ */
+ @OnUnknownDeserializationProperty
+ public void onUnknownDeserializationProperty(UnknownProperty property) {
+ if ("allow_empty_stream_on_execution".equals(property.getName())) {
+ // "Y" / "true" => the old checkbox was enabled => at least one
non-empty stream was required.
+ conditionStreamBehavior =
Review Comment:
**[suggestion]** Annotated fields are applied before unknown-property
callbacks, so when both tags are present XML always lets
`allow_empty_stream_on_execution` overwrite `condition_stream_behavior`,
including an explicit `always`. In JSON the winner depends on key order. New
saves omit the old tag, but a file that contains both cannot represent `ALWAYS`.
**Suggestion:** Apply the legacy boolean only when
`condition_stream_behavior` is absent. `convertLegacyXml` can see both XML
tags; the JSON path needs the same guard so a legacy key that appears after the
new one does not replace it.
##########
core/src/main/java/org/apache/hop/metadata/serializer/json/JsonMetadataParser.java:
##########
@@ -101,6 +105,23 @@ private void loadProperties(Object object,
com.fasterxml.jackson.core.JsonParser
// This is a recognized piece of data. We can load this...
//
loadProperty(object, jsonParser, key, field);
+ } else if (!unknownPropertyCallbacks.isEmpty()) {
+ // A key that does not map to any annotated field. Read its raw
value and fire the
+ // callbacks so they can translate a legacy/renamed property into a
current field.
+ //
+ jsonParser.nextToken();
+ String rawValue = "null".equals(jsonParser.getText()) ? null :
jsonParser.getText();
Review Comment:
**[bug]** An unknown value is consumed with one `nextToken()` and
`getText()`. That is enough for a scalar, but an object or array leaves the
parser inside the value. The nested `END_OBJECT` or `END_ARRAY` then ends the
`loadProperties` loop, so every property after that unknown structure is
dropped. Any metadata type that opts into `@OnUnknownDeserializationProperty`
can silently lose later fields. `"null".equals(getText())` also collapses the
JSON string `"null"` to Java null, because a JSON null and that string share
the same text.
**Suggestion:** Advance to the value token, call `skipChildren()` for
`START_OBJECT` and `START_ARRAY`, and treat only `VALUE_NULL` as null. Pass
other scalar text through unchanged.
--
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]
