Repository navigation
KNOX-3556: Rewrite: add find option to apply so URLs embedded in HTML attribute values and style text can be rewritten - #1497
Conversation
Test Results 5 files 5 suites 1m 38s ⏱️ Results for commit 83c6076. ♻️ This comment has been updated with latest results. |
Co-authored-by: Claude <noreply@anthropic.com>
…d. Update dev-guide. Co-authored-by: Claude <noreply@anthropic.com>
smolnar82
left a comment
smolnar82
left a comment
There was a problem hiding this comment.
Thanks, the approach looks good.
One naming nit: find reflects the Matcher.find() vs matches() implementation detail, but to a service-definition author find="true" doesn't really say "rewrite every occurrence within the value instead of requiring a whole-value match".
Something like match="partial" (default "whole") or scope="substring" would be more self-explanatory, and an enum also leaves room for other modes later. Since no service definition uses it yet, now would be the cheapest time to rename.
| } | ||
|
|
||
| /* | ||
| * The applies are processed one after the other and every apply searches the output of the previous one, so an |
There was a problem hiding this comment.
The applies are chained: each find apply searches the output of the previous one, so with multiple find applies a value can get rewritten twice (e.g. /app -> /gateway/topo/app, then a second find apply matching /gateway/... rewrites it again). That was already the case for <script> content, but with this change it applies to every HTML attribute and text node. Could the dev guide state that find paths must be disjoint/anchored (or should a value be rewritten by at most one find apply)?
There was a problem hiding this comment.
I went with the dev guide option.
Applies are chained on purpose, the same as for JS content, so there's one behavior to document.
I've added a note to the dev guide that several partial applies are chained and their paths must be disjoint/anchored, plus a complete example with a rule and an anchored path.
If a real need for several overlapping partial applies comes up, "rewrite each part at most once" can be added then; it would only change configs that already rewrite twice.
| return this; | ||
| } | ||
|
|
||
| public void setFind( boolean find ) { |
There was a problem hiding this comment.
find is populated via digester's SetPropertiesRule / BeanUtils, which silently converts an invalid value to false (verified: ConvertUtils.convert("ture", boolean.class) returns false with beanutils 1.11.0). So a typo like find="ture" quietly disables the rewrite with no error or log. Not a blocker, but it may be worth validating the attribute (only true/false accepted) or at least logging a warning when loading the rules.
There was a problem hiding this comment.
With the rename to match="partial|whole", the attribute is an enum parsed via Enum.valueOf, so an invalid value like match="parttial" now fails the rules load instead of silently falling back (same as an invalid flow). This is covered by testLoadApplyInvalidMatch.
A misspelled attribute name is still ignored, but that's Digester's SetPropertiesRule default for every rewrite.xml attribute, so I'd leave it out of this ticket.
The test revealed that the digester is not cleared after failure, so if a subsequent load() invocations could fail. I'd keep that as a separate ticket.
find came from Matcher.find() vs matches(). To a service definition
author it doesn't say what it does. Use match="partial" instead, with
match="whole" as the default. An enum also leaves room for other
modes later. No service definition uses the attribute yet.
- Add a UrlRewriteFilterApplyMatch { WHOLE, PARTIAL } enum, like UrlRewriteStepFlow, in place of find(). It is parsed case-insensitively like flow, and the exporter writes it back without a special case.
- Rename the related methods, tests and docs.
Co-Authored-By: Claude <noreply@anthropic.com>
Several match="partial" applies are applied one after the other, so their paths have to be disjoint. Add an example with a rule and a path anchored on both sides. Co-Authored-By: Claude <noreply@anthropic.com>
KNOX-3556 - Add find mode to rewrite
<apply>for HTML attributes and text.What changes were proposed in this pull request?
Add find mode to rewrite
<apply>for HTML attributes and text.Updated dev guide.
How was this patch tested?