exceptionfactory commented on code in PR #11086:
URL: https://github.com/apache/nifi/pull/11086#discussion_r3022167208
##########
nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/util/FlowDifferenceFilters.java:
##########
@@ -909,6 +910,57 @@ public static boolean
isPropertyRenameWithMatchingValue(final FlowDifference dif
return evaluatedContext.propertyRenamesWithMatchingValues().isEmpty()
? false :
evaluatedContext.propertyRenamesWithMatchingValues().contains(difference);
}
+ /**
+ * Determines whether a PROPERTY_ADDED difference is an environmental
change because the added property is a
+ * statically-defined property on the component. Static properties are
defined in component code and cannot be
+ * added by users. When a PROPERTY_ADDED diff exists for a static
property, it means the component's code was
+ * updated (e.g., via NiFi upgrade or bundle version change) and migration
added the property. This is an
+ * environmental change regardless of whether a corresponding
BUNDLE_CHANGED diff is present, because the
+ * VCI baseline may already have resolved bundles (e.g., after {@code
discoverCompatibleBundles} during import).
+ *
+ * <p>Trade-off: if a user subsequently edits a migration-added property,
that edit will also be classified as
+ * environmental because the diff relative to the registry is still
PROPERTY_ADDED (the registry version predates
+ * the migration). This means such edits will not make the flow dirty.
However, the change remains visible in the
+ * "Show Local Changes" environmental view, and when the user commits for
any other reason, the full flow
+ * (including the edited property) is saved to the registry.</p>
+ */
+ public static boolean isNewStaticPropertyOnComponent(final FlowDifference
difference, final FlowManager flowManager) {
Review Comment:
It looks like this method should be `protected` or `private` since it is not
called outside of this class.
##########
nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/util/FlowDifferenceFilters.java:
##########
@@ -909,6 +910,57 @@ public static boolean
isPropertyRenameWithMatchingValue(final FlowDifference dif
return evaluatedContext.propertyRenamesWithMatchingValues().isEmpty()
? false :
evaluatedContext.propertyRenamesWithMatchingValues().contains(difference);
}
+ /**
+ * Determines whether a PROPERTY_ADDED difference is an environmental
change because the added property is a
+ * statically-defined property on the component. Static properties are
defined in component code and cannot be
+ * added by users. When a PROPERTY_ADDED diff exists for a static
property, it means the component's code was
+ * updated (e.g., via NiFi upgrade or bundle version change) and migration
added the property. This is an
+ * environmental change regardless of whether a corresponding
BUNDLE_CHANGED diff is present, because the
+ * VCI baseline may already have resolved bundles (e.g., after {@code
discoverCompatibleBundles} during import).
+ *
+ * <p>Trade-off: if a user subsequently edits a migration-added property,
that edit will also be classified as
+ * environmental because the diff relative to the registry is still
PROPERTY_ADDED (the registry version predates
+ * the migration). This means such edits will not make the flow dirty.
However, the change remains visible in the
+ * "Show Local Changes" environmental view, and when the user commits for
any other reason, the full flow
+ * (including the edited property) is saved to the registry.</p>
+ */
+ public static boolean isNewStaticPropertyOnComponent(final FlowDifference
difference, final FlowManager flowManager) {
+ if (difference.getDifferenceType() != DifferenceType.PROPERTY_ADDED) {
+ return false;
+ }
+
+ final Optional<String> fieldName = difference.getFieldName();
+ if (fieldName.isEmpty()) {
+ return false;
+ }
+
+ final VersionedComponent componentB = difference.getComponentB();
Review Comment:
Should any other types of components be considered in addition to Processors
and Controller Services?
##########
nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/util/FlowDifferenceFilters.java:
##########
@@ -909,6 +910,57 @@ public static boolean
isPropertyRenameWithMatchingValue(final FlowDifference dif
return evaluatedContext.propertyRenamesWithMatchingValues().isEmpty()
? false :
evaluatedContext.propertyRenamesWithMatchingValues().contains(difference);
}
+ /**
+ * Determines whether a PROPERTY_ADDED difference is an environmental
change because the added property is a
+ * statically-defined property on the component. Static properties are
defined in component code and cannot be
+ * added by users. When a PROPERTY_ADDED diff exists for a static
property, it means the component's code was
+ * updated (e.g., via NiFi upgrade or bundle version change) and migration
added the property. This is an
+ * environmental change regardless of whether a corresponding
BUNDLE_CHANGED diff is present, because the
+ * VCI baseline may already have resolved bundles (e.g., after {@code
discoverCompatibleBundles} during import).
+ *
+ * <p>Trade-off: if a user subsequently edits a migration-added property,
that edit will also be classified as
+ * environmental because the diff relative to the registry is still
PROPERTY_ADDED (the registry version predates
+ * the migration). This means such edits will not make the flow dirty.
However, the change remains visible in the
+ * "Show Local Changes" environmental view, and when the user commits for
any other reason, the full flow
+ * (including the edited property) is saved to the registry.</p>
+ */
+ public static boolean isNewStaticPropertyOnComponent(final FlowDifference
difference, final FlowManager flowManager) {
+ if (difference.getDifferenceType() != DifferenceType.PROPERTY_ADDED) {
+ return false;
+ }
+
+ final Optional<String> fieldName = difference.getFieldName();
+ if (fieldName.isEmpty()) {
+ return false;
+ }
+
+ final VersionedComponent componentB = difference.getComponentB();
+ if (componentB instanceof InstantiatedVersionedProcessor) {
+ final ProcessorNode processorNode =
flowManager.getProcessorNode(componentB.getInstanceIdentifier());
+ return isStaticProperty(fieldName.get(), processorNode);
+ } else if (componentB instanceof
InstantiatedVersionedControllerService) {
+ final ControllerServiceNode controllerService =
flowManager.getControllerServiceNode(componentB.getInstanceIdentifier());
+ return isStaticProperty(fieldName.get(), controllerService);
+ }
+
+ return false;
+ }
+
+ private static boolean isStaticProperty(final String propertyName, final
ComponentNode componentNode) {
+ if (componentNode == null) {
+ return false;
+ }
+
+ final ConfigurableComponent configurableComponent =
componentNode.getComponent();
+ if (configurableComponent == null) {
+ return false;
+ }
+
+ return configurableComponent.getPropertyDescriptors().stream()
+ .map(PropertyDescriptor::getName)
+ .anyMatch(propertyName::equals);
+ }
Review Comment:
Similar to above, recommend a single return.
##########
nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/util/FlowDifferenceFilters.java:
##########
@@ -909,6 +910,57 @@ public static boolean
isPropertyRenameWithMatchingValue(final FlowDifference dif
return evaluatedContext.propertyRenamesWithMatchingValues().isEmpty()
? false :
evaluatedContext.propertyRenamesWithMatchingValues().contains(difference);
}
+ /**
+ * Determines whether a PROPERTY_ADDED difference is an environmental
change because the added property is a
+ * statically-defined property on the component. Static properties are
defined in component code and cannot be
+ * added by users. When a PROPERTY_ADDED diff exists for a static
property, it means the component's code was
+ * updated (e.g., via NiFi upgrade or bundle version change) and migration
added the property. This is an
+ * environmental change regardless of whether a corresponding
BUNDLE_CHANGED diff is present, because the
+ * VCI baseline may already have resolved bundles (e.g., after {@code
discoverCompatibleBundles} during import).
+ *
+ * <p>Trade-off: if a user subsequently edits a migration-added property,
that edit will also be classified as
+ * environmental because the diff relative to the registry is still
PROPERTY_ADDED (the registry version predates
+ * the migration). This means such edits will not make the flow dirty.
However, the change remains visible in the
+ * "Show Local Changes" environmental view, and when the user commits for
any other reason, the full flow
+ * (including the edited property) is saved to the registry.</p>
+ */
+ public static boolean isNewStaticPropertyOnComponent(final FlowDifference
difference, final FlowManager flowManager) {
+ if (difference.getDifferenceType() != DifferenceType.PROPERTY_ADDED) {
+ return false;
+ }
+
+ final Optional<String> fieldName = difference.getFieldName();
+ if (fieldName.isEmpty()) {
+ return false;
+ }
+
+ final VersionedComponent componentB = difference.getComponentB();
+ if (componentB instanceof InstantiatedVersionedProcessor) {
+ final ProcessorNode processorNode =
flowManager.getProcessorNode(componentB.getInstanceIdentifier());
+ return isStaticProperty(fieldName.get(), processorNode);
+ } else if (componentB instanceof
InstantiatedVersionedControllerService) {
+ final ControllerServiceNode controllerService =
flowManager.getControllerServiceNode(componentB.getInstanceIdentifier());
+ return isStaticProperty(fieldName.get(), controllerService);
+ }
+
+ return false;
Review Comment:
Given the number of `return` statements, I recommend refactoring this method
to have a single return, instead of the short-circuit approach.
##########
nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/util/FlowDifferenceFilters.java:
##########
@@ -909,6 +910,57 @@ public static boolean
isPropertyRenameWithMatchingValue(final FlowDifference dif
return evaluatedContext.propertyRenamesWithMatchingValues().isEmpty()
? false :
evaluatedContext.propertyRenamesWithMatchingValues().contains(difference);
}
+ /**
+ * Determines whether a PROPERTY_ADDED difference is an environmental
change because the added property is a
+ * statically-defined property on the component. Static properties are
defined in component code and cannot be
+ * added by users. When a PROPERTY_ADDED diff exists for a static
property, it means the component's code was
+ * updated (e.g., via NiFi upgrade or bundle version change) and migration
added the property. This is an
+ * environmental change regardless of whether a corresponding
BUNDLE_CHANGED diff is present, because the
+ * VCI baseline may already have resolved bundles (e.g., after {@code
discoverCompatibleBundles} during import).
+ *
+ * <p>Trade-off: if a user subsequently edits a migration-added property,
that edit will also be classified as
+ * environmental because the diff relative to the registry is still
PROPERTY_ADDED (the registry version predates
+ * the migration). This means such edits will not make the flow dirty.
However, the change remains visible in the
+ * "Show Local Changes" environmental view, and when the user commits for
any other reason, the full flow
+ * (including the edited property) is saved to the registry.</p>
+ */
+ public static boolean isNewStaticPropertyOnComponent(final FlowDifference
difference, final FlowManager flowManager) {
+ if (difference.getDifferenceType() != DifferenceType.PROPERTY_ADDED) {
+ return false;
+ }
+
+ final Optional<String> fieldName = difference.getFieldName();
+ if (fieldName.isEmpty()) {
+ return false;
+ }
+
+ final VersionedComponent componentB = difference.getComponentB();
+ if (componentB instanceof InstantiatedVersionedProcessor) {
+ final ProcessorNode processorNode =
flowManager.getProcessorNode(componentB.getInstanceIdentifier());
+ return isStaticProperty(fieldName.get(), processorNode);
+ } else if (componentB instanceof
InstantiatedVersionedControllerService) {
+ final ControllerServiceNode controllerService =
flowManager.getControllerServiceNode(componentB.getInstanceIdentifier());
+ return isStaticProperty(fieldName.get(), controllerService);
+ }
+
+ return false;
+ }
+
+ private static boolean isStaticProperty(final String propertyName, final
ComponentNode componentNode) {
Review Comment:
Perhaps `isNotDynamicProperty()` to avoid potential confusion around the
word `static`, and align with the API naming for dynamic properties?
##########
nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/util/FlowDifferenceFilters.java:
##########
@@ -909,6 +910,57 @@ public static boolean
isPropertyRenameWithMatchingValue(final FlowDifference dif
return evaluatedContext.propertyRenamesWithMatchingValues().isEmpty()
? false :
evaluatedContext.propertyRenamesWithMatchingValues().contains(difference);
}
+ /**
+ * Determines whether a PROPERTY_ADDED difference is an environmental
change because the added property is a
+ * statically-defined property on the component. Static properties are
defined in component code and cannot be
+ * added by users. When a PROPERTY_ADDED diff exists for a static
property, it means the component's code was
+ * updated (e.g., via NiFi upgrade or bundle version change) and migration
added the property. This is an
+ * environmental change regardless of whether a corresponding
BUNDLE_CHANGED diff is present, because the
+ * VCI baseline may already have resolved bundles (e.g., after {@code
discoverCompatibleBundles} during import).
+ *
+ * <p>Trade-off: if a user subsequently edits a migration-added property,
that edit will also be classified as
+ * environmental because the diff relative to the registry is still
PROPERTY_ADDED (the registry version predates
+ * the migration). This means such edits will not make the flow dirty.
However, the change remains visible in the
+ * "Show Local Changes" environmental view, and when the user commits for
any other reason, the full flow
+ * (including the edited property) is saved to the registry.</p>
+ */
+ public static boolean isNewStaticPropertyOnComponent(final FlowDifference
difference, final FlowManager flowManager) {
Review Comment:
Perhaps `isPropertyAddedFromMigration` to align more closely with the intent
of the method?
--
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]
