NIFI-7172: Trim whitespace from NiFi properties by eaolson · Pull Request #4854 · apache/nifi

eaolson · 2021-02-28T20:55:18Z

Description of PR

Fixes NIFI-7172. Trims whitespace from NiFi properties. Multi-line properties are truncated at the first line break. The underlying java.util.Properties actually takes care of leading whitespace, but it's included in the test case as well.

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?
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?

… are truncated at the first line terminator.

ChrisSamo632 · 2021-03-03T21:29:23Z

...rk/nifi-properties-loader/src/main/java/org/apache/nifi/properties/NiFiPropertiesLoader.java

            rawProperties.load(inStream);
            logger.info("Loaded {} properties from {}", rawProperties.size(), file.getAbsolutePath());

+            // Trim whitespace from each property. If property is multi-line, remove anything after the first line break.


Should we allow for whitespace at the start/end of properties if the user wants it (e.g. a password)? So either a flag (in the properties) to enable/disable the whitespace trimming or an escape/quote megabits, e.g. "foo " would retain the trailing whitespace but not the quotes?

Suspect this is somewhat of an edge case but always running whitespace can lead to frustrations

Allow me to counterpoint:

Leading whitespace on the value is taken care of by the underlying java.util.Properties . That's already being stripped out and this pull request doesn't change that. So to support leading whitespace would mean writing a whole new reader.

Similarly, trailing whitespace on the key is already taken care of by the Properties instance. So supporting leading whitespace would mean the possibility of "foo=bar" and "foo = bar" (which are currently equivalent) being different. And "foo =bar" would be equivalent to the former and not the latter. If nothing else, "supportLeadingWhitespace=false" should be the default, because you just know someone out there has configured their properties with spaces around the "=" and it's working now.

Supporting surrounding quote characters just changes the situation from "no trailing whitespace" to "no leading or trailing quote marks", though I supposed you could have a quote inside the value if you escape it somehow.

Yeah, I realised the quotes issue after mashing the comment (i.e. just chasing it to handle surrounding quotes instead of whitespace)

Allowing for multi-line properties is good and as I say, this would be something of an edge case that isn't likely to happen regularly. If the standard property reader already removes such things anyway then as you say, maybe better to leave as-is

ChrisSamo632 · 2021-03-04T06:33:07Z

...ties-loader/src/test/groovy/org/apache/nifi/properties/NiFiPropertiesLoaderGroovyTest.groovy

+        assert niFiProperties.getProperty("nifi.whitespace.propWithLeadingSpace") == "foo";
+        assert niFiProperties.getProperty("nifi.whitespace.propWithTrailingSpace") == "foo";
+        assert niFiProperties.getProperty("nifi.whitespace.propWithLeadingAndTrailingSpace") == "foo";
+        assert niFiProperties.getProperty("nifi.whitespace.propWithTrailingTab") == "foo";


Looks like this test is failing in all the checks, looking at your regex, not sure you're currently removing tab characters

I think I misunderstood how multiline properties worked and this change fixes that. That simplified the regex a lot. Users can still embed a newline character with "\n". I figure if they go to that much trouble, they should be allowed to. The tab issue has been fixed. I also moved the test into a test class that executes under Windows, where I'm working. The failing check seems to have nothing to do with my change.

JDK 11 build failed still, though can't see from the errors whether that's caused by your changes or an intermittent issue with the build (and therefore whether a simple re-execution of that check may let it pass)?

…ngTab to use \t instead of a binary tab; fails contrib-check. Move test to a class that runs under Windows.

ChrisSamo632 · 2021-03-08T09:39:37Z

...rk/nifi-properties-loader/src/main/java/org/apache/nifi/properties/NiFiPropertiesLoader.java

+                String key = itr.next();
+                String prop = rawProperties.getProperty(key);
+                if(!prop.isEmpty()){
+                    prop = Pattern.compile("\\s+$").matcher(prop).replaceFirst("");


Probably better to define the Pattern once as a class-level static final field or use the String class' replace such as prop.replace("\\s+$", ""), although I'd guess using the pre-compiled Pattern would be slightly more efficient... but this code is probably only run once at startup anyway so it's unlikely a big deal (but I'd still prefer Pattern not to be compiled every time within the while loop)

I've moved the pattern compilation out of the loop.

exceptionfactory

Thanks for your contribution @eaolson. There appear to be some opportunities to optimize the approach. It would also be helpful to assign the associated NiFi Jira issue to yourself and set the status to Patch Available based on your work.

...rk/nifi-properties-loader/src/main/java/org/apache/nifi/properties/NiFiPropertiesLoader.java

...ifi-framework/nifi-properties-loader/src/test/resources/conf/nifi_with_whitespace.properties

Incorporate suggestions from exceptionfactory.

exceptionfactory · 2021-03-24T12:47:06Z

Thanks for making the adjustments @eaolson. Do you have any additional comments @ChrisSamo632?

ChrisSamo632 · 2021-03-24T13:32:13Z

Thanks for making the adjustments @eaolson. Do you have any additional comments @ChrisSamo632?

@exceptionfactory nothing more from me (I can't see how to resolve conversations, maybe I don't have access, nevermind)

exceptionfactory · 2021-03-25T20:21:42Z

Thanks for making the adjustments @eaolson. Do you have any additional comments @ChrisSamo632?

@exceptionfactory nothing more from me (I can't see how to resolve conversations, maybe I don't have access, nevermind)

Thanks for the confirmation @ChrisSamo632!

exceptionfactory

Thanks for your work on this @eaolson! +1 Merging.

This closes apache#4854 Signed-off-by: David Handermann <exceptionfactory@apache.org>

NIFI-7172: Trim whitespace from NiFi properties. Multiline properties…

ba0849a

… are truncated at the first line terminator.

ChrisSamo632 reviewed Mar 3, 2021

View reviewed changes

ChrisSamo632 reviewed Mar 4, 2021

View reviewed changes

NIFI-7272: Fix issue with multiline properties. Change propWithTraili…

9f8f054

…ngTab to use \t instead of a binary tab; fails contrib-check. Move test to a class that runs under Windows.

ChrisSamo632 reviewed Mar 8, 2021

View reviewed changes

NIFI-7172: Move regex compilation out of the loop.

4dad03f

exceptionfactory reviewed Mar 18, 2021

View reviewed changes

NIFI-7172

fa09b86

Incorporate suggestions from exceptionfactory.

exceptionfactory approved these changes Mar 25, 2021

View reviewed changes

asfgit closed this in bff3e94 Mar 25, 2021

krisztina-zsihovszki pushed a commit to krisztina-zsihovszki/nifi that referenced this pull request Jun 28, 2022

NIFI-7172 Trim trailing whitespace from NiFi properties

8b7ed96

This closes apache#4854 Signed-off-by: David Handermann <exceptionfactory@apache.org>

Conversation

eaolson commented Feb 28, 2021

Description of PR

For all changes:

For code changes:

Uh oh!

ChrisSamo632 Mar 3, 2021

Choose a reason for hiding this comment

Uh oh!

eaolson Mar 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisSamo632 Mar 4, 2021

Choose a reason for hiding this comment

Uh oh!

ChrisSamo632 Mar 4, 2021

Choose a reason for hiding this comment

Uh oh!

eaolson Mar 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisSamo632 Mar 8, 2021

Choose a reason for hiding this comment

Uh oh!

ChrisSamo632 Mar 8, 2021

Choose a reason for hiding this comment

Uh oh!

eaolson Mar 10, 2021

Choose a reason for hiding this comment

Uh oh!

exceptionfactory left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

exceptionfactory commented Mar 24, 2021

Uh oh!

ChrisSamo632 commented Mar 24, 2021

Uh oh!

exceptionfactory commented Mar 25, 2021

Uh oh!

exceptionfactory left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

eaolson Mar 4, 2021 •

edited

Loading

eaolson Mar 7, 2021 •

edited

Loading