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 2022/05/20 04:34:02 UTC
[GitHub] [nifi] markobean opened a new pull request, #6061: NIFIDEVS-5378: prevent duplicate keys with different values in nifi.p…
markobean opened a new pull request, #6061:
URL: https://github.com/apache/nifi/pull/6061
…roperties
<!-- Licensed to the Apache Software Foundation (ASF) under one or more -->
<!-- contributor license agreements. See the NOTICE file distributed with -->
<!-- this work for additional information regarding copyright ownership. -->
<!-- The ASF licenses this file to You under the Apache License, Version 2.0 -->
<!-- (the "License"); you may not use this file except in compliance with -->
<!-- the License. You may obtain a copy of the License at -->
<!-- http://www.apache.org/licenses/LICENSE-2.0 -->
<!-- Unless required by applicable law or agreed to in writing, software -->
<!-- distributed under the License is distributed on an "AS IS" BASIS, -->
<!-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -->
<!-- See the License for the specific language governing permissions and -->
<!-- limitations under the License. -->
# Summary
When loading nifi.properties file, first check for duplicate key/value entries. Duplicate keys are allowed if their values are the same. If the same key has different values, an error with key/value details is written to nifi-app.log and NiFi will not start.
[NIFI-4278](https://issues.apache.org/jira/browse/NIFI-5378)
# Tracking
Please complete the following tracking steps prior to pull request creation.
### Issue Tracking
- [X] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue created
### Pull Request Tracking
- [X] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-00000`
- [X] Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-00000`
### Pull Request Formatting
- [X] Pull Request based on current revision of the `main` branch
- [X] Pull Request refers to a feature branch with one commit containing changes
# Verification
Installed NiFi and attempted to start with various combinations of key/value duplicates.
Duplicate keys with unique values
Duplicate keys with same values
All unique keys
### Build
- [X] Build completed using `mvn clean install -P contrib-check`
- [X] JDK 8
- [X] JDK 11
- [ ] JDK 17
### Licensing
- [ ] New dependencies are compatible with the [Apache License 2.0](https://apache.org/licenses/LICENSE-2.0) according to the [License Policy](https://www.apache.org/legal/resolved.html)
- [ ] New dependencies are documented in applicable `LICENSE` and `NOTICE` files
### Documentation
- [ ] Documentation formatting appears as expected in rendered files
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] NissimShiman commented on pull request #6061: NIFI-5378 prevent duplicate keys with different values in nifi.p…
Posted by GitBox <gi...@apache.org>.
NissimShiman commented on PR #6061:
URL: https://github.com/apache/nifi/pull/6061#issuecomment-1135017663
@markap14 Its true that additional properties values add complexity ... Just putting out ideas to try to see what could work
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markobean commented on a diff in pull request #6061: NIFI-5378 prevent duplicate keys with different values in nifi.p…
Posted by GitBox <gi...@apache.org>.
markobean commented on code in PR #6061:
URL: https://github.com/apache/nifi/pull/6061#discussion_r896952611
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/main/java/org/apache/nifi/properties/NiFiPropertiesLoader.java:
##########
@@ -289,4 +310,30 @@ private void setBootstrapKey(final SensitivePropertyProviderFactory providerFact
}
}
}
+
+ private static class Props extends Properties {
Review Comment:
Renamed to DuplicateDetectingProperties
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #6061: NIFI-5378 prevent duplicate keys with different values in nifi.p…
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on code in PR #6061:
URL: https://github.com/apache/nifi/pull/6061#discussion_r879742311
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/main/java/org/apache/nifi/properties/NiFiPropertiesLoader.java:
##########
@@ -211,6 +220,49 @@ public NiFiProperties get() {
return instance;
}
+ private void checkForDuplicates(File file) {
+ Map<String, String> properties = new HashMap<>();
+ Map<String, Set<String>> duplicateProperties = new HashMap<>();
+ Pattern keyPattern = Pattern.compile("([^!#=][^=]*)=(.*)");
+
+ if (file == null) {
+ throw new IllegalArgumentException("NiFi properties file missing or unreadable");
+ }
+ // Scan the properties file for duplicate keys
+ try (BufferedReader bufferedReader = new BufferedReader(new FileReader(file))) {
+ String line;
+ while ((line = bufferedReader.readLine()) != null) {
+ Matcher matcher = keyPattern.matcher(line);
+
+ if (matcher.matches()) {
+ String key = matcher.group(1);
+ String value = matcher.group(2);
+
+ String existingValue = properties.put(key, value);
+
+ if (existingValue != null) {
+ if (existingValue.equals(value)) {
+ logger.warn("Duplicate keys found for key '{}', but values are the same.", key);
+ } else {
+ Set<String> dupes = duplicateProperties.computeIfAbsent(key, k -> new HashSet<>(Collections.singleton(existingValue)));
+ dupes.add(value);
+ }
+ }
+ }
+ }
+ } catch (IOException ioe) {
+ throw new IllegalArgumentException("Cannot load properties file [" + file.getAbsolutePath() + "] due to "
+ + ioe.getLocalizedMessage(), ioe);
+ }
+
+ if (!duplicateProperties.isEmpty()) {
+ duplicateProperties.keySet().forEach(
+ k -> logger.error("Duplicate entries found for key '{}'. Conflicting values are: {}", k, duplicateProperties.get(k))
Review Comment:
@markobean One other very important thing to note is that these property values should never be logged. Various values in `nifi.properties` are sensitive, such as keystore passwords. In addition to the parsing changes noted, property values should not be logged or included in error messages.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markobean commented on pull request #6061: NIFI-5378 prevent duplicate keys with different values in nifi.p…
Posted by GitBox <gi...@apache.org>.
markobean commented on PR #6061:
URL: https://github.com/apache/nifi/pull/6061#issuecomment-1138594164
> > @exceptionfactory The PR indicates you still have on outstanding requested change. The referenced comments discusses both `IllegalArgumentException` and overriding the `put()` method for a custom Properties class that will handles the duplicates like we want. I have addressed the latter.
> > Are you still anticipating a change to the exception? The exception will in fact prevent NiFi from starting, but that is as intended. The discussion with @markap14 describes the rationale in more detail.
>
> Thanks for making the updates @markobean, the requested change status remains as I have not yet had chance to review the updates. I will take a closer look soon, but the recent changes look good from a cursory review.
Roger that. I just wanted to be sure I knew who's court the ball was in
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markobean commented on a diff in pull request #6061: NIFI-5378 prevent duplicate keys with different values in nifi.p…
Posted by GitBox <gi...@apache.org>.
markobean commented on code in PR #6061:
URL: https://github.com/apache/nifi/pull/6061#discussion_r896953148
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/main/java/org/apache/nifi/properties/NiFiPropertiesLoader.java:
##########
@@ -289,4 +310,30 @@ private void setBootstrapKey(final SensitivePropertyProviderFactory providerFact
}
}
}
+
+ private static class Props extends Properties {
+ // Only need to retain Properties key. This will help prevent possible inadvertent exposure of sensitive Properties value
+ private final Set<String> duplicateKeys = new HashSet<>();
+ public Props() {
+ super();
+ }
+
+ public Set<String> duplicateKeySet() {
+ return duplicateKeys;
+ }
+
+ @Override
+ public Object put(Object key, Object value) {
+ Object existingValue = super.put(key, value);
+ if (existingValue != null) {
+ if (existingValue.toString().equals(value.toString())) {
+ logger.warn("Duplicate keys found for key '{}', but values are the same.", key);
Review Comment:
Wording updated
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markobean commented on a diff in pull request #6061: NIFI-5378 prevent duplicate keys with different values in nifi.p…
Posted by GitBox <gi...@apache.org>.
markobean commented on code in PR #6061:
URL: https://github.com/apache/nifi/pull/6061#discussion_r878099667
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/main/java/org/apache/nifi/properties/NiFiPropertiesLoader.java:
##########
@@ -211,6 +220,45 @@ public NiFiProperties get() {
return instance;
}
+ private void checkForDuplicates(File file) {
+ Map<String, String> properties = new HashMap<>();
+ Map<String, Set<String>> duplicateProperties = new HashMap<>();
+ Pattern keyPattern = Pattern.compile("([^!#=][^=]*)=(.*)");
+
+ if (file == null) {
+ throw new IllegalArgumentException("NiFi properties file missing or unreadable");
+ }
+ // Scan the properties file for duplicate keys
+ try (BufferedReader bufferedReader = new BufferedReader(new FileReader(file))) {
+ String line;
+ while ((line = bufferedReader.readLine()) != null) {
+ Matcher matcher = keyPattern.matcher(line);
+
+ if (matcher.matches()) {
+ String key = matcher.group(1);
+ String value = matcher.group(2);
+
+ String existingValue = properties.put(key, value);
+
+ if (existingValue != null && !existingValue.equals(value)) {
+ Set<String> dupes = duplicateProperties.computeIfAbsent(key, k -> new HashSet<>(Collections.singleton(existingValue)));
+ dupes.add(value);
+ }
+ }
+ }
+ } catch (IOException ioe) {
+ throw new IllegalArgumentException("Cannot load properties file [" + file.getAbsolutePath() + "] due to "
+ + ioe.getLocalizedMessage(), ioe);
+ }
+
+ if (!duplicateProperties.isEmpty()) {
+ duplicateProperties.keySet().forEach(
+ k -> logger.error("Duplicate values found for key '{}': {}", k, duplicateProperties.get(k))
+ );
+ throw new IllegalArgumentException("Duplicate keys were detected in the properties file. See previous errors.");
Review Comment:
This is not really a concern. NiFi property loading is just about the first thing to happen. None of the other chatty log messages are entered. The following is an example of the entire nifi-app.log when a duplicate property is found in nifi.properties - and specifically this example duplicated the last property in the file:
2022-05-20 08:34:43,428 INFO [main] org.apache.nifi.NiFi Launching NiFi...
2022-05-20 08:34:43,465 ERROR [main] o.a.nifi.properties.NiFiPropertiesLoader Duplicate values found for key 'nifi.diagnostics.on.shutdown.max.directory.size': [20 MB, 10 MB]
2022-05-20 08:34:43,469 ERROR [main] org.apache.nifi.NiFi Failure to launch NiFi
java.lang.IllegalArgumentException: There was an issue decrypting protected properties
at org.apache.nifi.NiFi.initializeProperties(NiFi.java:373)
at org.apache.nifi.NiFi.convertArgumentsToValidatedNiFiProperties(NiFi.java:341)
at org.apache.nifi.NiFi.convertArgumentsToValidatedNiFiProperties(NiFi.java:337)
at org.apache.nifi.NiFi.main(NiFi.java:329)
Caused by: java.lang.IllegalArgumentException: Duplicate keys were detected in the properties file. See previous errors.
at org.apache.nifi.properties.NiFiPropertiesLoader.checkForDuplicates(NiFiPropertiesLoader.java:258)
at org.apache.nifi.properties.NiFiPropertiesLoader.load(NiFiPropertiesLoader.java:159)
at org.apache.nifi.properties.NiFiPropertiesLoader.load(NiFiPropertiesLoader.java:199)
at org.apache.nifi.properties.NiFiPropertiesLoader.loadDefault(NiFiPropertiesLoader.java:263)
at org.apache.nifi.properties.NiFiPropertiesLoader.getDefaultProperties(NiFiPropertiesLoader.java:267)
at org.apache.nifi.properties.NiFiPropertiesLoader.get(NiFiPropertiesLoader.java:217)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.apache.nifi.NiFi.initializeProperties(NiFi.java:368)
... 3 common frames omitted
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markobean commented on pull request #6061: NIFI-5378 prevent duplicate keys with different values in nifi.p…
Posted by GitBox <gi...@apache.org>.
markobean commented on PR #6061:
URL: https://github.com/apache/nifi/pull/6061#issuecomment-1138073721
@exceptionfactory The PR indicates you still have on outstanding requested change. The referenced comments discusses both `IllegalArgumentException` and overriding the `put()` method for a custom Properties class that will handles the duplicates like we want. I have addressed the latter.
Are you still anticipating a change to the exception? The exception will in fact prevent NiFi from starting, but that is as intended. The discussion with @markap14 describes the rationale in more detail.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markobean commented on a diff in pull request #6061: NIFI-5378 prevent duplicate keys with different values in nifi.p…
Posted by GitBox <gi...@apache.org>.
markobean commented on code in PR #6061:
URL: https://github.com/apache/nifi/pull/6061#discussion_r896951159
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/main/java/org/apache/nifi/properties/NiFiPropertiesLoader.java:
##########
@@ -211,6 +213,25 @@ public NiFiProperties get() {
return instance;
}
+ private void checkForDuplicates(File file) {
+ if (file == null || !file.exists() || !file.canRead()) {
+ throw new IllegalArgumentException("NiFi properties file missing or unreadable");
+ }
+
+ Props properties = new Props();
+ try (final InputStream inputStream = new BufferedInputStream(new FileInputStream(file))) {
+ properties.load(inputStream);
+ } catch (final Exception e) {
+ throw new RuntimeException(String.format("Loading Application Properties [%s] failed", file), e);
+ }
+
+ Set<String> duplicateKeys = properties.duplicateKeySet();
+ if (!duplicateKeys.isEmpty()) {
Review Comment:
Updated the location of the check to avoid duplicate loading/parsing of properties file.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on pull request #6061: NIFI-5378 prevent duplicate keys with different values in nifi.p…
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on PR #6061:
URL: https://github.com/apache/nifi/pull/6061#issuecomment-1135084798
> @exceptionfactory I get your point about how properties are loaded with the increased flexibility naturally built into the Properties class. The problem with `Properties.load()` is it automatically turns the property keys into a Set which makes it impossible to find duplicates. The only way I thought of to do this was standard FileReader and going line by line. As far as I know, multi-line properties are not supported (and a test case putting a property in a new line verified that behavior.)
>
The OpenJDK implementation of `java.util.Properties` calls the public `put` method after reading the property and value, so the `put` method could be overridden to check and store duplicate keys, avoiding the need for custom parsing.
https://github.com/openjdk/jdk/blob/f77a658557be193e2deb5cbd5c69e561bb88524b/src/java.base/share/classes/java/util/Properties.java#L457
The standard implementation supports multiline property values using a continuation character as follows:
```
nifi.ui.banner.text=Banner \
Extended
```
> I'm not sure the characterset issues you mention are necessarily relevant in the check for duplicates. As long as the property values are being read the same way regardless of UTF-8, ISO-8859-1, etc. then the String compare would still be accurate for match/not match. Please correct me if you still think this is an issue.
>
That's a good point about the character set when it comes to comparing values. Although the property value might not match the parsed value, it should still be possible to compare the same byte string.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] michael81877 commented on a diff in pull request #6061: NIFIDEVS-5378: prevent duplicate keys with different values in nifi.p…
Posted by GitBox <gi...@apache.org>.
michael81877 commented on code in PR #6061:
URL: https://github.com/apache/nifi/pull/6061#discussion_r877815352
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/main/java/org/apache/nifi/properties/NiFiPropertiesLoader.java:
##########
@@ -211,6 +220,45 @@ public NiFiProperties get() {
return instance;
}
+ private void checkForDuplicates(File file) {
+ Map<String, String> properties = new HashMap<>();
+ Map<String, Set<String>> duplicateProperties = new HashMap<>();
+ Pattern keyPattern = Pattern.compile("([^!#=][^=]*)=(.*)");
+
+ if (file == null) {
+ throw new IllegalArgumentException("NiFi properties file missing or unreadable");
+ }
+ // Scan the properties file for duplicate keys
+ try (BufferedReader bufferedReader = new BufferedReader(new FileReader(file))) {
+ String line;
+ while ((line = bufferedReader.readLine()) != null) {
+ Matcher matcher = keyPattern.matcher(line);
+
+ if (matcher.matches()) {
+ String key = matcher.group(1);
+ String value = matcher.group(2);
+
+ String existingValue = properties.put(key, value);
+
+ if (existingValue != null && !existingValue.equals(value)) {
Review Comment:
Did you want to log a warning when there are duplicates but the values are the same? I believe this was noted as an exceptional case and the startup should continue.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] NissimShiman commented on pull request #6061: NIFI-5378 prevent duplicate keys with different values in nifi.p…
Posted by GitBox <gi...@apache.org>.
NissimShiman commented on PR #6061:
URL: https://github.com/apache/nifi/pull/6061#issuecomment-1134902560
Tested with duplicate nifi.web.https.host properties and failed on startup within a 1/10 of a second with clear log about which duplicate key was found.
For FWIW I like this idea as I've had situations where I had unintended duplicates in nifi.properties and didn't notice until later, but I hear the concerns and wouldn't want current users to have unintended consequences.
Maybe another property can be added,
nifi.startup.properties.verify.unique
which by default will be false, but can be set to true
This property won't even be in the nifi.properties by default, just in the documentation
for those interested in it.
Log with message details is in nifi-app as opposed to nifi-bootstrap so maybe this could be in nifi-bootstrap as well although it seems most startup issues tend to have the details logged in nifi-app only, so this follows that pattern, so I believe this is fine as well.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markobean commented on a diff in pull request #6061: NIFI-5378 prevent duplicate keys with different values in nifi.p…
Posted by GitBox <gi...@apache.org>.
markobean commented on code in PR #6061:
URL: https://github.com/apache/nifi/pull/6061#discussion_r879774421
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/main/java/org/apache/nifi/properties/NiFiPropertiesLoader.java:
##########
@@ -211,6 +220,49 @@ public NiFiProperties get() {
return instance;
}
+ private void checkForDuplicates(File file) {
+ Map<String, String> properties = new HashMap<>();
+ Map<String, Set<String>> duplicateProperties = new HashMap<>();
+ Pattern keyPattern = Pattern.compile("([^!#=][^=]*)=(.*)");
+
+ if (file == null) {
+ throw new IllegalArgumentException("NiFi properties file missing or unreadable");
+ }
+ // Scan the properties file for duplicate keys
+ try (BufferedReader bufferedReader = new BufferedReader(new FileReader(file))) {
+ String line;
+ while ((line = bufferedReader.readLine()) != null) {
+ Matcher matcher = keyPattern.matcher(line);
+
+ if (matcher.matches()) {
+ String key = matcher.group(1);
+ String value = matcher.group(2);
+
+ String existingValue = properties.put(key, value);
+
+ if (existingValue != null) {
+ if (existingValue.equals(value)) {
+ logger.warn("Duplicate keys found for key '{}', but values are the same.", key);
+ } else {
+ Set<String> dupes = duplicateProperties.computeIfAbsent(key, k -> new HashSet<>(Collections.singleton(existingValue)));
+ dupes.add(value);
+ }
+ }
+ }
+ }
+ } catch (IOException ioe) {
+ throw new IllegalArgumentException("Cannot load properties file [" + file.getAbsolutePath() + "] due to "
+ + ioe.getLocalizedMessage(), ioe);
+ }
+
+ if (!duplicateProperties.isEmpty()) {
+ duplicateProperties.keySet().forEach(
+ k -> logger.error("Duplicate entries found for key '{}'. Conflicting values are: {}", k, duplicateProperties.get(k))
Review Comment:
Removed. Pushed change.
Also, noted I was only checking for File being null. I added `|| !file.exists() || !file.canRead()` for completeness.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markobean commented on pull request #6061: NIFI-5378 prevent duplicate keys with different values in nifi.p…
Posted by GitBox <gi...@apache.org>.
markobean commented on PR #6061:
URL: https://github.com/apache/nifi/pull/6061#issuecomment-1137264022
@exceptionfactory I pushed an update. This will handle the multiple line properties better. As you recommended, I overrode the `put<Object, Object>` method to handle the duplicate property detection. Thanks for the suggestion. This makes the comparison more robust.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markap14 commented on pull request #6061: NIFI-5378 prevent duplicate keys with different values in nifi.p…
Posted by GitBox <gi...@apache.org>.
markap14 commented on PR #6061:
URL: https://github.com/apache/nifi/pull/6061#issuecomment-1134789559
I'm a little bit hesitant on this. I think adding a WARN log message makes sense. But if I'm reading this correctly, this would prevent NiFi from starting in the case of duplicate properties? Do we have a reason that we need to do this?
With .properties files, if there are duplicates, the last entry in the file wins. And because of this it's pretty common to see scripts such as:
```
echo "nifi.my.property.value=My-Value" >> nifi.properties
echo "nifi.your.property.value=Your-Value" >> nifi.properties
```
In doing this, we append the line to the end of nifi.properties, and this can be done to set the properties as we want. The alternative would be to use sed or something like that, but that can be more complicated.
To be clear here, I'm not saying that I'm a -1 and I think we should avoid this. But we should be careful here and realize that introducing this could break some people, so we need to make sure that introducing this new restriction provide a benefit that is helpful enough to outweigh the potential downside.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markobean commented on pull request #6061: NIFI-5378 prevent duplicate keys with different values in nifi.p…
Posted by GitBox <gi...@apache.org>.
markobean commented on PR #6061:
URL: https://github.com/apache/nifi/pull/6061#issuecomment-1134861610
@markap14 It seems to me it is a high risk to allow NiFi to start with duplicate properties (with different values.) You've presented one use case where users simply append updated properties at the end of the file. This approach is susceptible to unnoticed configuration errors. Different values represent an ambiguous configuration. A user not in the know will not even be certain if the last property takes precedence or the first - or is it random? The documentation makes no mention of this. Therefore, we are in the gray area requiring the user to assume - never a good position to be in.
That is largely the point of this ticket: to ensure a well-defined, unambiguous state of the application. I don't find that prospect overly limiting nor ill advised.
The use case which brought about this ticket is a user (or maybe it was an automated script.. it's been lost to history a bit) updated nifi.properties in a similar way as you mentioned. However, operators looked at nifi.properties, and specifically looked at the area of the file that is commented and documented to hold certain properties, e.g. "# security properties #". Those values were correct, but a new property was added towards the end of the file and was unnoticed.
So, we seem to be discussing the valid - or advisable - structure of the nifi.properties file, and the appropriate response of the application. Through this ticket, I would argue that it is not recommended to allow the properties file such wide latitude of property loading. There is very little downside to being more strict, yet potentially costly results for being more lax. I think it is perfectly reasonable to disallow start with the reason being logged. (In fact, failure happens so quickly that the log file is extremely short making it immediately obvious what the problem was.)
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #6061: NIFI-5378 prevent duplicate keys with different values in nifi.p…
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on code in PR #6061:
URL: https://github.com/apache/nifi/pull/6061#discussion_r885698687
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/main/java/org/apache/nifi/properties/NiFiPropertiesLoader.java:
##########
@@ -289,4 +310,30 @@ private void setBootstrapKey(final SensitivePropertyProviderFactory providerFact
}
}
}
+
+ private static class Props extends Properties {
+ // Only need to retain Properties key. This will help prevent possible inadvertent exposure of sensitive Properties value
+ private final Set<String> duplicateKeys = new HashSet<>();
+ public Props() {
+ super();
+ }
+
+ public Set<String> duplicateKeySet() {
+ return duplicateKeys;
+ }
+
+ @Override
+ public Object put(Object key, Object value) {
+ Object existingValue = super.put(key, value);
+ if (existingValue != null) {
+ if (existingValue.toString().equals(value.toString())) {
+ logger.warn("Duplicate keys found for key '{}', but values are the same.", key);
Review Comment:
Recommend adjusting the message to remove the punctuation for easier reading:
```suggestion
logger.warn("Duplicate property key found [{}]", key);
```
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/main/java/org/apache/nifi/properties/NiFiPropertiesLoader.java:
##########
@@ -289,4 +310,30 @@ private void setBootstrapKey(final SensitivePropertyProviderFactory providerFact
}
}
}
+
+ private static class Props extends Properties {
Review Comment:
Recommend adjusting the class name to something like `ProcessedProperties` or `DuplicateDetectingProperties`.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/main/java/org/apache/nifi/properties/NiFiPropertiesLoader.java:
##########
@@ -211,6 +213,25 @@ public NiFiProperties get() {
return instance;
}
+ private void checkForDuplicates(File file) {
+ if (file == null || !file.exists() || !file.canRead()) {
+ throw new IllegalArgumentException("NiFi properties file missing or unreadable");
+ }
+
+ Props properties = new Props();
+ try (final InputStream inputStream = new BufferedInputStream(new FileInputStream(file))) {
+ properties.load(inputStream);
+ } catch (final Exception e) {
+ throw new RuntimeException(String.format("Loading Application Properties [%s] failed", file), e);
+ }
+
+ Set<String> duplicateKeys = properties.duplicateKeySet();
+ if (!duplicateKeys.isEmpty()) {
Review Comment:
With the change to use an extension of `java.util.Properties`, recommend relocating this check into `loadProtectProperties()`. That would avoid the need to load the file twice, and also remove the duplicate file existence checking.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/main/java/org/apache/nifi/properties/NiFiPropertiesLoader.java:
##########
@@ -211,6 +213,25 @@ public NiFiProperties get() {
return instance;
}
+ private void checkForDuplicates(File file) {
+ if (file == null || !file.exists() || !file.canRead()) {
+ throw new IllegalArgumentException("NiFi properties file missing or unreadable");
+ }
+
+ Props properties = new Props();
+ try (final InputStream inputStream = new BufferedInputStream(new FileInputStream(file))) {
+ properties.load(inputStream);
+ } catch (final Exception e) {
+ throw new RuntimeException(String.format("Loading Application Properties [%s] failed", file), e);
+ }
+
+ Set<String> duplicateKeys = properties.duplicateKeySet();
+ if (!duplicateKeys.isEmpty()) {
+ duplicateKeys.forEach(v -> logger.error("Multiple entries with different values found for key '{}'.", v));
+ throw new IllegalArgumentException("Duplicate keys were detected in the properties file. See previous errors.");
Review Comment:
Now that error handling includes only property keys, recommend aggregating the error into the exception message, as opposed to multiple log statements:
```suggestion
throw new IllegalArgumentException(String.format("Application Properties contains duplicate property names [%s]", String.join(",", duplicateKeys));
```
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markobean commented on a diff in pull request #6061: NIFI-5378 prevent duplicate keys with different values in nifi.p…
Posted by GitBox <gi...@apache.org>.
markobean commented on code in PR #6061:
URL: https://github.com/apache/nifi/pull/6061#discussion_r878121366
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/main/java/org/apache/nifi/properties/NiFiPropertiesLoader.java:
##########
@@ -211,6 +220,45 @@ public NiFiProperties get() {
return instance;
}
+ private void checkForDuplicates(File file) {
+ Map<String, String> properties = new HashMap<>();
+ Map<String, Set<String>> duplicateProperties = new HashMap<>();
+ Pattern keyPattern = Pattern.compile("([^!#=][^=]*)=(.*)");
+
+ if (file == null) {
+ throw new IllegalArgumentException("NiFi properties file missing or unreadable");
+ }
+ // Scan the properties file for duplicate keys
+ try (BufferedReader bufferedReader = new BufferedReader(new FileReader(file))) {
+ String line;
+ while ((line = bufferedReader.readLine()) != null) {
+ Matcher matcher = keyPattern.matcher(line);
+
+ if (matcher.matches()) {
+ String key = matcher.group(1);
+ String value = matcher.group(2);
+
+ String existingValue = properties.put(key, value);
+
+ if (existingValue != null && !existingValue.equals(value)) {
Review Comment:
I considered this 'no harm, no foul' as the application is not affected by duplicate keys with the same value. However, I agree that a warning level message might be a good idea to help users (who are paying attention) keep their nifi.properties file clean.
I added a warning log message in cases of duplicate keys with the same value.
Full disclosure: I also changed the wording of the error message when values are not the same.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] michael81877 commented on a diff in pull request #6061: NIFIDEVS-5378: prevent duplicate keys with different values in nifi.p…
Posted by GitBox <gi...@apache.org>.
michael81877 commented on code in PR #6061:
URL: https://github.com/apache/nifi/pull/6061#discussion_r877815352
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/main/java/org/apache/nifi/properties/NiFiPropertiesLoader.java:
##########
@@ -211,6 +220,45 @@ public NiFiProperties get() {
return instance;
}
+ private void checkForDuplicates(File file) {
+ Map<String, String> properties = new HashMap<>();
+ Map<String, Set<String>> duplicateProperties = new HashMap<>();
+ Pattern keyPattern = Pattern.compile("([^!#=][^=]*)=(.*)");
+
+ if (file == null) {
+ throw new IllegalArgumentException("NiFi properties file missing or unreadable");
+ }
+ // Scan the properties file for duplicate keys
+ try (BufferedReader bufferedReader = new BufferedReader(new FileReader(file))) {
+ String line;
+ while ((line = bufferedReader.readLine()) != null) {
+ Matcher matcher = keyPattern.matcher(line);
+
+ if (matcher.matches()) {
+ String key = matcher.group(1);
+ String value = matcher.group(2);
+
+ String existingValue = properties.put(key, value);
+
+ if (existingValue != null && !existingValue.equals(value)) {
Review Comment:
Did you want to log a warning when there are duplicates but the values are the same? I believe this was noted as a case where the startup should continue.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markobean commented on pull request #6061: NIFI-5378 prevent duplicate keys with different values in nifi.p…
Posted by GitBox <gi...@apache.org>.
markobean commented on PR #6061:
URL: https://github.com/apache/nifi/pull/6061#issuecomment-1135011789
@exceptionfactory I get your point about how properties are loaded with the increased flexibility naturally built into the Properties class. The problem with `Properties.load()` is it automatically turns the property keys into a Set which makes it impossible to find duplicates. The only way I thought of to do this was standard FileReader and going line by line. As far as I know, multi-line properties are not supported (and a test case putting a property in a new line verified that behavior.)
I'm not sure the characterset issues you mention are necessarily relevant in the check for duplicates. As long as the property values are being read the same way regardless of UTF-8, ISO-8859-1, etc. then the String compare would still be accurate for match/not match. Please correct me if you still think this is an issue.
Note that the duplicate check is performed separately from reading/storing actual property values. It is literally and solely a key/value pair per line sanity check.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markobean commented on pull request #6061: NIFI-5378 prevent duplicate keys with different values in nifi.p…
Posted by GitBox <gi...@apache.org>.
markobean commented on PR #6061:
URL: https://github.com/apache/nifi/pull/6061#issuecomment-1135042179
> @NissimShiman I think I would avoid added yet another property, just to control how we load properties. When adding properties, there's generally a balance, where adding more properties = more flexibility but more complexity. Fewer properties = less flexible but simpler.
>
> @markobean "Through this ticket, I would argue that it is not recommended to allow the properties file such wide latitude of property loading. There is very little downside to being more strict, yet potentially costly results for being more lax." I think there are arguments to be made for each side. Personally, I don't really have a strong preference one way or the other. Just wanted to make sure that these things were being thought through.
@markap14 I get it. I do in fact see both sides. I believe it makes NiFi a stronger, more reliable application to follow best practices of failing safely.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markap14 commented on pull request #6061: NIFI-5378 prevent duplicate keys with different values in nifi.p…
Posted by GitBox <gi...@apache.org>.
markap14 commented on PR #6061:
URL: https://github.com/apache/nifi/pull/6061#issuecomment-1134968841
@NissimShiman I think I would avoid added yet another property, just to control how we load properties. When adding properties, there's generally a balance, where adding more properties = more flexibility but more complexity. Fewer properties = less flexible but simpler.
@markobean "Through this ticket, I would argue that it is not recommended to allow the properties file such wide latitude of property loading. There is very little downside to being more strict, yet potentially costly results for being more lax."
I think there are arguments to be made for each side.
Personally, I don't really have a strong preference one way or the other. Just wanted to make sure that these things were being thought through.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on pull request #6061: NIFI-5378 prevent duplicate keys with different values in nifi.p…
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on PR #6061:
URL: https://github.com/apache/nifi/pull/6061#issuecomment-1138109296
> @exceptionfactory The PR indicates you still have on outstanding requested change. The referenced comments discusses both `IllegalArgumentException` and overriding the `put()` method for a custom Properties class that will handles the duplicates like we want. I have addressed the latter.
>
> Are you still anticipating a change to the exception? The exception will in fact prevent NiFi from starting, but that is as intended. The discussion with @markap14 describes the rationale in more detail.
Thanks for making the updates @markobean, the requested change status remains as I have not yet had chance to review the updates. I will take a closer look soon, but the recent changes look good from a cursory review.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] michael81877 commented on a diff in pull request #6061: NIFIDEVS-5378: prevent duplicate keys with different values in nifi.p…
Posted by GitBox <gi...@apache.org>.
michael81877 commented on code in PR #6061:
URL: https://github.com/apache/nifi/pull/6061#discussion_r877815352
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/main/java/org/apache/nifi/properties/NiFiPropertiesLoader.java:
##########
@@ -211,6 +220,45 @@ public NiFiProperties get() {
return instance;
}
+ private void checkForDuplicates(File file) {
+ Map<String, String> properties = new HashMap<>();
+ Map<String, Set<String>> duplicateProperties = new HashMap<>();
+ Pattern keyPattern = Pattern.compile("([^!#=][^=]*)=(.*)");
+
+ if (file == null) {
+ throw new IllegalArgumentException("NiFi properties file missing or unreadable");
+ }
+ // Scan the properties file for duplicate keys
+ try (BufferedReader bufferedReader = new BufferedReader(new FileReader(file))) {
+ String line;
+ while ((line = bufferedReader.readLine()) != null) {
+ Matcher matcher = keyPattern.matcher(line);
+
+ if (matcher.matches()) {
+ String key = matcher.group(1);
+ String value = matcher.group(2);
+
+ String existingValue = properties.put(key, value);
+
+ if (existingValue != null && !existingValue.equals(value)) {
Review Comment:
Did you want to log a warning when there are duplicates but the values are the same? I believe this wasn't noted as an exceptional case and the startup should continue.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markobean commented on a diff in pull request #6061: NIFI-5378 prevent duplicate keys with different values in nifi.p…
Posted by GitBox <gi...@apache.org>.
markobean commented on code in PR #6061:
URL: https://github.com/apache/nifi/pull/6061#discussion_r896951762
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/main/java/org/apache/nifi/properties/NiFiPropertiesLoader.java:
##########
@@ -211,6 +213,25 @@ public NiFiProperties get() {
return instance;
}
+ private void checkForDuplicates(File file) {
+ if (file == null || !file.exists() || !file.canRead()) {
+ throw new IllegalArgumentException("NiFi properties file missing or unreadable");
+ }
+
+ Props properties = new Props();
+ try (final InputStream inputStream = new BufferedInputStream(new FileInputStream(file))) {
+ properties.load(inputStream);
+ } catch (final Exception e) {
+ throw new RuntimeException(String.format("Loading Application Properties [%s] failed", file), e);
+ }
+
+ Set<String> duplicateKeys = properties.duplicateKeySet();
+ if (!duplicateKeys.isEmpty()) {
+ duplicateKeys.forEach(v -> logger.error("Multiple entries with different values found for key '{}'.", v));
+ throw new IllegalArgumentException("Duplicate keys were detected in the properties file. See previous errors.");
Review Comment:
Log messages have been aggregated.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] michael81877 commented on a diff in pull request #6061: NIFIDEVS-5378: prevent duplicate keys with different values in nifi.p…
Posted by GitBox <gi...@apache.org>.
michael81877 commented on code in PR #6061:
URL: https://github.com/apache/nifi/pull/6061#discussion_r877812454
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/main/java/org/apache/nifi/properties/NiFiPropertiesLoader.java:
##########
@@ -211,6 +220,45 @@ public NiFiProperties get() {
return instance;
}
+ private void checkForDuplicates(File file) {
+ Map<String, String> properties = new HashMap<>();
+ Map<String, Set<String>> duplicateProperties = new HashMap<>();
+ Pattern keyPattern = Pattern.compile("([^!#=][^=]*)=(.*)");
+
+ if (file == null) {
+ throw new IllegalArgumentException("NiFi properties file missing or unreadable");
+ }
+ // Scan the properties file for duplicate keys
+ try (BufferedReader bufferedReader = new BufferedReader(new FileReader(file))) {
+ String line;
+ while ((line = bufferedReader.readLine()) != null) {
+ Matcher matcher = keyPattern.matcher(line);
+
+ if (matcher.matches()) {
+ String key = matcher.group(1);
+ String value = matcher.group(2);
+
+ String existingValue = properties.put(key, value);
+
+ if (existingValue != null && !existingValue.equals(value)) {
+ Set<String> dupes = duplicateProperties.computeIfAbsent(key, k -> new HashSet<>(Collections.singleton(existingValue)));
+ dupes.add(value);
+ }
+ }
+ }
+ } catch (IOException ioe) {
+ throw new IllegalArgumentException("Cannot load properties file [" + file.getAbsolutePath() + "] due to "
+ + ioe.getLocalizedMessage(), ioe);
+ }
+
+ if (!duplicateProperties.isEmpty()) {
+ duplicateProperties.keySet().forEach(
+ k -> logger.error("Duplicate values found for key '{}': {}", k, duplicateProperties.get(k))
+ );
+ throw new IllegalArgumentException("Duplicate keys were detected in the properties file. See previous errors.");
Review Comment:
NiFI can be pretty chatty at startup. Do you think the error message can be aggregated and logged all at once (cause + reasons) so the user doesn't need to sift through logs to find the problem?
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] michael81877 commented on a diff in pull request #6061: NIFIDEVS-5378: prevent duplicate keys with different values in nifi.p…
Posted by GitBox <gi...@apache.org>.
michael81877 commented on code in PR #6061:
URL: https://github.com/apache/nifi/pull/6061#discussion_r877812454
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/main/java/org/apache/nifi/properties/NiFiPropertiesLoader.java:
##########
@@ -211,6 +220,45 @@ public NiFiProperties get() {
return instance;
}
+ private void checkForDuplicates(File file) {
+ Map<String, String> properties = new HashMap<>();
+ Map<String, Set<String>> duplicateProperties = new HashMap<>();
+ Pattern keyPattern = Pattern.compile("([^!#=][^=]*)=(.*)");
+
+ if (file == null) {
+ throw new IllegalArgumentException("NiFi properties file missing or unreadable");
+ }
+ // Scan the properties file for duplicate keys
+ try (BufferedReader bufferedReader = new BufferedReader(new FileReader(file))) {
+ String line;
+ while ((line = bufferedReader.readLine()) != null) {
+ Matcher matcher = keyPattern.matcher(line);
+
+ if (matcher.matches()) {
+ String key = matcher.group(1);
+ String value = matcher.group(2);
+
+ String existingValue = properties.put(key, value);
+
+ if (existingValue != null && !existingValue.equals(value)) {
+ Set<String> dupes = duplicateProperties.computeIfAbsent(key, k -> new HashSet<>(Collections.singleton(existingValue)));
+ dupes.add(value);
+ }
+ }
+ }
+ } catch (IOException ioe) {
+ throw new IllegalArgumentException("Cannot load properties file [" + file.getAbsolutePath() + "] due to "
+ + ioe.getLocalizedMessage(), ioe);
+ }
+
+ if (!duplicateProperties.isEmpty()) {
+ duplicateProperties.keySet().forEach(
+ k -> logger.error("Duplicate values found for key '{}': {}", k, duplicateProperties.get(k))
+ );
+ throw new IllegalArgumentException("Duplicate keys were detected in the properties file. See previous errors.");
Review Comment:
NiFI can be pretty chatty at startup. Do you think the error message aggregated and logged all at once so the user doesn't need to sift through logs to find the problem?
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markobean commented on a diff in pull request #6061: NIFI-5378 prevent duplicate keys with different values in nifi.p…
Posted by GitBox <gi...@apache.org>.
markobean commented on code in PR #6061:
URL: https://github.com/apache/nifi/pull/6061#discussion_r879756966
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/main/java/org/apache/nifi/properties/NiFiPropertiesLoader.java:
##########
@@ -211,6 +220,49 @@ public NiFiProperties get() {
return instance;
}
+ private void checkForDuplicates(File file) {
+ Map<String, String> properties = new HashMap<>();
+ Map<String, Set<String>> duplicateProperties = new HashMap<>();
+ Pattern keyPattern = Pattern.compile("([^!#=][^=]*)=(.*)");
+
+ if (file == null) {
+ throw new IllegalArgumentException("NiFi properties file missing or unreadable");
+ }
+ // Scan the properties file for duplicate keys
+ try (BufferedReader bufferedReader = new BufferedReader(new FileReader(file))) {
+ String line;
+ while ((line = bufferedReader.readLine()) != null) {
+ Matcher matcher = keyPattern.matcher(line);
+
+ if (matcher.matches()) {
+ String key = matcher.group(1);
+ String value = matcher.group(2);
+
+ String existingValue = properties.put(key, value);
+
+ if (existingValue != null) {
+ if (existingValue.equals(value)) {
+ logger.warn("Duplicate keys found for key '{}', but values are the same.", key);
+ } else {
+ Set<String> dupes = duplicateProperties.computeIfAbsent(key, k -> new HashSet<>(Collections.singleton(existingValue)));
+ dupes.add(value);
+ }
+ }
+ }
+ }
+ } catch (IOException ioe) {
+ throw new IllegalArgumentException("Cannot load properties file [" + file.getAbsolutePath() + "] due to "
+ + ioe.getLocalizedMessage(), ioe);
+ }
+
+ if (!duplicateProperties.isEmpty()) {
+ duplicateProperties.keySet().forEach(
+ k -> logger.error("Duplicate entries found for key '{}'. Conflicting values are: {}", k, duplicateProperties.get(k))
Review Comment:
Good point! I will remove logging of property values.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] NissimShiman commented on a diff in pull request #6061: NIFI-5378 prevent duplicate keys with different values in nifi.p…
Posted by GitBox <gi...@apache.org>.
NissimShiman commented on code in PR #6061:
URL: https://github.com/apache/nifi/pull/6061#discussion_r879729545
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-properties-loader/src/main/java/org/apache/nifi/properties/NiFiPropertiesLoader.java:
##########
@@ -211,6 +220,49 @@ public NiFiProperties get() {
return instance;
}
+ private void checkForDuplicates(File file) {
+ Map<String, String> properties = new HashMap<>();
+ Map<String, Set<String>> duplicateProperties = new HashMap<>();
+ Pattern keyPattern = Pattern.compile("([^!#=][^=]*)=(.*)");
+
+ if (file == null) {
+ throw new IllegalArgumentException("NiFi properties file missing or unreadable");
+ }
+ // Scan the properties file for duplicate keys
+ try (BufferedReader bufferedReader = new BufferedReader(new FileReader(file))) {
+ String line;
+ while ((line = bufferedReader.readLine()) != null) {
+ Matcher matcher = keyPattern.matcher(line);
+
+ if (matcher.matches()) {
+ String key = matcher.group(1);
+ String value = matcher.group(2);
+
+ String existingValue = properties.put(key, value);
+
+ if (existingValue != null) {
+ if (existingValue.equals(value)) {
+ logger.warn("Duplicate keys found for key '{}', but values are the same.", key);
+ } else {
+ Set<String> dupes = duplicateProperties.computeIfAbsent(key, k -> new HashSet<>(Collections.singleton(existingValue)));
+ dupes.add(value);
+ }
+ }
Review Comment:
Tested case where property key, nifi.web.https.host, was a triplicate, both when property value was the same for all three (and it accepted with this warn) and when it was different (and nifi didn't start up. but it mentioned all three values in the error message)
logic well though tout
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory closed pull request #6061: NIFI-5378 prevent duplicate keys with different values in nifi.p…
Posted by GitBox <gi...@apache.org>.
exceptionfactory closed pull request #6061: NIFI-5378 prevent duplicate keys with different values in nifi.p…
URL: https://github.com/apache/nifi/pull/6061
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org