FileSystemPipe more exits #6740

tnleeuw · 2024-04-29T20:46:52Z

No description provided.

…of cleanups in code.

…nues teardown all the way.

…ouched in this PR Begin updating tests for FileSystemPipe.

… feedback.

jkosternl

You've really vacuumed a lot of code, but I think those are good improvements. Thanks Tim!

filesystem/src/test/java/org/frankframework/filesystem/smb/Samba2FileSystemTestHelper.java

commons/src/main/java/org/frankframework/util/CloseUtils.java

nielsm5 · 2024-05-09T11:01:50Z

filesystem/src/main/java/org/frankframework/filesystem/FileSystemUtils.java

@@ -63,13 +62,13 @@ public static <F> void checkSource(IBasicFileSystem<F> fileSystem, F source, Fil
 	public static <F> void prepareDestination(IWritableFileSystem<F> fileSystem, F destination, boolean overwrite, int numOfBackups, FileSystemAction action) throws FileSystemException {
 		if (fileSystem.exists(destination)) {
 			if (overwrite) {
-				log.debug("removing current destination file ["+fileSystem.getCanonicalName(destination)+"]");
+				log.debug("removing current destination file [{}]", fileSystem.getCanonicalName(destination));


Het ophalen van de canonical name kan voor extra compute zorgen bij sommige fs impl.

Suggested change

log.debug("removing current destination file [{}]", fileSystem.getCanonicalName(destination));

log.debug("removing current destination file [{}]", () -> fileSystem.getCanonicalName(destination));

Dat kan niet, want fileSystem.getCanonicalName(x) kan een exception gooien.

Het zou dan dit moeten worden (Wat de log ook veiliger maakt, natuurlijk):

Suggested change

log.debug("removing current destination file [{}]", fileSystem.getCanonicalName(destination));

log.debug("removing current destination file [{}]", ()-> {

try {

return fileSystem.getCanonicalName(destination);

} catch (FileSystemException e) {

return "<Cannot get canonical name for destination file>";

}

});

Eigenlijk is dit wel een issue. Want ik zie dat er verschillende exceptions zijn, waar we deze method ook gebruiken. Dan gaan we straks fouten verbergen met een fout uit deze method... Dat is niet goed.

Ik ga hier wat meer structureels aan doen misschien, een method getNameSafe() of zoiets.

filesystem/src/main/java/org/frankframework/filesystem/LocalFileSystem.java

nielsm5 · 2024-05-09T11:06:50Z

filesystem/src/main/java/org/frankframework/filesystem/LocalFileSystem.java

@@ -109,13 +109,18 @@ public OutputStream appendFile(Path f) throws IOException {

 	@Override
 	public Message readFile(Path f, String charset) throws FileSystemException {
+		if (!f.toFile().exists()) {


Suggested change

if (!f.toFile().exists()) {

if (!Files.exists(p)) {

?

f, of p?

filesystem/src/test/java/org/frankframework/filesystem/FileSystemActorTest.java

filesystem/src/test/java/org/frankframework/filesystem/FileSystemSenderTest.java

nielsm5 · 2024-05-09T13:29:29Z

core/src/main/java/org/frankframework/core/ISender.java

+	@Override
 	void close() throws SenderException;


Ik heb ooit #5045 aangemaakt om open/close te vervangen door start en stop. Mocht een pipe/sender dan zelf toch nog AutoCloseable willen implementeren dan kan dat maar het moet niet een verplichting zijn.

En wil je deze AutoClosable nog weghalen of...?

Ik vind eigenlijk de AutoClosable of een sender nog steeds wel logisch, maar nu weggehaald, zie commit:
cbd3334

core/src/main/java/org/frankframework/pipes/AbstractPipe.java

filesystem/src/main/java/org/frankframework/filesystem/FileSystemActor.java

filesystem/src/main/java/org/frankframework/filesystem/FileSystemException.java

filesystem/src/main/java/org/frankframework/filesystem/IMailFileSystem.java

# Conflicts: # filesystem/src/main/java/org/frankframework/filesystem/FileSystemActor.java

…fix only in LocalFileSystem. Add test to validate behaviour in various scenarios, that were failing before.

…y their temp files, and also test the scenario of root folder that doesn't yet exist before opening the Local FS.

jkosternl · 2024-05-14T10:13:12Z

filesystem/src/main/java/org/frankframework/filesystem/IBasicFileSystem.java

+	 * @param f File for which to try to get canonical name
+	 * @return Either the canonical name of the file, or an error.
+	 */
+	default String getCanonicalNameOrError(F f) {


Suggested change

default String getCanonicalNameOrError(F f) {

default String getCanonicalNameOrErrorMessage(F f) {

Otherwise it might suggest that it gives a canonical name or throws an error.

Ben ik met je eens, die twijfel had ik ook. Maar dit is wel heel lang. Ik dacht aan getCanonicalNameForLog() maar het is niet alleen voor logging, ook voor error messages, dus dat denkt het ook niet.
Eigenlijk vind ik alle namen voor deze method lelijk. Ik houd me open voor betere suggesties.

getCanonicalNameSafely perhaps.
Or getCanonicalNameWithoutDisappointingUncleBob 😉

filesystem/src/test/java/org/frankframework/filesystem/LocalFileSystemActorTest.java

nielsm5

Ik vind nog steeds de closeable op ISender een beetje vreemd

nielsm5 · 2024-05-21T15:54:41Z

filesystem/src/main/java/org/frankframework/filesystem/FileSystemSender.java

-			throw new SenderException(e);
+			String forwardName = e.getForward().getForwardName();
+
+			log.error("Error from FileSystemActor, will return forward name [{}]",forwardName, e);


Suggested change

log.error("Error from FileSystemActor, will return forward name [{}]",forwardName, e);

log.info("error from FileSystemActor, will call forward name [{}]",forwardName, e);

Error level is wel heel erg bot.. dat hoeft toch niet?

Je hebt helemaal gelijk. Ik weet niet waarom daar log.error() stond.

nielsm5 · 2024-05-21T15:56:26Z

core/src/main/java/org/frankframework/core/ISender.java

+	@Override
 	void close() throws SenderException;


En wil je deze AutoClosable nog weghalen of...?

…4933_FileSystemPipeExits # Conflicts: # filesystem/src/main/java/org/frankframework/filesystem/FileSystemActor.java # filesystem/src/main/java/org/frankframework/filesystem/LocalFileSystem.java # filesystem/src/test/java/org/frankframework/filesystem/FileSystemActorTest.java # filesystem/src/test/java/org/frankframework/filesystem/LocalFileSystemActorTest.java

sonarcloud · 2024-05-22T07:27:08Z

Quality Gate failed

Failed conditions
58.4% Coverage on New Code (required ≥ 65%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

jkosternl · 2024-05-22T09:11:59Z

filesystem/src/main/java/org/frankframework/filesystem/FileSystemException.java

@@ -1,5 +1,5 @@
 /*
-   Copyright 2019 Integration Partners
+   Copyright 2019, 2024 Integration Partners


Suggested change

Copyright 2019, 2024 Integration Partners

Copyright 2019, 2024 WeAreFrank!

tnleeuw added 2 commits April 29, 2024 18:55

Draft of changes for more forwards on fs actions, including a number …

f877f9e

…of cleanups in code.

Fixing existing tests.

8e37730

tnleeuw added the Feature label Apr 29, 2024

tnleeuw self-assigned this Apr 29, 2024

tnleeuw linked an issue Apr 29, 2024 that may be closed by this pull request

Cannot create a FileSystemPipe to delete directory that does not yet exist #6597

Closed

tnleeuw added 12 commits May 1, 2024 13:29

Refactoring: make test tearDown safe so it doesn't throw, and conti…

a9ef5d3

…nues teardown all the way.

Fixes for a number of existing tests.

d2981cd

Fix last broken test

0be295d

Fix an accidental breakage in HttpSenderBase that I should not have t…

398b5ed

…ouched in this PR Begin updating tests for FileSystemPipe.

Fix AWS filesystem tests

d29f59e

Apply review comments from Niels

f4a937b

Some refactoring of how forward names are determined, based on review…

84819f7

… feedback.

More consistent use of exception types

42af8b0

Add some test coverage

e09bc6d

Add some more tests, some code cleanups.

b8a5d74

Merge branch 'refs/heads/master' into issue/4933_FileSystemPipeExits

e538303

Some PR cleanups

158979d

tnleeuw marked this pull request as ready for review May 7, 2024 20:19

tnleeuw requested review from jkosternl, nielsm5 and evandongen May 7, 2024 20:19

tnleeuw removed a link to an issue May 8, 2024

Cannot create a FileSystemPipe to delete directory that does not yet exist #6597

Closed

tnleeuw linked an issue May 8, 2024 that may be closed by this pull request

Cannot create a FileSystemPipe to delete directory that does not yet exist #6597

Closed

jkosternl approved these changes May 8, 2024

View reviewed changes

filesystem/src/test/java/org/frankframework/filesystem/smb/Samba2FileSystemTestHelper.java Show resolved Hide resolved

nielsm5 reviewed May 9, 2024

View reviewed changes

evandongen linked an issue May 10, 2024 that may be closed by this pull request

Feature: Directory/File does not exist & Directory/File already exists forwards #4933

Closed

evandongen requested changes May 10, 2024

View reviewed changes

tnleeuw added 3 commits May 13, 2024 10:32

Fix a typo (review comments)

1747e89

Merge branch 'refs/heads/master' into issue/4933_FileSystemPipeExits

2003648

Merge branch 'refs/heads/master' into issue/4933_FileSystemPipeExits

66824af

# Conflicts: # filesystem/src/main/java/org/frankframework/filesystem/FileSystemActor.java

tnleeuw added 6 commits May 13, 2024 17:01

Roll back changes to the folder-creation in FileSystemActor and have …

66450e9

…fix only in LocalFileSystem. Add test to validate behaviour in various scenarios, that were failing before.

Make tests a bit more robust

fe19a93

Merge branch 'refs/heads/master' into issue/4933_FileSystemPipeExits

dc18bf5

Merge branch 'refs/heads/master' into issue/4933_FileSystemPipeExits

ec12c5a

Fix local filesystem tests so they all work, and all clean up properl…

3307686

…y their temp files, and also test the scenario of root folder that doesn't yet exist before opening the Local FS.

Merge branch 'refs/heads/master' into issue/4933_FileSystemPipeExits

679462b

tnleeuw requested review from nielsm5, evandongen and jkosternl May 14, 2024 08:47

jkosternl approved these changes May 14, 2024

View reviewed changes

tnleeuw requested a review from a team May 14, 2024 10:25

evandongen approved these changes May 14, 2024

View reviewed changes

tnleeuw added 2 commits May 14, 2024 21:39

Apply Review Comments

4668421

Fix order of initialisation

24c28f4

nielsm5 approved these changes May 15, 2024

View reviewed changes

Update JavaDocs with the new forwards

647abf2

nielsm5 requested changes May 21, 2024

View reviewed changes

tnleeuw added 4 commits May 22, 2024 08:00

Apply review comments

2e0ab65

Apply review comments -- make ISender no longer extend AutoCloseable

cbd3334

Fix a few things post-merge.

6ff16c7

tnleeuw requested review from jkosternl, evandongen and nielsm5 May 22, 2024 07:35

jkosternl approved these changes May 22, 2024

View reviewed changes

evandongen approved these changes May 22, 2024

View reviewed changes

nielsm5 approved these changes May 22, 2024

View reviewed changes

nielsm5 merged commit 0929856 into master May 22, 2024
12 of 15 checks passed

nielsm5 deleted the issue/4933_FileSystemPipeExits branch May 22, 2024 10:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FileSystemPipe more exits #6740

FileSystemPipe more exits #6740

tnleeuw commented Apr 29, 2024

jkosternl left a comment

nielsm5 May 9, 2024

tnleeuw May 13, 2024

nielsm5 May 9, 2024

tnleeuw May 13, 2024

nielsm5 May 9, 2024

nielsm5 May 21, 2024

tnleeuw May 22, 2024

jkosternl May 14, 2024

tnleeuw May 14, 2024

jkosternl May 14, 2024 •

edited

Loading

nielsm5 left a comment

nielsm5 May 21, 2024

tnleeuw May 22, 2024

nielsm5 May 21, 2024

sonarcloud bot commented May 22, 2024

jkosternl May 22, 2024

	log.debug("removing current destination file [{}]", fileSystem.getCanonicalName(destination));
	log.debug("removing current destination file [{}]", () -> fileSystem.getCanonicalName(destination));

-				log.debug("removing current destination file [{}]", fileSystem.getCanonicalName(destination));
+				log.debug("removing current destination file [{}]", ()-> {
+					try {
+						return fileSystem.getCanonicalName(destination);
+					} catch (FileSystemException e) {
+						return "<Cannot get canonical name for destination file>";
+					}
+				});

	default String getCanonicalNameOrError(F f) {
	default String getCanonicalNameOrErrorMessage(F f) {

	log.error("Error from FileSystemActor, will return forward name [{}]",forwardName, e);
	log.info("error from FileSystemActor, will call forward name [{}]",forwardName, e);

	Copyright 2019, 2024 Integration Partners
	Copyright 2019, 2024 WeAreFrank!

FileSystemPipe more exits #6740

FileSystemPipe more exits #6740

Conversation

tnleeuw commented Apr 29, 2024

jkosternl 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

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

jkosternl May 14, 2024 • edited Loading

Choose a reason for hiding this comment

nielsm5 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

sonarcloud bot commented May 22, 2024

Quality Gate failed

Choose a reason for hiding this comment

jkosternl May 14, 2024 •

edited

Loading