NIFI-10143 - ListFTP/GetFTP charset issues#6172
Conversation
exceptionfactory
left a comment
exceptionfactory
left a comment
There was a problem hiding this comment.
Thanks for digging into this issue and developing a new test @greyp9! The ClientProvider changes make sense.
The new test is helpful, and covers a lot of ground. It does a lot of work, and it seems to stretch the boundary of a unit test versus an integration test. In light of having existing unit tests for FTP processors, it seems better to maintain a minimal unit test, and then provide this new test class as an integration test. As noted in the detailed comments, there are several ways the test could be optimized. Supporting an external FTP server is also useful, and could be parameterized through System properties or environment variables.
What do you think about changing the test to an integration test, an updating existing unit tests with more minimal changes to incorporate evaluation of UTF-8 encoding?
| private static final int PORT = 2121; | ||
| private static final String USER = "ftpuser"; | ||
| private static final String PASSWORD = "admin"; | ||
| private static final File FOLDER = new File("target", TestFTPCharset.class.getSimpleName()); |
There was a problem hiding this comment.
Instead of using a static directory in the target directory, it would better to use the JUnit 5 TempDir annotation to manage the lifecycle of the temporary directory.
|
|
||
| private static final String USE_UTF8 = Boolean.TRUE.toString(); | ||
| private static final String HOSTNAME = "localhost"; | ||
| private static final int PORT = 2121; |
There was a problem hiding this comment.
It would be better to use a random port to avoid theoretical conflicts in different environments.
| * @throws FtpException on server startup failure | ||
| */ | ||
| @BeforeAll | ||
| static void beforeAll() throws FtpException { |
There was a problem hiding this comment.
It would be helpful to name this method something like startServer, which may obviate the need for the method comments.
| // setup ftp user | ||
| final PropertiesUserManagerFactory userManagerFactory = new PropertiesUserManagerFactory(); | ||
| userManagerFactory.setUrl(TestFTPCharset.class.getClassLoader() | ||
| .getResource("TestFTPCharset/users.properties")); |
There was a problem hiding this comment.
Although it would take a little more setup work, creating the properties file in the test class, using the parameterized username and password, would help avoid potential mismatch between class properties and file properties.
| final FileSystemView view = fileSystemFactory.createFileSystemView(ftpuser); | ||
| final FtpFile workingDirectory = view.getWorkingDirectory(); | ||
| final Object physicalFile = workingDirectory.getPhysicalFile(); | ||
| assertTrue(physicalFile instanceof File); |
There was a problem hiding this comment.
JUnit 5 supports assertInstanceOf
| import static org.junit.jupiter.params.provider.Arguments.arguments; | ||
|
|
||
| /** | ||
| * Seed a UTF-7 folder in FTP server with files using i18n known filenames. Iterate through a set of i18n folder |
There was a problem hiding this comment.
| * Seed a UTF-7 folder in FTP server with files using i18n known filenames. Iterate through a set of i18n folder | |
| * Seed a UTF-8 folder in FTP server with files using i18n known filenames. Iterate through a set of i18n folder |
| * To test against a live FTP server: | ||
| * <ol> | ||
| * <li>replace EMBED_FTP_SERVER value with <code>false</code></li> | ||
| * <li>replace HOSTNAME, PORT, USER, PASSWORD as needed</li> | ||
| * </ol> |
There was a problem hiding this comment.
If the goal is to provide the option for testing against a real FTP server, this could be parameterized through JUnit 5 annotations.
There was a problem hiding this comment.
Added method serverParametersProvider(), which allows for override based on a system property.
| throw new RuntimeException(e); | ||
| } | ||
| } | ||
| FileUtils.deleteDirectory(FOLDER); |
There was a problem hiding this comment.
Refactoring to use JUnit 5 TempDir should remove the need for deleting the directory manually, although it might be necessary to clear the directory, depending on the approach.
| try { | ||
| Thread.sleep(100L); | ||
| } catch (InterruptedException e) { | ||
| throw new RuntimeException(e); | ||
| } |
There was a problem hiding this comment.
The InterruptedException could be declared on the method signature instead of caught and thrown again.
| final Charset charset = unicodeEnabled ? StandardCharsets.UTF_8 : Charset.defaultCharset(); | ||
| client.setCharset(charset); |
There was a problem hiding this comment.
Does this change the current behavior, or does the internal character set of the client use Charset.defaultCharset() when not otherwise specified? This looks like the right way to go, just wanted to clarify.
There was a problem hiding this comment.
After evaluating the Apache FTPClient class and parent classes, it appears that the Charset property is not used, so it seems best to avoid setting the value.
There was a problem hiding this comment.
It was a speculative change. I have no evidence that it makes a difference. Removed this.
I've renamed to *IT to align with integration test naming convention. I'd like to defer modifications of the existing unit tests to be part of future efforts, as needed. |
exceptionfactory
left a comment
exceptionfactory
left a comment
There was a problem hiding this comment.
Thanks for the updates @greyp9, noted a couple additional issues, but this looks close to completion.
...cessors/src/main/java/org/apache/nifi/processors/standard/ftp/StandardFTPClientProvider.java
Outdated
Show resolved
Hide resolved
...-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestFTPCharsetIT.java
Outdated
Show resolved
Hide resolved
exceptionfactory
left a comment
exceptionfactory
left a comment
There was a problem hiding this comment.
Thanks for working through the feedback @greyp9! The additional integration test is helpful and the latest version looks good. +1 merging
2 participants
Summary
NIFI-10143
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000NIFI-00000Pull Request Formatting
mainbranchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
mvn clean install -P contrib-checkLicensing
LICENSEandNOTICEfilesDocumentation