You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2020/11/24 19:47:16 UTC
[GitHub] [nifi] markap14 opened a new pull request #4685: NIFI-8042: Fixed bug that was escaping Expression Language references…
markap14 opened a new pull request #4685:
URL: https://github.com/apache/nifi/pull/4685
… 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 `squash` or use `--force` when pushing to allow for clean monitoring of changes._
### For code changes:
- [ ] Have you ensured that the full suite of tests is executed via `mvn -Pcontrib-check clean install` at the root `nifi` folder?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] Have you verified that the full build is successful on JDK 8?
- [ ] Have you verified that the full build is successful on JDK 11?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the `LICENSE` file, including the main `LICENSE` file under `nifi-assembly`?
- [ ] If applicable, have you updated the `NOTICE` file, including the main `NOTICE` file found under `nifi-assembly`?
- [ ] If adding new Properties, have you added `.displayName` in addition to .name (programmatic access) for each of the new properties?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### 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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ottobackwards commented on pull request #4685: NIFI-8042: Fixed bug that was escaping Expression Language references…
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on pull request #4685:
URL: https://github.com/apache/nifi/pull/4685#issuecomment-737511510
+1 fwiw
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markap14 commented on a change in pull request #4685: NIFI-8042: Fixed bug that was escaping Expression Language references…
Posted by GitBox <gi...@apache.org>.
markap14 commented on a change in pull request #4685:
URL: https://github.com/apache/nifi/pull/4685#discussion_r534527090
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ReplaceText.java
##########
@@ -602,8 +602,7 @@ public boolean isAllDataBufferedForEntireText() {
@Override
public FlowFile replace(FlowFile flowFile, final ProcessSession session, final ProcessContext context, final String evaluateMode, final Charset charset, final int maxBufferSize) {
Review 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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markap14 commented on a change in pull request #4685: NIFI-8042: Fixed bug that was escaping Expression Language references…
Posted by GitBox <gi...@apache.org>.
markap14 commented on a change in pull request #4685:
URL: https://github.com/apache/nifi/pull/4685#discussion_r534465100
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestReplaceText.java
##########
@@ -53,6 +54,66 @@ public TestRunner getRunner() {
return runner;
}
Review 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 = ____
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ottobackwards commented on a change in pull request #4685: NIFI-8042: Fixed bug that was escaping Expression Language references…
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on a change in pull request #4685:
URL: https://github.com/apache/nifi/pull/4685#discussion_r533513856
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ReplaceText.java
##########
@@ -602,8 +602,7 @@ public boolean isAllDataBufferedForEntireText() {
@Override
public FlowFile replace(FlowFile flowFile, final ProcessSession session, final ProcessContext context, final String evaluateMode, final Charset charset, final int maxBufferSize) {
Review 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?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ottobackwards commented on a change in pull request #4685: NIFI-8042: Fixed bug that was escaping Expression Language references…
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on a change in pull request #4685:
URL: https://github.com/apache/nifi/pull/4685#discussion_r534497643
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ReplaceText.java
##########
@@ -602,8 +602,7 @@ public boolean isAllDataBufferedForEntireText() {
@Override
public FlowFile replace(FlowFile flowFile, final ProcessSession session, final ProcessContext context, final String evaluateMode, final Charset charset, final int maxBufferSize) {
Review 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
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestReplaceText.java
##########
@@ -53,6 +54,66 @@ public TestRunner getRunner() {
return runner;
}
Review comment:
Sorry, please disregard. I literally skipped past the LITERAL part of this.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ottobackwards commented on a change in pull request #4685: NIFI-8042: Fixed bug that was escaping Expression Language references…
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on a change in pull request #4685:
URL: https://github.com/apache/nifi/pull/4685#discussion_r533514874
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestReplaceText.java
##########
@@ -53,6 +54,66 @@ public TestRunner getRunner() {
return runner;
}
Review 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?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] asfgit closed pull request #4685: NIFI-8042: Fixed bug that was escaping Expression Language references…
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #4685:
URL: https://github.com/apache/nifi/pull/4685
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markap14 commented on a change in pull request #4685: NIFI-8042: Fixed bug that was escaping Expression Language references…
Posted by GitBox <gi...@apache.org>.
markap14 commented on a change in pull request #4685:
URL: https://github.com/apache/nifi/pull/4685#discussion_r534464347
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ReplaceText.java
##########
@@ -602,8 +602,7 @@ public boolean isAllDataBufferedForEntireText() {
@Override
public FlowFile replace(FlowFile flowFile, final ProcessSession session, final ProcessContext context, final String evaluateMode, final Charset charset, final int maxBufferSize) {
Review 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 :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org