mattcasters commented on code in PR #9078: URL: https://github.com/apache/hop/pull/9078#discussion_r4366756757
########## plugins/transforms/metainject/src/main/java/org/apache/hop/pipeline/transforms/metainject/ConditionStreamBehavior.java: ########## @@ -0,0 +1,134 @@ +/* + * 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.hop.pipeline.transforms.metainject; + +import org.apache.hop.core.Const; +import org.apache.hop.i18n.BaseMessages; +import org.apache.hop.metadata.api.IEnumHasCode; +import org.apache.hop.metadata.api.IEnumHasCodeAndDescription; + +/** + * Controls under which stream conditions the injected template pipeline is executed. + * + * <p>This replaces the legacy boolean {@code allowEmptyStreamOnExecution}: {@link #ALL_NON_EMPTY} + * and {@link #AT_LEAST_ONE_NON_EMPTY} preserve the old {@code false}/{@code true} semantics, while + * {@link #ALWAYS} adds the ability to run even when every input stream is empty (for templates that + * can run from constant mappings alone). + */ +public enum ConditionStreamBehavior implements IEnumHasCodeAndDescription { + /** Every input stream must produce at least one row. Equivalent to legacy {@code false}. */ + ALL_NON_EMPTY("all-non-empty", "ConditionStreamBehavior.AllNonEmpty.Description"), + + /** Run when at least one stream is non-empty even if others are empty. Legacy {@code true}. */ + AT_LEAST_ONE_NON_EMPTY( + "at-least-one-non-empty", "ConditionStreamBehavior.AtLeastOneNonEmpty.Description"), + + /** Run regardless of the stream contents, even when every input stream is empty. */ + ALWAYS("always", "ConditionStreamBehavior.Always.Description"); + + private static final Class<?> PKG = ConditionStreamBehavior.class; + + private final String code; + private final String descriptionKey; + + ConditionStreamBehavior(String code, String descriptionKey) { + this.code = code; + this.descriptionKey = descriptionKey; + } + + @Override + public String getCode() { + return code; + } + + @Override + public String getDescription() { + return BaseMessages.getString(PKG, descriptionKey); + } + + /** + * @return {@code true} when every stream is required to be non-empty ({@link #ALL_NON_EMPTY}). + */ + public boolean requiresAllNonEmpty() { + return this == ALL_NON_EMPTY; + } + + /** + * @return {@code true} when execution is allowed even if every stream is empty ({@link #ALWAYS}). + */ + public boolean allowsAllEmpty() { + return this == ALWAYS; + } + + /** + * Maps the legacy {@code allow_empty_stream_on_execution} boolean value onto this enum. Before + * this enum existed the transform stored a plain boolean (as {@code Y}/{@code N} in XML and + * {@code true}/{@code false} in JSON metadata / injection values). + * + * <p>A truthy value ({@code Y}/{@code yes}/{@code true}/{@code t}, per {@link Const#toBoolean}) + * meant "at least one non-empty stream was required" and maps to {@link #AT_LEAST_ONE_NON_EMPTY}; + * anything else maps to {@link #ALL_NON_EMPTY}. + * + * @param legacyValue the raw legacy boolean value, or {@code null} + * @return the equivalent enum constant, never {@code null} + */ + public static ConditionStreamBehavior fromLegacyBoolean(String legacyValue) { + return Const.toBoolean(legacyValue) + ? ConditionStreamBehavior.AT_LEAST_ONE_NON_EMPTY + : ConditionStreamBehavior.ALL_NON_EMPTY; + } + + /** + * Returns {@code true} when the given value is a legacy boolean token for the {@code + * allow_empty_stream_on_execution} setting: {@code Y}/{@code N}/{@code yes}/{@code no}/{@code + * true}/{@code false}/{@code t}/{@code f} (case-insensitive). This deliberately excludes the + * current enum codes ({@code all-non-empty}, {@code at-least-one-non-empty}, {@code always}) so + * they are never mistaken for a legacy value. + * + * @param value the value to test, or {@code null} + * @return {@code true} when the value is a legacy boolean token + */ + public static boolean isLegacyBoolean(String value) { + if (value == null) { + return false; + } + String lower = value.toLowerCase(); Review Comment: **[bug]** `isLegacyBoolean` lowercases with `String.toLowerCase()` (default locale) and then compares to English tokens. In a Turkish (or similar) default locale, `"TRUE".toLowerCase()` is `"truıe"` (dotless ı), so the method returns false. `resolveInjectionValue` then passes `"TRUE"` through, and `HopMetadataInjector.convertCodeToEnumeration` rejects it. `Const.toBoolean` is locale-safe via `equalsIgnoreCase`, and the javadoc says these tokens are case-insensitive, so a legacy `TRUE` value fails injection only because of the JVM locale. `"Y"` / `"N"` and already-lowercased `"true"` / `"false"` are unaffected. **Suggestion:** Compare with `equalsIgnoreCase`, or lowercase with `Locale.ROOT`, so the gate matches `Const.toBoolean`. ########## plugins/transforms/metainject/src/main/java/org/apache/hop/pipeline/transforms/metainject/MetaInjectDialog.java: ########## @@ -1092,11 +1104,15 @@ private void refreshTree() { */ private static MetaInjectMapping findMappingForTargetKey( List<MetaInjectMapping> mappings, MetaInjectMapping template) { + // Both keys are normalized so a legacy injection key (e.g. ALLOW_EMPTY_STREAM_ON_EXECUTION) + // that was renamed (to CONDITION_STREAM_BEHAVIOR) still matches the current tree row; the + // mapping's source then stays visible and is not offered for removal. + // for (MetaInjectMapping m : mappings) { if (Const.NVL(m.getTargetTransformName(), "") .equalsIgnoreCase(Const.NVL(template.getTargetTransformName(), "")) - && Const.NVL(m.getTargetAttributeKey(), "") - .equalsIgnoreCase(Const.NVL(template.getTargetAttributeKey(), ""))) { + && MetaInject.resolveInjectionKey(m) Review Comment: **[bug]** `findMappingForTargetKey` resolves `ALLOW_EMPTY_STREAM_ON_EXECUTION` onto the `CONDITION_STREAM_BEHAVIOR` tree row, but `enterMapping()` does not. It builds `MappingTarget` with the saved `mapping.getTargetAttributeKey()` (around line 1409) and `MappingTarget.equals` compares that string to the current property key. A streamed mapping that still stores `ALLOW_EMPTY_STREAM_ON_EXECUTION` gets `targetIndex == -1`, so it is left out of the mapping dialog. Confirming the dialog clears `targetMappings` and writes back only the matches, which deletes a mapping that execution would have honored. **Suggestion:** Build that `MappingTarget` with `MetaInject.resolveInjectionKey(mapping)` (same normalization as the tree and `getUnavailableTargetKeys`). Add a test that a streamed legacy key is pre-selected in the enter-mapping index and survives a confirm. ########## plugins/transforms/metainject/src/main/java/org/apache/hop/pipeline/transforms/metainject/MetaInjectDialog.java: ########## @@ -1239,7 +1255,8 @@ private void getInfo(MetaInjectMeta meta) { meta.setTargetFile(wTargetFile.getText()); meta.setCreateParentFolder(wCreateParentFolder.getSelection()); meta.setNoExecution(!wNoExecution.getSelection()); - meta.setAllowEmptyStreamOnExecution(wAllowEmptyStreamOnExecution.getSelection()); + meta.setConditionStreamBehavior( Review Comment: **[suggestion]** The behavior combo is editable (`SWT.SINGLE | SWT.LEFT | SWT.BORDER`, no `READ_ONLY`). `lookupDescription` returns `ALL_NON_EMPTY` when the text is not a known description, so OK silently stores "run only when every input stream is not empty" for a typo or a cleared field. A selected description round-trips correctly. **Suggestion:** Create the combo with `SWT.READ_ONLY` so the only possible values are `ConditionStreamBehavior.getDescriptions()`. ########## plugins/transforms/metainject/src/test/java/org/apache/hop/pipeline/transforms/metainject/MetaInjectTest.java: ########## @@ -191,6 +195,272 @@ void collectInjectionKeyValue_resolvesVariablesInConstant() throws Exception { assertEquals("resolved-value", injectionKeyData.get("SOME_KEY")); } + /** + * Backward compatibility (PR #9078 review): a metadata-injection mapping written before {@link Review Comment: **[suggestion]** Several new test comments narrate the review history ("PR #9078 review", "PR #9078 review (round 2)" at lines 199, 325, 384, and 414) instead of stating the behavior under test. The same narration shows up in the `resolveInjectionKey` javadoc's claim that every dialog match uses the normalized key. **Suggestion:** Describe the legacy key and the expected enum, and drop the review-round references. -- 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]
