mattcasters opened a new pull request, #11643:
URL: https://github.com/apache/nifi/pull/11643

   # Summary
   
   This addresses 
[NIFI-15694](https://issues.apache.org/jira/browse/NIFI-15694): after a NiFi 
upgrade, property migrations on processors/controller services are reported as 
**local modifications** on version-controlled process groups. That blocks 
Change Version / Git deploy, and Revert does not stick because 
`migrateProperties` is applied again.
   
   ## Use case
   
   A customer is upgrading **NiFi 2.9.0 → 2.11.0** with process groups tracked 
in Git.
   
   `AvroSchemaRegistry` (NIFI-15960) replaced:
   
   | 2.9 | 2.11 |
   | --- | --- |
   | `Validate Field Names` / `avro-reg-validated-field-names` = `true` / 
`false` | `Validation Strategy` = `VALIDATE` / `NONE` |
   
   On startup / flow sync, `migrateProperties()` rewrites the live flow and 
`flow.json`. The Git (or Registry) snapshot for the currently tracked version 
still has the old property. `getModifications()` then reports:
   
   - `PROPERTY_REMOVED` `Validate Field Names`
   - `PROPERTY_ADDED` `Validation Strategy`
   
   The group becomes `LOCALLY_MODIFIED` (or `LOCALLY_MODIFIED_AND_STALE` if Git 
also has newer commits). **Change Version is refused** because the group is 
dirty.
   
   **Revert** applies the Git snapshot (old property), then 
`StandardVersionedComponentSynchronizer` re-runs `migrateProperties` on the 
updated component so it remains valid. The new property comes back and the 
group is still dirty. Revert looks successful in the UI, then the local change 
reappears.
   
   This is the same general failure mode described on NIFI-15694 (also seen 
with e.g. ConsumeAzureEventHub `Authentication Strategy`). `AvroSchemaRegistry` 
is an especially sharp instance because it is `@DynamicProperty` (named 
schemas): `isStaticPropertyRemoved` previously refused to treat the dropped 
**static** property as environmental, so it looked like a user-deleted dynamic 
property.
   
   Re-running migration after loading Git content is correct. The bug is 
classifying those diffs as user edits.
   
   ## Changes
   
   No change to `AvroSchemaRegistry.migrateProperties` itself.
   
   1. **`isStaticPropertyRemoved`**  
      If the versioned snapshot recorded the removed property as 
**non-dynamic**, treat it as a dropped static property even when the live 
component supports dynamic properties. User-deleted schemas (`dynamic=true`) 
remain local changes.
   
   2. **Dry-run `migrateProperties`**  
      `ComponentNode.previewMigratedProperties()` (implemented for processors 
and controller services via `PropertyMigrationPreview`) runs 
`migrateProperties` on a copy of the snapshot properties. It does not persist 
and does not create controller services (NAR classloader is used in 
production).  
      `FlowDifferenceFilters.buildEnvironmentalChangeContext` marks a property 
diff environmental only when that dry-run **explains** it **and** the local 
value equals the migrated value:
      - `true` → `VALIDATE` and `false` → `NONE` are both environmental
      - a later user edit of the new property (dry-run `VALIDATE`, local 
`NONE`) stays a local change (NIFI-15863 preserved more accurately)
   
   3. **Tests** in `TestFlowDifferenceFilters` and 
`TestPropertyMigrationPreview` covering snapshot-static removal on a 
`@DynamicProperty` component, user-removed dynamic properties, both 
boolean→enum mappings, and post-migration user edits.
   
   ## Customer workarounds (until this is released)
   
   Revert cannot clear the dirty state on an upgraded node.
   
   1. **After this change:** upgrade every stage, then Change Version / deploy 
from Git. Committing the property rename first is not required for deploy to 
work.
   2. **Today, if every stage is already on 2.11:** commit the migrated flow 
from one environment so Git contains `Validation Strategy`, then on the other 
environments **stop version control and start it again** from that commit (or 
rewrite the stored version identifier as described on the Jira). Revert will 
not help.
   3. **Do not** commit the migrated properties from 2.11 and deploy that 
commit onto a node still on 2.9: `Validation Strategy` would be stored as a 
**dynamic schema** on 2.9 `AvroSchemaRegistry`.
   
   # Tracking
   
   - [x] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI-15694) 
issue created
   - [x] Pull Request title starts with Apache NiFi Jira issue number
   - [x] Pull Request commit message starts with Apache NiFi Jira issue number
   
   # Verification
   
   - [x] `./mvnw -pl 
nifi-framework-bundle/nifi-framework/nifi-framework-components -am test 
-Dtest=TestFlowDifferenceFilters,TestPropertyMigrationPreview 
-Dsurefire.failIfNoSpecifiedTests=false`
   - [ ] Full `contrib-check` not run on this draft (happy to run if reviewers 
want it)
   
   # Review
   
   This is a **draft** — I would very much appreciate a code review on the 
approach before we treat it as ready to merge.
   
   In particular I would like feedback on:
   
   - Using a dry-run of `migrateProperties` as the source of truth vs. more 
value-matching heuristics
   - The snapshot `dynamic=false` exemption in `isStaticPropertyRemoved` for 
`@DynamicProperty` components
   - Whether `previewMigratedProperties` belongs on `ComponentNode` (default 
no-op) with processor/CS overrides
   
   Thank you in advance for taking a look.


-- 
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]

Reply via email to