mattcasters commented on PR #8247:
URL: https://github.com/apache/hop/pull/8247#issuecomment-5531070231
Hi @leehaut,
Thank you for investigating this issue! You did great detective work
identifying the two root causes behind why pipelines and workflows were getting
marked dirty after simply clicking OK:
1. The copy factories inadvertently flipping `wrapperChanged` via setter
calls like `setLocation`.
2. The asymmetry between `null` and `""` across dialog round-trips.
The fixes to `DefaultTransformMetaCopyFactory`, `DefaultActionCopyFactory`,
and preserving the live `alreadyChanged` flag in the delegates are spot-on.
Those should definitely be merged.
However, I have some strong reservations regarding the additions to
`XmlHandler` (`sameContentIgnoringEmptyValues`) and its usage in
`XmlSnapshotUndo`:
---
### 1. Performance Impact on the UI Thread
In `XmlSnapshotUndo.sameXmlContent`, this comparison is run on
`commitDialogUndo` for the **entire pipeline or workflow XML**.
Calling `XmlHandler.sameContentIgnoringEmptyValues` means:
- Decompressing the entire pipeline snapshot twice.
- Parsing both complete pipeline XML documents into full W3C DOM Document
trees (`wrapLoadXmlString`) via `DocumentBuilder.parse()`.
- Recursively walking every single node, collecting child lists, and
trimming text on the SWT UI thread.
- `applyDirtyFlag` then repeats this check comparing against
`lastSavedSnapshot`.
For large pipelines (dozens or hundreds of transforms, hops, notes, and
connections), running full DOM tree builds and recursive comparisons on the UI
thread whenever a dialog closes introduces significant latency and heavy GC
churn. The snapshot undo system was specifically designed to use fast,
single-pass string comparison: `decompress(left).equals(decompress(right))`.
---
### 2. Edge Cases and Potential Data Loss in the XML Comparator
Implementing a custom XML equivalence crawler introduces subtle bugs:
- **Mixed content / text dropped:** In `sameElementIgnoringEmptyValues`, if
an element has child elements (`!leftChildren.isEmpty()`), `directText(left)`
is never checked. Any direct text on that parent element is ignored in
comparison.
- **False equality on list / table rows:** In `extraEmptyListItemIsIgnored`,
`<fields><field><name>a</name></field></fields>` is considered equal to
`<fields><field><name>a</name></field><field/></fields>`. If a user adds a new
blank row in a table/grid, or clears all values in a row, the comparator
considers it unchanged. The file will not be marked dirty, no undo point will
be created, and the edit won't be saved.
- **Layering:** `XmlHandler` is a low-level XML utility class. It shouldn't
contain domain-specific heuristics where omitted elements ≡ empty elements. In
standard XML, an omitted element and an empty element can have very different
meanings.
---
### 3. Suggested Alternative Approach
The core reason `before.getXml().equals(after.getXml())` failed is:
- An uninitialized field was `null` → `XmlMetadataUtil` omitted the tag.
- The dialog opened, the text widget showed `""`, and on OK the dialog set
the field to `""`.
- `XmlMetadataUtil` serialized `""` as `<tag/>`.
In Hop, empty strings and `null` for text properties are semantically the
same (empty/unset). Hop XML files generally don't need or want empty tags like
`<tag/>` cluttering the document.
Rather than parsing DOM trees after the fact to paper over this difference:
1. **In `XmlMetadataUtil`:** If we don't serialize empty strings (treating
`""` like `null` in `serializeFieldValueToXml`):
```java
if (value != null && !(value instanceof String s && s.isEmpty()))
```
*(or alternatively in `GuiCompositeWidgets` / dialog binding by not
overwriting an existing `null` value with `""` if the control is empty)*
2. Then `before.getXml()` and `after.getXml()` will produce **byte-for-byte
identical XML**.
3. `XmlSnapshotUndo` can revert back to the simple, fast
`decompress(left).equals(decompress(right))`.
What do you think about separating the copy-factory fixes from the XML
comparison, and handling the empty-string serialization at the source instead?
--
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]
