GH-8691: Support age FTP, SMB over time filters #8725

AdamaSorho · 2023-09-02T05:15:53Z

Add LastModifiedFTPFileListFilter and LastModifiedSmbFileListFilter respectively for FTP and SMB to enhance available files filtering based on a last-modified strategy.

AdamaSorho · 2023-09-02T05:26:30Z

I ran the tests before starting to work on the issue, but I am getting an error for sftp. So, I did not touch this module. Let me know if I must configure something to pass this test. I will add age support filtering to sftp module once I pass the tests.

See screenshot below:

artembilan · 2023-09-02T15:20:46Z

Thank you for reporting that @AdamaSorho !

Please, consider to add testRuntimeOnly 'net.i2p.crypto:eddsa:0.3.0' into spring-integration-sftp module. See build.gradle.
As part of your PR is OK. Turned out you are not only one who faced this problem for non-default known_hosts file.

Also: please, stop posting screenshots for stacktraces: it would much easier for me to copy/paste info for search 😄

AdamaSorho · 2023-09-02T22:37:23Z

Understood, sorry about that.

The tests passed now for sftp module. Could you tell me why we need net.i2p.crypto:eddsa:0.3.0? I don't know understand 🙈

Thank you

artembilan · 2023-09-02T23:45:13Z

We need that library to be able to parse that Ed25519 encrypted key in your known_hosts file: https://github.com/str4d/ed25519-java

AdamaSorho · 2023-09-02T23:50:50Z

Ok I see. Thank you

artembilan · 2023-09-05T14:46:41Z

...src/main/java/org/springframework/integration/ftp/filters/LastModifiedFTPFileListFilter.java

+ *
+ * @since 6.2
+ */
+public class LastModifiedFTPFileListFilter implements DiscardAwareFileListFilter<FTPFile> {


We'd prefer to name this as FtpLastModifiedFileListFilter

Only this should be renamed or the other classes too?

Right. Others, too. Just follow same pattern for consistency.

artembilan · 2023-09-05T14:50:09Z

...src/main/java/org/springframework/integration/ftp/filters/LastModifiedFTPFileListFilter.java

+
+	private static final long ONE_SECOND = 1000;
+	private static final long DEFAULT_AGE = 60;
+	private volatile long age = DEFAULT_AGE;


Every member in the class must be surrounded with blank lines (including the last method in class before that closing }): much easier to read the code.
No need to have this as a volatile: I don't believe in properties changing at runtime where you cannot predict which one is going to be used right after that.

We also need to see if better to save this value as a Duration.

That FTPFile exposes an API like this:

/** * Gets the file timestamp. This usually the last modification time. * * @return A Calendar instance representing the file timestamp. * @since 3.9.0 */ public Instant getTimestampInstant() {

which is nice to work with Duration.

artembilan · 2023-09-05T14:54:53Z

...est/java/org/springframework/integration/ftp/filters/LastModifiedFTPFileListFilterTests.java

+ */
+@SpringJUnitConfig
+@DirtiesContext
+public class LastModifiedFTPFileListFilterTests extends FtpTestSupport {


Do we really need the real Spring configuration and FTP server to just test a logic of last modified filter?

artembilan · 2023-09-05T14:57:21Z

...c/main/java/org/springframework/integration/sftp/filters/LastModifiedSftpFileListFilter.java

+	}
+
+	private boolean fileIsAged(SftpClient.DirEntry file, long now) {
+		return file.getAttributes().getModifyTime().to(TimeUnit.SECONDS) + this.age <= now;


Same here about Duration and FileTime.toInstant()

artembilan · 2023-09-05T14:57:49Z

...t/java/org/springframework/integration/sftp/filters/LastModifiedSftpFileListFilterTests.java

+ */
+@SpringJUnitConfig
+@DirtiesContext
+public class LastModifiedSftpFileListFilterTests extends SftpTestSupport {


Same here about unreasonable overhead with Spring config and real SFTP server.

artembilan · 2023-09-05T15:00:57Z

...src/main/java/org/springframework/integration/smb/filters/LastModifiedSmbFileListFilter.java

+	}
+
+	private boolean fileIsAged(SmbFile file, long now) {
+		return file.getLastModified() / ONE_SECOND + this.age <= now;


This SMB API does not provide an Instant representation, so, we have to stick with millis.
However those blank lines around members an removal of volatile are still valid concerns.

artembilan · 2023-09-05T15:03:32Z

...est/java/org/springframework/integration/smb/filters/LastModifiedSmbFileListFilterTests.java

+ */
+@SpringJUnitConfig
+@DirtiesContext
+public class LastModifiedSmbFileListFilterTests extends SmbTestSupport {


And this one is more drastic than others: it starts a Docker container for Samba server.
Therefore I'd prefer to have all these new tests against mocks and fully in-memory.

artembilan

This is very close to the merge.

Now, let's consider to mentioned these new filters in the respective FTP, SFTP and SMB chapters of the docs.
The change could make it into the ftp/inbound.adoc, sftp/inbound.adoc, and smb.adoc, respectively.

The whats-new.adoc also must have a new simple section like === Remote Files Support Changes and mention all new filter there.

Thank you!

artembilan · 2023-09-11T16:28:39Z

...src/main/java/org/springframework/integration/ftp/filters/FtpLastModifiedFileListFilter.java

+	 * @param age the age in seconds.
+	 * @param unit the timeUnit.
+	 */
+	public void setAge(long age, TimeUnit unit) {


I don't think we need a TimeUnit-based API since we have a Duration-one. It really covers any possible time units.

artembilan · 2023-09-11T16:30:16Z

...est/java/org/springframework/integration/ftp/filters/FtpLastModifiedFileListFilterTests.java

+	@Test
+	public void testAge() {
+		FtpLastModifiedFileListFilter filter = new FtpLastModifiedFileListFilter();
+		filter.setAge(60, TimeUnit.SECONDS);


Why do we need to set this value if it is exactly what we have by default?

artembilan · 2023-09-11T16:31:57Z

...src/main/java/org/springframework/integration/ftp/filters/FtpLastModifiedFileListFilter.java

@@ -0,0 +1,147 @@
+/*
+ * Copyright 2015-2023 the original author or authors.


This class was really created in the 2023. Therefore copyright is just for the current year.

artembilan · 2023-09-11T16:32:10Z

...c/main/java/org/springframework/integration/sftp/filters/SftpLastModifiedFileListFilter.java

@@ -0,0 +1,143 @@
+/*
+ * Copyright 2013-2023 the original author or authors.


artembilan · 2023-09-11T16:32:22Z

...c/main/java/org/springframework/integration/sftp/filters/SftpLastModifiedFileListFilter.java

+	 * @param age the age in seconds.
+	 * @param unit the timeUnit.
+	 */
+	public void setAge(long age, TimeUnit unit) {


Turned out we don't need anymore since we're using Duration for age in FtpLastModifiedFileListFilter and SftpLastModifiedFileListFilter.

artembilan

Looks like something is wrong with your Git workflow.

Switch to main
Perform git pull upstream main
Switch back to your branch
Perform git rebase main
git push origin -f

Otherwise your PR contains changes you didn't do. Plus it suggests them as new changes somehow...

artembilan

OK. Just one nit-pick.
Will take care about that on merge.

Forgot to mention: when we force push to the PR, GitHub doesn't notify us about changes.
So, consider in the future to leave some comment that it ready for review.

Thanks

artembilan · 2023-09-12T20:36:46Z

...src/main/java/org/springframework/integration/smb/filters/SmbLastModifiedFileListFilter.java

+	 * @param age the age in seconds.
+	 * @param unit the timeUnit.
+	 */
+	public void setAge(long age, TimeUnit unit) {


I believe this one is still has to be remove: the Duration does the trick for us anyway.

artembilan

OK. After looking into this closely, I suggest this evolution.

How about extracting an AstractLastModifiedFileListFilter<F> with all the common logic and only leave something like abstract Instant getLastModified(F remoteFile); for the implementors?
I believe this way even that existing LastModifiedFileListFilter could benefit from new super class.

Yes, some of the implementors would need to use Instant.ofEpochSecond() to convert their long to expected format by super class.

artembilan · 2023-09-12T20:54:18Z

src/reference/antora/modules/ROOT/pages/ftp/inbound.adoc

+public FtpLastModifiedFileListFilter ftpLastModifiedFileListFilter() {
+	FtpLastModifiedFileListFilter filter = new FtpLastModifiedFileListFilter();
+
+	filter.setAge(Duration.ofSeconds(180));


We have ctor for this. Therefore this sample would end up with a single line.
More over I think we don't this examples at all: the API of the class is pretty straightforward and there is a good Javadoc.
We may say though, please, look into its Javadoc for more info.

AdamaSorho · 2023-09-13T14:54:42Z

Yeah, that will be perfect!

AdamaSorho · 2023-09-13T17:28:39Z

...in/java/org/springframework/integration/file/filters/AbstractLastModifiedFileListFilter.java

+
+	protected abstract Instant getLastModified(F remoteFile);
+
+	protected Duration getCurrentAge() {


I added this method because we have a long getAge() in LastModifiedFileListFilter for backward compatibility.

I think we should deprecate long getAge().

Yes, let's deprecate that one with no replacement.
This protected one probably still makes sense, but let's call it getAgeDuration()!

...e/src/main/java/org/springframework/integration/file/filters/LastModifiedFileListFilter.java

artembilan

This looks very cool!
Just a couple minor comments.

You may not be agreed with me on some of them, so just comment with your arguments.
Otherwise I believe it can be merged after those fixes.

artembilan · 2023-09-13T20:02:30Z

...in/java/org/springframework/integration/file/filters/AbstractLastModifiedFileListFilter.java

+
+	protected abstract Instant getLastModified(F remoteFile);
+
+	protected Duration getCurrentAge() {


Yes, let's deprecate that one with no replacement.
This protected one probably still makes sense, but let's call it getAgeDuration()!

...e/src/main/java/org/springframework/integration/file/filters/LastModifiedFileListFilter.java

artembilan · 2023-09-13T20:07:42Z

...src/main/java/org/springframework/integration/smb/filters/SmbLastModifiedFileListFilter.java

+ */
+public class SmbLastModifiedFileListFilter extends AbstractLastModifiedFileListFilter<SmbFile> {
+
+	private static final long ONE_SECOND = 1000;


I think this can go to super class as protected constant. Even if it is not used over there, it is still convenient for its implementors and we won't duplicate ourselves.

artembilan

Pulling locally for the final review, clean up and merge...

artembilan · 2023-09-14T16:17:20Z

Merged as 73ed3ee after some clean up.

Thank you again for contributions!

AdamaSorho · 2023-09-14T17:20:28Z

Thank you!

Don't hesitate to ping me on any issue at anytime.

artembilan requested changes Sep 5, 2023

View reviewed changes

artembilan requested changes Sep 11, 2023

View reviewed changes

AdamaSorho added 5 commits September 11, 2023 14:20

spring-projectsGH-8691: Support age FTP, SMB over time filters

e966cfe

spring-projectsGH-8691: Support age for sftp module

9071998

spring-projectsGH-8691: update tests and class names

fd6ec9e

spring-projectsGH-8691: Remove setAge with TimeUnit

66c1315

Turned out we don't need anymore since we're using Duration for age in FtpLastModifiedFileListFilter and SftpLastModifiedFileListFilter.

spring-projectsGH-8691: Add changes to the docs

dbc3e1d

artembilan requested changes Sep 11, 2023

View reviewed changes

AdamaSorho added 3 commits September 11, 2023 16:05

spring-projectsGH-8691: Support age FTP, SMB over time filters

4723fe8

spring-projectsGH-8691: Support age for sftp module

651666d

spring-projectsGH-8691: update tests and class names

7794676

AdamaSorho force-pushed the GH-8691 branch from c17dff5 to 7794676 Compare September 11, 2023 21:20

artembilan approved these changes Sep 12, 2023

View reviewed changes

artembilan requested changes Sep 12, 2023

View reviewed changes

spring-projectsGH-8691: Introduce AbstractLastModifiedFileListFilter

e6b17c7

AdamaSorho commented Sep 13, 2023

View reviewed changes

...e/src/main/java/org/springframework/integration/file/filters/LastModifiedFileListFilter.java Show resolved Hide resolved

artembilan requested changes Sep 13, 2023

View reviewed changes

spring-projectsGH-8691: Deprecate unnecessary methods

66dc9c6

artembilan approved these changes Sep 14, 2023

View reviewed changes

artembilan closed this Sep 14, 2023

AdamaSorho deleted the GH-8691 branch September 14, 2023 17:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-8691: Support age FTP, SMB over time filters #8725

GH-8691: Support age FTP, SMB over time filters #8725

AdamaSorho commented Sep 2, 2023

AdamaSorho commented Sep 2, 2023

artembilan commented Sep 2, 2023

AdamaSorho commented Sep 2, 2023

artembilan commented Sep 2, 2023

AdamaSorho commented Sep 2, 2023

artembilan Sep 5, 2023

AdamaSorho Sep 5, 2023

artembilan Sep 5, 2023

artembilan Sep 5, 2023

artembilan Sep 5, 2023

artembilan Sep 5, 2023

artembilan Sep 5, 2023

artembilan Sep 5, 2023

artembilan Sep 5, 2023

artembilan left a comment

artembilan Sep 11, 2023

artembilan Sep 11, 2023

artembilan Sep 11, 2023

artembilan Sep 11, 2023

artembilan Sep 11, 2023

artembilan left a comment

artembilan left a comment

artembilan Sep 12, 2023

artembilan left a comment

artembilan Sep 12, 2023

AdamaSorho commented Sep 13, 2023

AdamaSorho Sep 13, 2023

artembilan Sep 13, 2023

artembilan left a comment

artembilan Sep 13, 2023

artembilan Sep 13, 2023

artembilan left a comment

artembilan commented Sep 14, 2023

AdamaSorho commented Sep 14, 2023

		@@ -0,0 +1,147 @@
		/*
		* Copyright 2015-2023 the original author or authors.

		@@ -0,0 +1,143 @@
		/*
		* Copyright 2013-2023 the original author or authors.


		protected abstract Instant getLastModified(F remoteFile);

		protected Duration getCurrentAge() {

GH-8691: Support age FTP, SMB over time filters #8725

GH-8691: Support age FTP, SMB over time filters #8725

Conversation

AdamaSorho commented Sep 2, 2023

AdamaSorho commented Sep 2, 2023

artembilan commented Sep 2, 2023

AdamaSorho commented Sep 2, 2023

artembilan commented Sep 2, 2023

AdamaSorho commented Sep 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artembilan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artembilan left a comment

Choose a reason for hiding this comment

artembilan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artembilan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdamaSorho commented Sep 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artembilan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artembilan left a comment

Choose a reason for hiding this comment

artembilan commented Sep 14, 2023

AdamaSorho commented Sep 14, 2023