NIFI-8042: Fixed bug that was escaping Expression Language references…#4685
NIFI-8042: Fixed bug that was escaping Expression Language references…#4685markap14 wants to merge 1 commit intoapache:mainfrom
Conversation
… for use in a Regular Expression (i.e., Pattern.quote) even though it wasn't being used in a Regular Expression
| @@ -602,8 +602,7 @@ private static class LiteralReplace implements ReplacementStrategyExecutor { | |||
| @Override | |||
| public FlowFile replace(FlowFile flowFile, final ProcessSession session, final ProcessContext context, final String evaluateMode, final Charset charset, final int maxBufferSize) { | |||
There was a problem hiding this comment.
I think one of the issues with maintaining this processor is that the reasons why things are quoted, escaped or not escaped are not documented in the code.
Maybe a comment here would be helpful?
There was a problem hiding this comment.
I can understand why the existence of it and the removal of it would be confusing. I don't think a comment in the code would be helpful though. It never should have been escaped. I think it was escaped as a result of refactoring the code, or perhaps because it is escaped when using a Regular Expression as the search value.... but if we added an inline comment about why we're not arbitrarily escaping something that doesn't need escaping, outside of the scope of this PR, I think it would result in more confusion than clarification :)
There was a problem hiding this comment.
fair enough. In general, I think better dev comments about what is going on in this class would help. Point taken on this specific ask
There was a problem hiding this comment.
Yeah, I cannot argue that point with you. Like many processors, this one started pretty simple, once upon a time. And a new feature was added. And another. And it's become quite the beast. Definitely wouldn't hurt to updates with some docs. And probably would help to add some additionalDetails.html, too, to be honest, because there are a lot of options here.
| @@ -53,6 +54,66 @@ public TestRunner getRunner() { | |||
| return runner; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
I think the edge case is where there is a mix of expression language and other things like captures etc.
Maybe a multi-case test would be appropriate?
There was a problem hiding this comment.
I'm not sure that I follow your suggestion. I feel it is quite heavily tested at this point. Can you provide an example configuration that you think would be worthwhile testing? As in, provide a set of:
Search Value = ___
Replacement Value = ___
Replacement Strategy = ___
Sample input = ____
Expected output = ____
There was a problem hiding this comment.
Sorry, please disregard. I literally skipped past the LITERAL part of this.
|
+1 fwiw |
… for use in a Regular Expression (i.e., Pattern.quote) even though it wasn't being used in a Regular Expression This closes apache#4685 Signed-off-by: Mike Thomsen <mthomsen@apache.org>
… for use in a Regular Expression (i.e., Pattern.quote) even though it wasn't being used in a Regular Expression This closes apache#4685 Signed-off-by: Mike Thomsen <mthomsen@apache.org>
3 participants
… for use in a Regular Expression (i.e., Pattern.quote) even though it wasn't being used in a Regular Expression
Thank you for submitting a contribution to Apache NiFi.
Please provide a short description of the PR here:
Description of PR
Enables X functionality; fixes bug NIFI-YYYY.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically
main)?Is your initial contribution a single, squashed commit? Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not
squashor use--forcewhen pushing to allow for clean monitoring of changes.For code changes:
mvn -Pcontrib-check clean installat the rootnififolder?LICENSEfile, including the mainLICENSEfile undernifi-assembly?NOTICEfile, including the mainNOTICEfile found undernifi-assembly?.displayNamein addition to .name (programmatic access) for each of the new properties?For documentation related changes:
Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI for build issues and submit an update to your PR as soon as possible.