You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "umarhussain15 (via GitHub)" <gi...@apache.org> on 2024/02/02 16:05:05 UTC
[PR] NIFI-12708 UnpackContent should allow the user to specify a character set to apply in reading paths and filenames [nifi]
umarhussain15 opened a new pull request, #8350:
URL: https://github.com/apache/nifi/pull/8350
<!-- 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
[NIFI-12708](https://issues.apache.org/jira/browse/NIFI-12708): add option in UnpackContent to specify encoding charset for filenames in zip unpacking
The processor can now take a filename encoding parameter and pass it to zip unpacking. This will allow
user to unzip files with specific encoding to get correct filenames in output.
This for example help with zip files created on Windows which by default uses Cp437 for filename encoding.
If the filename contains special character like German alphabet ä, ü etc., decoding this with Linux's
default encoding (usually UTF8) will contain `?` in output. When the same file is processed with property
set with `Cp437`, the processor outputs correct filenames with special characters preserved.
# 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
Please indicate the verification steps performed prior to pull request creation.
### Build
- [x] Build completed using `mvn clean install -P contrib-check`
- [x] JDK 21
### Licensing
- [x] 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)
- [x] New dependencies are documented in applicable `LICENSE` and `NOTICE` files
### Documentation
- [x] 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
Re: [PR] NIFI-12708 UnpackContent should allow the user to specify a character set to apply in reading paths and filenames [nifi]
Posted by "umarhussain15 (via GitHub)" <gi...@apache.org>.
umarhussain15 commented on code in PR #8350:
URL: https://github.com/apache/nifi/pull/8350#discussion_r1476587478
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/UnpackContent.java:
##########
@@ -473,24 +495,28 @@ protected void processEntry(final InputStream zipInputStream, final boolean dire
private static class CompressedZipInputStreamCallback extends ZipInputStreamCallback {
private final boolean allowStoredEntriesWithDataDescriptor;
+ private final Charset filenameEncoding;
private CompressedZipInputStreamCallback(
final Pattern fileFilter,
final ProcessSession session,
final FlowFile sourceFlowFile,
final List<FlowFile> unpacked,
final String fragmentId,
- final boolean allowStoredEntriesWithDataDescriptor
+ final boolean allowStoredEntriesWithDataDescriptor,
+ final Charset filenameEncoding
) {
super(fileFilter, session, sourceFlowFile, unpacked, fragmentId);
this.allowStoredEntriesWithDataDescriptor = allowStoredEntriesWithDataDescriptor;
+ this.filenameEncoding = filenameEncoding;
}
@Override
public void process(final InputStream inputStream) throws IOException {
- try (final ZipArchiveInputStream zipInputStream = new ZipArchiveInputStream(new BufferedInputStream(inputStream), null, true, allowStoredEntriesWithDataDescriptor)) {
+ try (final ZipArchiveInputStream zipInputStream = new ZipArchiveInputStream(new BufferedInputStream(inputStream),
+ filenameEncoding.toString(), true, allowStoredEntriesWithDataDescriptor)) {
ZipArchiveEntry zipEntry;
- while ((zipEntry = zipInputStream.getNextZipEntry()) != null) {
+ while ((zipEntry = zipInputStream.getNextEntry()) != null) {
Review Comment:
I will revert these method name changes
--
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
Re: [PR] NIFI-12708 UnpackContent should allow the user to specify a character set to apply in reading paths and filenames [nifi]
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #8350:
URL: https://github.com/apache/nifi/pull/8350#discussion_r1476482273
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/UnpackContent.java:
##########
@@ -139,6 +145,20 @@ public class UnpackContent extends AbstractProcessor {
PackageFormat.FLOWFILE_STREAM_FORMAT_V2.toString(), PackageFormat.FLOWFILE_TAR_FORMAT.toString())
.defaultValue(PackageFormat.AUTO_DETECT_FORMAT.toString())
.build();
+ public static final PropertyDescriptor ZIP_FILENAMES_ENCODING = new PropertyDescriptor.Builder()
+ .name("zip-filenames-encoding")
Review Comment:
Actually, using `Filename Character Set` for both the name and display name would be optimal.
--
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
Re: [PR] NIFI-12708 UnpackContent should allow the user to specify a character set to apply in reading paths and filenames [nifi]
Posted by "joewitt (via GitHub)" <gi...@apache.org>.
joewitt commented on code in PR #8350:
URL: https://github.com/apache/nifi/pull/8350#discussion_r1476452074
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/UnpackContent.java:
##########
@@ -353,7 +374,7 @@ public void unpack(final ProcessSession session, final FlowFile source, final Li
int fragmentCount = 0;
try (final TarArchiveInputStream tarIn = new TarArchiveInputStream(new BufferedInputStream(inputStream))) {
TarArchiveEntry tarEntry;
- while ((tarEntry = tarIn.getNextTarEntry()) != null) {
+ while ((tarEntry = tarIn.getNextEntry()) != null) {
Review Comment:
Is there some documentation you can point to suggesting changing this method call is preferable? also changing this tar one here given this is about zip changes is not ideal. Generally fine but if there is a clear value to doing so
--
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
Re: [PR] NIFI-12708 UnpackContent should allow the user to specify a character set to apply in reading paths and filenames [nifi]
Posted by "umarhussain15 (via GitHub)" <gi...@apache.org>.
umarhussain15 commented on code in PR #8350:
URL: https://github.com/apache/nifi/pull/8350#discussion_r1477060364
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestUnpackContent.java:
##########
@@ -222,6 +225,88 @@ public void testInvalidZip() throws IOException {
flowFile.assertContentEquals(path.toFile());
}
}
+ @Test
+ public void testZipEncodingField() {
+ final TestRunner unpackRunner = TestRunners.newTestRunner(new UnpackContent());
+ unpackRunner.setProperty(UnpackContent.PACKAGING_FORMAT, UnpackContent.PackageFormat.ZIP_FORMAT.toString());
+ unpackRunner.setProperty(UnpackContent.ZIP_FILENAME_CHARSET, "invalid-encoding");
+ unpackRunner.assertNotValid();
+ unpackRunner.setProperty(UnpackContent.ZIP_FILENAME_CHARSET, "IBM437");
+ unpackRunner.assertValid();
+ unpackRunner.setProperty(UnpackContent.ZIP_FILENAME_CHARSET, "Cp437");
+ unpackRunner.assertValid();
+ unpackRunner.setProperty(UnpackContent.ZIP_FILENAME_CHARSET, StandardCharsets.ISO_8859_1.name());
+ unpackRunner.assertValid();
+ unpackRunner.setProperty(UnpackContent.ZIP_FILENAME_CHARSET, StandardCharsets.UTF_8.name());
+ unpackRunner.assertValid();
+
+ }
+ @Test
+ public void testZipWithCp437Encoding() throws IOException {
+ String zipFilename = "windows-with-cp437.zip";
+ final TestRunner unpackRunner = TestRunners.newTestRunner(new UnpackContent());
+ final TestRunner autoUnpackRunner = TestRunners.newTestRunner(new UnpackContent());
+ unpackRunner.setProperty(UnpackContent.PACKAGING_FORMAT, UnpackContent.PackageFormat.ZIP_FORMAT.toString());
+ unpackRunner.setProperty(UnpackContent.ZIP_FILENAME_CHARSET, "Cp437");
+ unpackRunner.setProperty(UnpackContent.ALLOW_STORED_ENTRIES_WITH_DATA_DESCRIPTOR, "true"); // just forces this to be exercised
+
+ autoUnpackRunner.setProperty(UnpackContent.PACKAGING_FORMAT, UnpackContent.PackageFormat.AUTO_DETECT_FORMAT.toString());
+ autoUnpackRunner.setProperty(UnpackContent.ZIP_FILENAME_CHARSET, "Cp437");
+
+ unpackRunner.enqueue(dataPath.resolve(zipFilename));
+ unpackRunner.enqueue(dataPath.resolve(zipFilename));
+
+ Map<String, String> attributes = new HashMap<>(1);
+ attributes.put("mime.type", "application/zip");
+ autoUnpackRunner.enqueue(dataPath.resolve(zipFilename), attributes);
+ autoUnpackRunner.enqueue(dataPath.resolve(zipFilename), attributes);
+ unpackRunner.run(2);
+ autoUnpackRunner.run(2);
+
+ unpackRunner.assertTransferCount(UnpackContent.REL_FAILURE, 0);
+ autoUnpackRunner.assertTransferCount(UnpackContent.REL_FAILURE, 0);
+
+ final List<MockFlowFile> unpacked =
+ unpackRunner.getFlowFilesForRelationship(UnpackContent.REL_SUCCESS);
+ for (final MockFlowFile flowFile : unpacked) {
+ final String filename = flowFile.getAttribute(CoreAttributes.FILENAME.key());
+ assertTrue(StringUtils.containsNone(filename, "?"), "filename contains '?': " + filename);
+ assertTrue(StringUtils.containsNone(filename, "�"), "filename contains '�': " + filename);
+ final String path = flowFile.getAttribute(CoreAttributes.PATH.key());
+ assertTrue(StringUtils.containsNone(path, "?"), "path contains '?': " + path);
+ assertTrue(StringUtils.containsNone(path, "�"), "path contains '�': " + path);
Review Comment:
Thanks. I have updated the test cases to generate zip with known Unicode character and to look for it in the output filename.
The test case with Windows zip file now only look for `?` in test case as the input contains both files with special character in names and without any special character in name. Also, UnpackContent is always outputting `?` for this zip file (the original issue reported in the ticket).
--
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
Re: [PR] NIFI-12708 UnpackContent should allow the user to specify a character set to apply in reading paths and filenames [nifi]
Posted by "joewitt (via GitHub)" <gi...@apache.org>.
joewitt commented on code in PR #8350:
URL: https://github.com/apache/nifi/pull/8350#discussion_r1476463506
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestUnpackContent.java:
##########
@@ -222,6 +225,88 @@ public void testInvalidZip() throws IOException {
flowFile.assertContentEquals(path.toFile());
}
}
+ @Test
+ public void testZipEncodingField() {
Review Comment:
Super wonderful that you created a unit test to cover this and provided a simple example zip to do so! But the checks will likely fail because the zip file doesn't have a license header so I believe you'll need to add it to an excludes list for the RAT process to not try to extract a header. You'll see other examples in some pom files for such exclusions.
--
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
Re: [PR] NIFI-12708 UnpackContent should allow the user to specify a character set to apply in reading paths and filenames [nifi]
Posted by "dan-s1 (via GitHub)" <gi...@apache.org>.
dan-s1 commented on code in PR #8350:
URL: https://github.com/apache/nifi/pull/8350#discussion_r1476461300
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/UnpackContent.java:
##########
@@ -139,6 +145,20 @@ public class UnpackContent extends AbstractProcessor {
PackageFormat.FLOWFILE_STREAM_FORMAT_V2.toString(), PackageFormat.FLOWFILE_TAR_FORMAT.toString())
.defaultValue(PackageFormat.AUTO_DETECT_FORMAT.toString())
.build();
+ public static final PropertyDescriptor ZIP_FILENAMES_ENCODING = new PropertyDescriptor.Builder()
+ .name("zip-filenames-encoding")
+ .displayName("Zip Filenames Encoding")
+ .description(
+ "The encoding used by zip creating utility, for the file names inside the zip. Processor will pass this encoding to Zip unpacker. For example 'Cp437', 'UTF8' etc. Default is to "
+ + "use platform's encoding. This can be useful for example incase a zip was created on Windows with Cp437 and unpacked on Linux machine. Without correct encoding value special "
+ + "characters in filenames will be outputted as `?`")
+ .required(false)
+ .dependsOn(
+ PACKAGING_FORMAT,
+ PackageFormat.ZIP_FORMAT.toString(),
+ PackageFormat.AUTO_DETECT_FORMAT.toString())
+ .addValidator(new CharsetStringValidator())
+ .build();
Review Comment:
Add a default value so can simplify code on lines 255-257
```suggestion
.defaultValue(Charset.defaultCharset())
.build();
```
--
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
Re: [PR] NIFI-12708 UnpackContent should allow the user to specify a character set to apply in reading paths and filenames [nifi]
Posted by "joewitt (via GitHub)" <gi...@apache.org>.
joewitt commented on code in PR #8350:
URL: https://github.com/apache/nifi/pull/8350#discussion_r1476455470
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/UnpackContent.java:
##########
@@ -473,24 +495,28 @@ protected void processEntry(final InputStream zipInputStream, final boolean dire
private static class CompressedZipInputStreamCallback extends ZipInputStreamCallback {
private final boolean allowStoredEntriesWithDataDescriptor;
+ private final Charset filenameEncoding;
private CompressedZipInputStreamCallback(
final Pattern fileFilter,
final ProcessSession session,
final FlowFile sourceFlowFile,
final List<FlowFile> unpacked,
final String fragmentId,
- final boolean allowStoredEntriesWithDataDescriptor
+ final boolean allowStoredEntriesWithDataDescriptor,
+ final Charset filenameEncoding
) {
super(fileFilter, session, sourceFlowFile, unpacked, fragmentId);
this.allowStoredEntriesWithDataDescriptor = allowStoredEntriesWithDataDescriptor;
+ this.filenameEncoding = filenameEncoding;
}
@Override
public void process(final InputStream inputStream) throws IOException {
- try (final ZipArchiveInputStream zipInputStream = new ZipArchiveInputStream(new BufferedInputStream(inputStream), null, true, allowStoredEntriesWithDataDescriptor)) {
+ try (final ZipArchiveInputStream zipInputStream = new ZipArchiveInputStream(new BufferedInputStream(inputStream),
+ filenameEncoding.toString(), true, allowStoredEntriesWithDataDescriptor)) {
ZipArchiveEntry zipEntry;
- while ((zipEntry = zipInputStream.getNextZipEntry()) != null) {
+ while ((zipEntry = zipInputStream.getNextEntry()) != null) {
Review Comment:
Same ask as in the tar case. Why change the method call 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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-12708 UnpackContent should allow the user to specify a character set to apply in reading paths and filenames [nifi]
Posted by "dan-s1 (via GitHub)" <gi...@apache.org>.
dan-s1 commented on code in PR #8350:
URL: https://github.com/apache/nifi/pull/8350#discussion_r1476296507
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/UnpackContent.java:
##########
@@ -643,4 +672,20 @@ public static PackageFormat getFormat(String textValue) {
};
}
}
+
+ /**
+ * Validates if the given input string is a valid {@link java.nio.charset.Charset} representation
+ */
+ private static class CharsetStringValidator implements Validator {
+ @Override
+ public ValidationResult validate(String subject, String input, ValidationContext context) {
+ try {
+ Charsets.toCharset(input);
+ return new ValidationResult.Builder().subject(subject).input(input).valid(true).explanation("Valid charset").build();
+ } catch (UnsupportedCharsetException e) {
+ return new ValidationResult.Builder().subject(subject).input(input).valid(false)
+ .explanation("Invalid encoding charset: "+e.getMessage()).build();
+ }
+ }
+ }
}
Review Comment:
I believe there is no need to create this class as `nifi-commons/nifi-utils/src/main/java/org/apache/nifi/processor/util/StandardValidators.java` already has `CHARACTER_SET_VALIDATOR`
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/UnpackContent.java:
##########
@@ -139,6 +145,20 @@ public class UnpackContent extends AbstractProcessor {
PackageFormat.FLOWFILE_STREAM_FORMAT_V2.toString(), PackageFormat.FLOWFILE_TAR_FORMAT.toString())
.defaultValue(PackageFormat.AUTO_DETECT_FORMAT.toString())
.build();
+ public static final PropertyDescriptor ZIP_FILENAMES_ENCODING = new PropertyDescriptor.Builder()
+ .name("zip-filenames-encoding")
+ .displayName("Zip Filenames Encoding")
+ .description(
+ "The encoding used by zip creating utility, for the file names inside the zip. Processor will pass this encoding to Zip unpacker. For example 'Cp437', 'UTF8' etc. Default is to "
+ + "use platform's encoding. This can be useful for example incase a zip was created on Windows with Cp437 and unpacked on Linux machine. Without correct encoding value special "
+ + "characters in filenames will be outputted as `?`")
+ .required(false)
+ .dependsOn(
+ PACKAGING_FORMAT,
+ PackageFormat.ZIP_FORMAT.toString(),
+ PackageFormat.AUTO_DETECT_FORMAT.toString())
+ .addValidator(new CharsetStringValidator())
Review Comment:
Per earlier comment
```suggestion
.addValidator(StandardValidators.CHARACTER_SET_VALIDATOR)
```
--
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
Re: [PR] NIFI-12708 UnpackContent should allow the user to specify a character set to apply in reading paths and filenames [nifi]
Posted by "joewitt (via GitHub)" <gi...@apache.org>.
joewitt commented on code in PR #8350:
URL: https://github.com/apache/nifi/pull/8350#discussion_r1476391079
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/UnpackContent.java:
##########
@@ -139,6 +145,20 @@ public class UnpackContent extends AbstractProcessor {
PackageFormat.FLOWFILE_STREAM_FORMAT_V2.toString(), PackageFormat.FLOWFILE_TAR_FORMAT.toString())
.defaultValue(PackageFormat.AUTO_DETECT_FORMAT.toString())
.build();
+ public static final PropertyDescriptor ZIP_FILENAMES_ENCODING = new PropertyDescriptor.Builder()
+ .name("zip-filenames-encoding")
Review Comment:
Consider just 'filename-charset' for (name) and 'Filename Character Set' for Display name.
This only applies when it is zip format selection so no need to add 'zip' to the name as the context for use is already established.
--
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
Re: [PR] NIFI-12708 UnpackContent should allow the user to specify a character set to apply in reading paths and filenames [nifi]
Posted by "dan-s1 (via GitHub)" <gi...@apache.org>.
dan-s1 commented on code in PR #8350:
URL: https://github.com/apache/nifi/pull/8350#discussion_r1476296507
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/UnpackContent.java:
##########
@@ -643,4 +672,20 @@ public static PackageFormat getFormat(String textValue) {
};
}
}
+
+ /**
+ * Validates if the given input string is a valid {@link java.nio.charset.Charset} representation
+ */
+ private static class CharsetStringValidator implements Validator {
+ @Override
+ public ValidationResult validate(String subject, String input, ValidationContext context) {
+ try {
+ Charsets.toCharset(input);
+ return new ValidationResult.Builder().subject(subject).input(input).valid(true).explanation("Valid charset").build();
+ } catch (UnsupportedCharsetException e) {
+ return new ValidationResult.Builder().subject(subject).input(input).valid(false)
+ .explanation("Invalid encoding charset: "+e.getMessage()).build();
+ }
+ }
+ }
}
Review Comment:
I believe there is no need to create this class as `nifi-commons/nifi-utils/src/main/java/org/apache/nifi/processor/util/StandardValidators.java` already has `CHARACTER_SET_VALIDATOR`
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/UnpackContent.java:
##########
@@ -139,6 +145,20 @@ public class UnpackContent extends AbstractProcessor {
PackageFormat.FLOWFILE_STREAM_FORMAT_V2.toString(), PackageFormat.FLOWFILE_TAR_FORMAT.toString())
.defaultValue(PackageFormat.AUTO_DETECT_FORMAT.toString())
.build();
+ public static final PropertyDescriptor ZIP_FILENAMES_ENCODING = new PropertyDescriptor.Builder()
+ .name("zip-filenames-encoding")
+ .displayName("Zip Filenames Encoding")
+ .description(
+ "The encoding used by zip creating utility, for the file names inside the zip. Processor will pass this encoding to Zip unpacker. For example 'Cp437', 'UTF8' etc. Default is to "
+ + "use platform's encoding. This can be useful for example incase a zip was created on Windows with Cp437 and unpacked on Linux machine. Without correct encoding value special "
+ + "characters in filenames will be outputted as `?`")
+ .required(false)
+ .dependsOn(
+ PACKAGING_FORMAT,
+ PackageFormat.ZIP_FORMAT.toString(),
+ PackageFormat.AUTO_DETECT_FORMAT.toString())
+ .addValidator(new CharsetStringValidator())
Review Comment:
Per earlier comment
```suggestion
.addValidator(StandardValidators.CHARACTER_SET_VALIDATOR)
```
--
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
Re: [PR] NIFI-12708 UnpackContent should allow the user to specify a character set to apply in reading paths and filenames [nifi]
Posted by "joewitt (via GitHub)" <gi...@apache.org>.
joewitt commented on code in PR #8350:
URL: https://github.com/apache/nifi/pull/8350#discussion_r1476456337
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/UnpackContent.java:
##########
@@ -643,4 +672,20 @@ public static PackageFormat getFormat(String textValue) {
};
}
}
+
Review Comment:
Can remove this block as per above comment on standard validator available.
--
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
Re: [PR] NIFI-12708 UnpackContent should allow the user to specify a character set to apply in reading paths and filenames [nifi]
Posted by "joewitt (via GitHub)" <gi...@apache.org>.
joewitt commented on PR #8350:
URL: https://github.com/apache/nifi/pull/8350#issuecomment-1932485274
Thanks @umarhussain15 - good work and good discussion
--
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
Re: [PR] NIFI-12708 UnpackContent should allow the user to specify a character set to apply in reading paths and filenames [nifi]
Posted by "joewitt (via GitHub)" <gi...@apache.org>.
joewitt commented on code in PR #8350:
URL: https://github.com/apache/nifi/pull/8350#discussion_r1476411597
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/UnpackContent.java:
##########
@@ -139,6 +145,20 @@ public class UnpackContent extends AbstractProcessor {
PackageFormat.FLOWFILE_STREAM_FORMAT_V2.toString(), PackageFormat.FLOWFILE_TAR_FORMAT.toString())
.defaultValue(PackageFormat.AUTO_DETECT_FORMAT.toString())
.build();
+ public static final PropertyDescriptor ZIP_FILENAMES_ENCODING = new PropertyDescriptor.Builder()
+ .name("zip-filenames-encoding")
+ .displayName("Zip Filenames Encoding")
+ .description(
+ "The encoding used by zip creating utility, for the file names inside the zip. Processor will pass this encoding to Zip unpacker. For example 'Cp437', 'UTF8' etc. Default is to "
Review Comment:
Consider alternative text
'If supplied this character set will be supplied to the Zip utility to attempt to decode filenames using the specific character set. If not specified the default platform character set will be used. This is useful if a Zip was created with a different character set than the platform default and the zip uses non standard values to specify.
--
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
Re: [PR] NIFI-12708 UnpackContent should allow the user to specify a character set to apply in reading paths and filenames [nifi]
Posted by "joewitt (via GitHub)" <gi...@apache.org>.
joewitt commented on code in PR #8350:
URL: https://github.com/apache/nifi/pull/8350#discussion_r1476441590
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/UnpackContent.java:
##########
@@ -139,6 +145,20 @@ public class UnpackContent extends AbstractProcessor {
PackageFormat.FLOWFILE_STREAM_FORMAT_V2.toString(), PackageFormat.FLOWFILE_TAR_FORMAT.toString())
.defaultValue(PackageFormat.AUTO_DETECT_FORMAT.toString())
.build();
+ public static final PropertyDescriptor ZIP_FILENAMES_ENCODING = new PropertyDescriptor.Builder()
+ .name("zip-filenames-encoding")
+ .displayName("Zip Filenames Encoding")
+ .description(
+ "The encoding used by zip creating utility, for the file names inside the zip. Processor will pass this encoding to Zip unpacker. For example 'Cp437', 'UTF8' etc. Default is to "
Review Comment:
Instead of describing the Cp437 scenario in the property description consider creating an '@UseCase' annotation to describe the specific case and how the whole processor would be configured. See 'PublishSlack' for an example of this done 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
Re: [PR] NIFI-12708 UnpackContent should allow the user to specify a character set to apply in reading paths and filenames [nifi]
Posted by "dan-s1 (via GitHub)" <gi...@apache.org>.
dan-s1 commented on code in PR #8350:
URL: https://github.com/apache/nifi/pull/8350#discussion_r1476463929
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/UnpackContent.java:
##########
@@ -231,7 +252,10 @@ public void onScheduled(ProcessContext context) throws ProcessException {
}
final PropertyValue allowStoredEntriesWithDataDescriptorVal = context.getProperty(ALLOW_STORED_ENTRIES_WITH_DATA_DESCRIPTOR);
final boolean allowStoredEntriesWithDataDescriptor = allowStoredEntriesWithDataDescriptorVal.isSet() ? allowStoredEntriesWithDataDescriptorVal.asBoolean() : false;
- zipUnpacker = new ZipUnpacker(fileFilter, password, allowStoredEntriesWithDataDescriptor);
+ final PropertyValue filenamesEncodingVal = context.getProperty(ZIP_FILENAMES_ENCODING);
+
+ Charset filenamesEncoding =filenamesEncodingVal.isSet() ? Charsets.toCharset(filenamesEncodingVal.getValue()) : Charset.defaultCharset();
Review Comment:
See prior comment
```suggestion
final String filenamesEncodingVal = context.getProperty(ZIP_FILENAMES_ENCODING).getValue();
final Charset filenamesEncoding = Charsets.toCharset(filenamesEncodingVal);
```
--
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
Re: [PR] NIFI-12708 UnpackContent should allow the user to specify a character set to apply in reading paths and filenames [nifi]
Posted by "joewitt (via GitHub)" <gi...@apache.org>.
joewitt commented on PR #8350:
URL: https://github.com/apache/nifi/pull/8350#issuecomment-1924378592
@umarhussain15 Great start here. Left you a series of comments plus Dan just left one to consider as well. Thanks!
--
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
Re: [PR] NIFI-12708 UnpackContent should allow the user to specify a character set to apply in reading paths and filenames [nifi]
Posted by "asfgit (via GitHub)" <gi...@apache.org>.
asfgit closed pull request #8350: NIFI-12708 UnpackContent should allow the user to specify a character set to apply in reading paths and filenames
URL: https://github.com/apache/nifi/pull/8350
--
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
Re: [PR] NIFI-12708 UnpackContent should allow the user to specify a character set to apply in reading paths and filenames [nifi]
Posted by "dan-s1 (via GitHub)" <gi...@apache.org>.
dan-s1 commented on code in PR #8350:
URL: https://github.com/apache/nifi/pull/8350#discussion_r1476461300
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/UnpackContent.java:
##########
@@ -139,6 +145,20 @@ public class UnpackContent extends AbstractProcessor {
PackageFormat.FLOWFILE_STREAM_FORMAT_V2.toString(), PackageFormat.FLOWFILE_TAR_FORMAT.toString())
.defaultValue(PackageFormat.AUTO_DETECT_FORMAT.toString())
.build();
+ public static final PropertyDescriptor ZIP_FILENAMES_ENCODING = new PropertyDescriptor.Builder()
+ .name("zip-filenames-encoding")
+ .displayName("Zip Filenames Encoding")
+ .description(
+ "The encoding used by zip creating utility, for the file names inside the zip. Processor will pass this encoding to Zip unpacker. For example 'Cp437', 'UTF8' etc. Default is to "
+ + "use platform's encoding. This can be useful for example incase a zip was created on Windows with Cp437 and unpacked on Linux machine. Without correct encoding value special "
+ + "characters in filenames will be outputted as `?`")
+ .required(false)
+ .dependsOn(
+ PACKAGING_FORMAT,
+ PackageFormat.ZIP_FORMAT.toString(),
+ PackageFormat.AUTO_DETECT_FORMAT.toString())
+ .addValidator(new CharsetStringValidator())
+ .build();
Review Comment:
Add a default value so can simplify code on lines 255-257
--
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
Re: [PR] NIFI-12708 UnpackContent should allow the user to specify a character set to apply in reading paths and filenames [nifi]
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #8350:
URL: https://github.com/apache/nifi/pull/8350#discussion_r1476854242
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestUnpackContent.java:
##########
@@ -222,6 +225,88 @@ public void testInvalidZip() throws IOException {
flowFile.assertContentEquals(path.toFile());
}
}
+ @Test
+ public void testZipEncodingField() {
+ final TestRunner unpackRunner = TestRunners.newTestRunner(new UnpackContent());
+ unpackRunner.setProperty(UnpackContent.PACKAGING_FORMAT, UnpackContent.PackageFormat.ZIP_FORMAT.toString());
+ unpackRunner.setProperty(UnpackContent.ZIP_FILENAME_CHARSET, "invalid-encoding");
+ unpackRunner.assertNotValid();
+ unpackRunner.setProperty(UnpackContent.ZIP_FILENAME_CHARSET, "IBM437");
+ unpackRunner.assertValid();
+ unpackRunner.setProperty(UnpackContent.ZIP_FILENAME_CHARSET, "Cp437");
+ unpackRunner.assertValid();
+ unpackRunner.setProperty(UnpackContent.ZIP_FILENAME_CHARSET, StandardCharsets.ISO_8859_1.name());
+ unpackRunner.assertValid();
+ unpackRunner.setProperty(UnpackContent.ZIP_FILENAME_CHARSET, StandardCharsets.UTF_8.name());
+ unpackRunner.assertValid();
+
+ }
+ @Test
+ public void testZipWithCp437Encoding() throws IOException {
+ String zipFilename = "windows-with-cp437.zip";
+ final TestRunner unpackRunner = TestRunners.newTestRunner(new UnpackContent());
+ final TestRunner autoUnpackRunner = TestRunners.newTestRunner(new UnpackContent());
+ unpackRunner.setProperty(UnpackContent.PACKAGING_FORMAT, UnpackContent.PackageFormat.ZIP_FORMAT.toString());
+ unpackRunner.setProperty(UnpackContent.ZIP_FILENAME_CHARSET, "Cp437");
+ unpackRunner.setProperty(UnpackContent.ALLOW_STORED_ENTRIES_WITH_DATA_DESCRIPTOR, "true"); // just forces this to be exercised
+
+ autoUnpackRunner.setProperty(UnpackContent.PACKAGING_FORMAT, UnpackContent.PackageFormat.AUTO_DETECT_FORMAT.toString());
+ autoUnpackRunner.setProperty(UnpackContent.ZIP_FILENAME_CHARSET, "Cp437");
+
+ unpackRunner.enqueue(dataPath.resolve(zipFilename));
+ unpackRunner.enqueue(dataPath.resolve(zipFilename));
+
+ Map<String, String> attributes = new HashMap<>(1);
+ attributes.put("mime.type", "application/zip");
+ autoUnpackRunner.enqueue(dataPath.resolve(zipFilename), attributes);
+ autoUnpackRunner.enqueue(dataPath.resolve(zipFilename), attributes);
+ unpackRunner.run(2);
+ autoUnpackRunner.run(2);
+
+ unpackRunner.assertTransferCount(UnpackContent.REL_FAILURE, 0);
+ autoUnpackRunner.assertTransferCount(UnpackContent.REL_FAILURE, 0);
+
+ final List<MockFlowFile> unpacked =
+ unpackRunner.getFlowFilesForRelationship(UnpackContent.REL_SUCCESS);
+ for (final MockFlowFile flowFile : unpacked) {
+ final String filename = flowFile.getAttribute(CoreAttributes.FILENAME.key());
+ assertTrue(StringUtils.containsNone(filename, "?"), "filename contains '?': " + filename);
+ assertTrue(StringUtils.containsNone(filename, "�"), "filename contains '�': " + filename);
+ final String path = flowFile.getAttribute(CoreAttributes.PATH.key());
+ assertTrue(StringUtils.containsNone(path, "?"), "path contains '?': " + path);
+ assertTrue(StringUtils.containsNone(path, "�"), "path contains '�': " + path);
Review Comment:
These tests for other characters are helpful, but actual character representation is not clear. Instead of using the literal unknown character here, recommend using the Java [Unicode Character](https://www.baeldung.com/java-unicode-character-from-code-point-hex-string#understanding-the-hexadecimal-number-after-u) reference with the hexadecimal number for readability.
--
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
Re: [PR] NIFI-12708 UnpackContent should allow the user to specify a character set to apply in reading paths and filenames [nifi]
Posted by "joewitt (via GitHub)" <gi...@apache.org>.
joewitt commented on code in PR #8350:
URL: https://github.com/apache/nifi/pull/8350#discussion_r1476402391
##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/UnpackContent.java:
##########
@@ -139,6 +145,20 @@ public class UnpackContent extends AbstractProcessor {
PackageFormat.FLOWFILE_STREAM_FORMAT_V2.toString(), PackageFormat.FLOWFILE_TAR_FORMAT.toString())
.defaultValue(PackageFormat.AUTO_DETECT_FORMAT.toString())
.build();
+ public static final PropertyDescriptor ZIP_FILENAMES_ENCODING = new PropertyDescriptor.Builder()
+ .name("zip-filenames-encoding")
+ .displayName("Zip Filenames Encoding")
+ .description(
+ "The encoding used by zip creating utility, for the file names inside the zip. Processor will pass this encoding to Zip unpacker. For example 'Cp437', 'UTF8' etc. Default is to "
+ + "use platform's encoding. This can be useful for example incase a zip was created on Windows with Cp437 and unpacked on Linux machine. Without correct encoding value special "
+ + "characters in filenames will be outputted as `?`")
+ .required(false)
+ .dependsOn(
+ PACKAGING_FORMAT,
+ PackageFormat.ZIP_FORMAT.toString(),
+ PackageFormat.AUTO_DETECT_FORMAT.toString())
+ .addValidator(new CharsetStringValidator())
Review Comment:
Consider changing this to StandardValidators.CHARACTER_SET_VALIDATOR and removing the custom validator created below.
--
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