bamaer commented on PR #8419: URL: https://github.com/apache/hop/pull/8419#issuecomment-5710533422
@mattcasters all 11 items addressed. Two of them needed a different fix than suggested; details below. ### Bugs | # | Fix | |---|---| | 1 | `TextChunkerMeta.supportsErrorHandling()` returns true, so the `putError` branch is reachable | | 2 | Gone with the dialog rewrite: no hand-rolled focus listener, and each combo's selection is restored individually when filled | | 3 | `chunkSize` and `chunkOverlap` are `String`, resolved once in `init()` into `TextChunkerData`, not per row. `check()` resolves before validating | | 4 | `start = overlap > 0 ? Math.max(start + 1, chunkEnd - overlap) : chunkEnd`, so overlap no longer depends on finding a boundary. Boundary detection also moved from a 5-character ASCII list to `Character.isWhitespace` | | 5 | `HopMetadataJsonParser` no longer dumps the raw connection node, and `prettyJson` redacts, which covers the other three call sites | | 6 | `HopXmlSupport.serializeElement` redacts secret-bearing tags | | 7 | Paragraphs pack up to `maxSize`, as the manual states | **5 and 6** share a new `SecretRedaction` helper. Names are normalised before matching, so `client_secret`, `API-KEY` and `private_key` all hit, and any value starting with `Encrypted ` is redacted whatever the field is called. A bare `key` is deliberately **not** matched: it is an ordinary field name in lookups and joins. New IT `0117-text-chunker-redaction` shows `lookupKeys: customer_id` surviving next to `password: **redacted**`. **7 needed a different implementation than suggested.** Rebuilding the chunk by joining with `\n\n` broke a correctness invariant: a test caught that start and end positions no longer addressed the source, since the real separator might be `\n \n`. Packing now takes the source slice between the first and last paragraph, so content and positions both stay faithful. That change also exposed a test asserting `sum(chunk lengths) == text.length()`, which **only held because overlap was being skipped**. Rewritten to assert coverage, correct addressing, and that a word with no whitespace still overlaps. ### Suggestions **Dialog.** Rewritten onto `GuiCompositeWidgets`, 401 lines down to 186. `TextChunkerMeta` has `@GuiPlugin` and 13 `@GuiWidgetElement` annotations in three `BOXES` groups. `registerExtraGroup` was not needed: `widgets.getWidgetsMap().get(widgetId)` returns the created control, so the three stream-field combos are annotated normally and filled from `pipelineMeta.getPrevTransformFields(...)` after creation, the way `ParquetOutputDialog` fills its table columns. `comboValuesMethod` cannot do this, since it only receives a log channel and a metadata provider. Verified in Hop GUI. **Zero chunk size.** `init()` now fails with a logged error rather than dropping the row, which closes the metadata-injection path. ### Nits Tooltip says `article/blog=Markdown`. The empty-text message says an empty chunk row is emitted. ### Verification 92 unit tests, up from 66. Both redaction fixes were verified by disabling redaction and confirming the unit tests and the IT fail, with `password: letmein` and `"proxyPassword" : "hunter2"` appearing in the output. Four integration tests pass. `0115-text-chunker-paragraph` moved from `chunkSize=60` to `25` so the three paragraphs still split and the test keeps its purpose under the new packing rule. The seven unit tests that encoded one-chunk-per-paragraph were not simply flipped: the ones testing paragraph *detection* keep that intent via a smaller `maxSize`, and two new tests cover packing itself. 0 checkstyle violations, spotless and rat clean. -- 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]
