spring-projects / spring-boot

Spring Boot helps you to create Spring-powered, production-grade applications and services with absolute minimum fuss.
https://spring.io/projects/spring-boot
Apache License 2.0
75.17k stars 40.68k forks source link

FileSystemWatcher trigger filter not working as intended #38715

Closed Vidalee closed 11 months ago

Vidalee commented 11 months ago

Hi all,

You can use FileSystemWatcher.setTriggerFilter() to filter the files for which the watcher will be triggered.

It takes a class implementing java.io.FileFilter as an argument.

According to the documentation, the function accept(File pathname) has to return true for the files you want to include, false otherwise.

But if you try to use a FileFilter with a FileSystemWatcher, this behavior is reversed, meaning that it will only trigger for files rejected by the FileFilter.

I investigated the reason and it's because of this function: https://github.com/spring-projects/spring-boot/blob/e6970243eeddc13fd54bb8afd5c9c1e8bec1ad13/spring-boot-project/spring-boot-devtools/src/main/java/org/springframework/boot/devtools/filewatch/DirectorySnapshot.java#L102-L104

As you can see, we accept the changedFile if the triggerFilter does not accept it.

I did not find any tests for this behavior, or documentation. The FileSystemWatcher documentation only says "Set an optional FileFilter used to limit the files that trigger a change." for the setTriggerFilter(FileFilter triggerFilter) function.

Since this behavior is not consistent with the FileFilter expected usage, I believe it's a bug.

If confirmed, I can make a PR to fix it and add a test.

wilkinsona commented 11 months ago

I think there's a misunderstanding of trigger files here. A trigger file is an additional file that's not part of the application that, when changed, will trigger a Devtools restart. As such, the file should not be included in the ChangedFiles that are derived from the snapshot. Note that the result from accept is used without negation when checking to see if two snapshots have changed before creating the ChangedFiles that contains details of those changes:

https://github.com/spring-projects/spring-boot/blob/e6970243eeddc13fd54bb8afd5c9c1e8bec1ad13/spring-boot-project/spring-boot-devtools/src/main/java/org/springframework/boot/devtools/filewatch/DirectorySnapshot.java#L143-L145

There is a test for this behavior in FileSystemWatcherTests. It verifies that no changes are detected until the trigger file has changed and that when they are detected after the trigger file has changed, only the change file and not the changed trigger file are included in the change set.

https://github.com/spring-projects/spring-boot/blob/e6970243eeddc13fd54bb8afd5c9c1e8bec1ad13/spring-boot-project/spring-boot-devtools/src/test/java/org/springframework/boot/devtools/filewatch/FileSystemWatcherTests.java#L250-L268

What were you trying to do that led to you raising this issue? It sounds like we may need to clarify the javadoc.

Vidalee commented 11 months ago

Only checked for tests on DirectorySnapshot, my bad.

I now understand the correct application of the setTriggerFilter function. My initial expectations where that I could filter files triggering a change event (which is correct) and that only these files will be sent to the listeners of the change event (this is not the case).

My expected usage was to only listen to changes in .txt files in a directory for example, or only to a specific depth. Then, it would only send the .txt files that changed (even if other files changed) to my listeners.

I guess I will have to not use the setTriggerFilter function and listen to every change, then filter in the listener.

Thanks for the clarifications !

Vidalee commented 11 months ago

On a side note, do you think adding a parameter to specify the max scan depth would be interesting for other users?

wilkinsona commented 11 months ago

I can think of a reason why specifying a max scan depth would be generally useful. The depth of the directory/package structure doesn't feel like a common way to determine what should and should not be scanned.

I'll close this one now as things are working as expected.