Soft delete projects by moving them to trash #10440

4e6 · 2024-07-03T17:41:01Z

Pull Request Description

close #10357

Changelog:

add: LinuxTrashBin implementing Freedesktop.org trash specification
add: WindowsTrashBin calling native platform APIs for Windows
add: MacTrashBin calling native platform APIs for MacOS
update: project/delete method moves projects to trash and falls back to directory deletion

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
All code follows the
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
Unit tests have been written where possible.

Akirathan

Unit tests are fine, but in this case, we also need some QA. Please, ensure that the deleted project can be restored (or at least seen) in the trash on all the platforms.

Akirathan · 2024-07-08T09:32:54Z

lib/java/desktop-environment/src/test/java/org/enso/desktopenvironment/TrashTest.java

+      Assert.assertTrue(trash.moveToTrash(path));
+      Assert.assertFalse(trash.moveToTrash(path));
+    } finally {
+      FileUtils.deleteQuietly(path.toFile());


You can use @Rule TemporaryFolder just like in https://github.com/enso-org/enso/blob/develop/engine/runtime-integration-tests/src/test/java/org/enso/interpreter/test/exports/ExportExtensionMethodTest.java#L23 and avoid the need to delete the path yourself - JUnit runner will do that for you.

JaroslavTulach · 2024-07-26T06:17:45Z

Update PR description:

close #10357

Changelog:

add: LinuxTrash implementing Freedesktop.org trash specification
add: JnaTrash calling native platform APIs for Windows and macOS
update: project/delete method moves projects to trash and falls back to directory deletion
update: native image configuration with the JNA dependency

It is out of date.

JaroslavTulach

RandomUtils seem over complicated and potentially replaceable by UUID. Please update the PR description.

JaroslavTulach · 2024-07-26T06:22:53Z

lib/java/desktop-environment/src/main/java/org/enso/desktopenvironment/RandomUtils.java

+      throw new IllegalArgumentException("String size should be positive.");
+    }
+
+    var random = LazyRandom.getInstance();


Why don't we just use random = new Random()?

To initialize it once

Why would you need to initialize once? For purposes of randomness, initializing again and again is completely OK unless you expect multiple random values to be needed during the same millisecond.

JaroslavTulach · 2024-07-26T06:25:34Z

lib/java/desktop-environment/src/main/java/org/enso/desktopenvironment/LinuxTrashBin.java

+            trashInfo,
+            toDelete,
+            contents,
+            RandomUtils.alphanumericString(SUFFIX_SIZE),


It is quite common in Enso to use UUID - which is usually created randomly based on various conditions including time seed. Is not it easier to remove RandomUtils and just ask for UUID?

hubertp

Approving changes that I haven't committed to this PR. @4e6 please review our work on SVM C's interface.

hubertp · 2024-07-26T11:56:56Z

@JaroslavTulach As expected, unit tests on Windows and Mac fail because C's interface is only compiled for native image.
We should either disable those tests for non-Linux or add some workaround in those cases (you mentioned you had some idea?). Alternatively bring back JNA just for testing but I'm not sure it is worth it.

JaroslavTulach · 2024-07-26T14:34:32Z

@JaroslavTulach As expected, unit tests on Windows and Mac fail because C's interface is only compiled for native image.

NullPointerException - that's great! We could also get some LinkageError... NPE is easy to recover from.

Can we recover from the exception and fallback to File.delete()?

If you want to check we run in AOT mode and only fallback to File.delete() in such case, there is Boolean.getBoolean("com.oracle.graalvm.isaot") to verify that.

Alternatively bring back JNA just for testing but I'm not sure it is worth it.

No, rather not.

4e6 · 2024-07-26T16:17:38Z

lib/java/desktop-environment/src/main/java/org/enso/desktopenvironment/MacTrashBin.java

+              .warn(
+                  "Moving to MacOS's Trash Bin is not supported in non-AOT mode. Deleting"
+                      + " permanently");
+          return path.toFile().delete();


It should return false. Right now the project manager logic removes the project directory permanently if was unable to move it to trash bin.

4e6 · 2024-07-26T16:22:17Z

lib/java/desktop-environment/src/main/java/org/enso/desktopenvironment/Platform.java

+  private static final Platform OPERATING_SYSTEM = detectOperatingSystem();
+
+  /**
+   * @GuardedBy("this")


It is not a javadoc annotation

4e6 · 2024-07-26T16:23:37Z

lib/java/desktop-environment/src/main/java/org/enso/desktopenvironment/RandomUtils.java

+      throw new IllegalArgumentException("String size should be positive.");
+    }
+
+    var random = LazyRandom.getInstance();


To initialize it once

4e6 · 2024-07-26T16:27:24Z

lib/java/desktop-environment/src/main/java/org/enso/desktopenvironment/MacTrashBin.java

+
+  @Override
+  public boolean moveToTrash(Path path) {
+    if (Platform.getOperatingSystem().isMacOs())


I'd say this check is unnecessary because the class is not exposed, and its instantiation depends on the Platform.getOperatingSystem(). I.e. the factory method guarantees the correct OS

It would fail native image build otherwise.

4e6 · 2024-07-26T16:27:55Z

lib/java/desktop-environment/src/main/java/org/enso/desktopenvironment/WindowsTrashBin.java

+
+  @Override
+  public boolean moveToTrash(Path path) {
+    if (Platform.getOperatingSystem().isWindows())


I'd say unnecessary, the same way as for macOS

Same reason as for MacOS - native image build.

4e6 · 2024-07-26T16:28:35Z

lib/java/desktop-environment/src/main/java/org/enso/desktopenvironment/WindowsTrashBin.java

+              .warn(
+                  "Moving to Windows' Trash Bin is not supported in non-AOT mode. Deleting"
+                      + " permanently");
+          return path.toFile().delete();


Just return false, the project manager logic will delete the project directory permanently as a fallback action.

4e6 · 2024-07-26T16:30:16Z

lib/scala/project-manager/src/main/scala/org/enso/projectmanager/service/ProjectService.scala

+        ifTrue = log.info("Project moved to trash [{}].", projectId),
+        ifFalse =
+          log.info("Failed to trash the project. Deleting [{}].", projectId) *>
+          deleteProject *>


Here is the fallback logic that deletes the project permanently

JaroslavTulach · 2024-07-27T05:49:16Z

lib/java/desktop-environment/src/main/java/org/enso/desktopenvironment/TrashBinFallback.java

+import java.util.stream.Collectors;
+import org.slf4j.Logger;
+
+public class TrashBinFallback {


This class shouldn't be public.

Suggested change

public class TrashBinFallback {

class TrashBinFallback {

JaroslavTulach · 2024-07-27T05:53:03Z

lib/java/desktop-environment/src/main/java/org/enso/desktopenvironment/WindowsTrashBin.java

@@ -15,7 +15,7 @@
 import org.slf4j.LoggerFactory;

 @CContext(WindowsTrashBin.ShellApi.class)
-final class WindowsTrashBin implements TrashBin {
+final class WindowsTrashBin extends TrashBinFallback implements TrashBin {


Using inheritance for implementation purposes not for any meaningful type hierarchy. What would Lišková said about it?

I would personally make hardDeletePath a static utility method in some existing class. But if you hide all these classes from the Javadoc, then I don't care.

JaroslavTulach

Try sbt desktop-environment/doc and verify it doesn't contain unnecessary elements. Otherwise good, get this in.

4e6 · 2024-07-27T13:19:36Z

Windows build works correctly for me

hubertp · 2024-07-28T13:46:57Z

I don't think disabling tests for Mac and Windows was desirable. Pretty sure folks will try it in dev mode and complain that we don't delete directories. Hard delete was better.

4e6 · 2024-07-29T07:27:40Z

Pretty sure folks will try it in dev mode and complain that we don't delete directories. Hard delete was better.

Hard delete will work in dev mode

enso/lib/scala/project-manager/src/main/scala/org/enso/projectmanager/service/ProjectService.scala

Lines 158 to 163 in 73cb5d1

    
           _ <- CovariantFlatMap[F].ifM(moveProjectToTrash)( 
        
             ifTrue = log.info("Project moved to trash [{}].", projectId), 
        
             ifFalse = 
        
               log.info("Failed to trash the project. Deleting [{}].", projectId) *> 
        
               deleteProject *> 
        
               log.info("Project deleted [{}]", projectId)

JaroslavTulach · 2024-07-29T07:30:38Z

lib/java/desktop-environment/src/main/java/org/enso/desktopenvironment/RandomUtils.java

+
+    private LazyRandom() {}
+
+    public static Random getInstance() {


If you really want to...

To initialize it once

... then this method is missing synchronized modifier.

JaroslavTulach · 2024-07-29T07:31:20Z

lib/java/desktop-environment/src/main/java/org/enso/desktopenvironment/RandomUtils.java

+  private RandomUtils() {}
+
+  /** Delay the {@link Random} instance initialization when building the native image. */
+  private static final class LazyRandom {


Delay the {@link Random} instance initialization when building the native image.

Remove LazyRandom class. There is no point having it.

4e6 added 2 commits July 3, 2024 18:30

feat: trash

f8f0c08

feat: move project to trash

0e580d6

4e6 added the CI: No changelog needed Do not require a changelog entry for this PR. label Jul 3, 2024

4e6 self-assigned this Jul 3, 2024

4e6 added 4 commits July 4, 2024 16:26

fix: TrashTest on Windows

8ffa14f

fix: native image

b0ff163

Merge branch 'develop' into wip/db/10357-add-recycle

569261f

fix: after merge

aa555cc

4e6 changed the title ~~Soft delete project by moving them to trash~~ Soft delete projects by moving them to trash Jul 4, 2024

4e6 added 3 commits July 5, 2024 08:06

DEBUG: lazy trash initialization

ff4cbec

DEBUG: trash initialization

3c90113

DEBUG: awt initialization

80dd6c8

Akirathan approved these changes Jul 8, 2024

View reviewed changes

4e6 added 5 commits July 8, 2024 17:37

misc: javafmt

df166a7

misc: properties setup

5fcd03c

fix: native image

efd9e05

feat: jna trash

bf177bf

feat: native image resources

170284b

4e6 force-pushed the wip/db/10357-add-recycle branch from 536515b to 170284b Compare July 9, 2024 14:49

4e6 added 4 commits July 9, 2024 16:04

misc: cleanup properties setup

b0d72f0

misc: legal review

e97b44d

Merge branch 'develop' into wip/db/10357-add-recycle

1d296ef

fix: after merge

9aa46a8

4e6 marked this pull request as ready for review July 9, 2024 17:27

4e6 requested review from JaroslavTulach, radeusgd, hubertp, jdunkerley, GregoryTravis and AdRiley as code owners July 9, 2024 17:27

hubertp added 3 commits July 25, 2024 22:28

Merge branch 'develop' into wip/db/10357-add-recycle

5a7d9c8

remove jna

961aa6c

update license

ccb8bd5

hubertp requested a review from JaroslavTulach July 25, 2024 20:54

nit

0de3f1e

JaroslavTulach reviewed Jul 26, 2024

View reviewed changes

hubertp added 2 commits July 26, 2024 10:42

fmt

2301840

Fix tests that broke during Platform refactoring

114cdca

hubertp approved these changes Jul 26, 2024

View reviewed changes

enso-bot bot mentioned this pull request Jul 26, 2024

Unable to download from subfolders #10453

Closed

Workaround for non-AOT mode

20b892f

4e6 commented Jul 26, 2024

View reviewed changes

deal with non-empty directories for fallback delete

c5aee4a

enso-bot bot mentioned this pull request Jul 27, 2024

Avoid race condition in GC_Example test #10665

Merged

2 tasks

JaroslavTulach reviewed Jul 27, 2024

View reviewed changes

JaroslavTulach approved these changes Jul 27, 2024

View reviewed changes

fix: tests

f6d66ab

4e6 added the CI: Ready to merge This PR is eligible for automatic merge label Jul 27, 2024

mergify bot merged commit 446834b into develop Jul 27, 2024
42 checks passed

mergify bot deleted the wip/db/10357-add-recycle branch July 27, 2024 13:37

JaroslavTulach reviewed Jul 29, 2024

View reviewed changes

enso-bot bot mentioned this pull request Jul 30, 2024

Jobs appear to be stuck on Base_Tests #10643

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Soft delete projects by moving them to trash #10440

Soft delete projects by moving them to trash #10440

4e6 commented Jul 3, 2024 •

edited by hubertp

Loading

Akirathan left a comment

Akirathan Jul 8, 2024

JaroslavTulach commented Jul 26, 2024

JaroslavTulach left a comment

JaroslavTulach Jul 26, 2024

4e6 Jul 26, 2024

JaroslavTulach Jul 29, 2024

JaroslavTulach Jul 26, 2024

hubertp left a comment

hubertp commented Jul 26, 2024

JaroslavTulach commented Jul 26, 2024

4e6 Jul 26, 2024

4e6 Jul 26, 2024

4e6 Jul 26, 2024

4e6 Jul 26, 2024

hubertp Jul 28, 2024

4e6 Jul 26, 2024

hubertp Jul 28, 2024

4e6 Jul 26, 2024

4e6 Jul 26, 2024

JaroslavTulach Jul 27, 2024

JaroslavTulach Jul 27, 2024

JaroslavTulach left a comment

4e6 commented Jul 27, 2024

hubertp commented Jul 28, 2024 •

edited

Loading

4e6 commented Jul 29, 2024

JaroslavTulach Jul 29, 2024

JaroslavTulach Jul 29, 2024

Soft delete projects by moving them to trash #10440

Soft delete projects by moving them to trash #10440

Conversation

4e6 commented Jul 3, 2024 • edited by hubertp Loading

Pull Request Description

Important Notes

Checklist

Akirathan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach commented Jul 26, 2024

JaroslavTulach 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

hubertp left a comment

Choose a reason for hiding this comment

hubertp commented Jul 26, 2024

JaroslavTulach commented Jul 26, 2024

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

Choose a reason for hiding this comment

JaroslavTulach left a comment

Choose a reason for hiding this comment

4e6 commented Jul 27, 2024

hubertp commented Jul 28, 2024 • edited Loading

4e6 commented Jul 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

4e6 commented Jul 3, 2024 •

edited by hubertp

Loading

hubertp commented Jul 28, 2024 •

edited

Loading