feat(webserver): improve namespace list in files editor #2457

Skraye · 2023-11-06T20:54:18Z

Now include namespace that have files & namespace that have flows (or both!)

I've started a flowUtils for test to avoid duplicated utils methods in every controller, but will do another PR as its unrelated to this one.
edit:
close #2452

brian-mulier-p · 2023-11-07T20:05:12Z

webserver/src/main/java/io/kestra/webserver/controllers/NamespaceFileController.java

+ }
+
+ protected List<String> distinctNamespacesFiles() throws IOException, URISyntaxException {
+ return storageInterface.list(tenantService.resolveTenant(), new URI(""))


It doesn't work with S3, idk for GCS, idk if that's the case for localstorage but for eg. in S3 you can't list "" & "/", not sure if it's an implementation issue or a SDK limitation, that's an issue we should adress quickly 🤔

Btw I think we have an issue as the extension is creating namespace file without a leading '/' and it seems to have an impact on some storage (s3 does, idk for gcs). We might need to discuss whether it is a problem and if we should keep the forward '/' also for namespace files (currently it is only the case for every other files)

This change should be made to fix this but we might need to discuss about it

We should never list all the internal storage like that.
What you want if I understood, is to list all namespaces that have files. But this is not what you did here as namespaces can have execution data but not files, and in this case, you will still include it in the list.

Moreover, listing like this will bypass security checks on EE so it's really not a good idea.

brian-mulier-p · 2023-11-07T20:18:20Z

webserver/src/main/java/io/kestra/webserver/controllers/NamespaceFileController.java

+ public List<String> distinctNamespaces() throws IOException, URISyntaxException {
+ List<String> storageNamespaces = distinctNamespacesFiles();
+ List<String> flowNamespaces = flowRepository.findDistinctNamespace(tenantService.resolveTenant());
+
+ return Stream.concat(storageNamespaces.stream(), flowNamespaces.stream()).distinct().toList();
+ }


as in the comment of the issue, I'm not sure the merge should be done at the backend level as it might be interesting for admin purpose to be able to list exclusively namespaces with files

Yes, those are two very different things: flows and namespace files, and we want to display both of them in the editor. So it's the responsibility of the editor to merge them.

loicmathieu · 2023-11-08T08:08:39Z

webserver/src/main/java/io/kestra/webserver/controllers/NamespaceFileController.java

+ public List<String> distinctNamespaces() throws IOException, URISyntaxException {
+ List<String> storageNamespaces = distinctNamespacesFiles();
+ List<String> flowNamespaces = flowRepository.findDistinctNamespace(tenantService.resolveTenant());
+
+ return Stream.concat(storageNamespaces.stream(), flowNamespaces.stream()).distinct().toList();
+ }


Yes, those are two very different things: flows and namespace files, and we want to display both of them in the editor. So it's the responsibility of the editor to merge them.

loicmathieu · 2023-11-08T08:12:07Z

webserver/src/main/java/io/kestra/webserver/controllers/NamespaceFileController.java

+ }
+
+ protected List<String> distinctNamespacesFiles() throws IOException, URISyntaxException {
+ return storageInterface.list(tenantService.resolveTenant(), new URI(""))


We should never list all the internal storage like that.
What you want if I understood, is to list all namespaces that have files. But this is not what you did here as namespaces can have execution data but not files, and in this case, you will still include it in the list.

Moreover, listing like this will bypass security checks on EE so it's really not a good idea.

loicmathieu · 2023-11-10T10:01:56Z

webserver/src/main/java/io/kestra/webserver/controllers/EditorController.java

+ @Inject NamespaceFileController namespaceFileController;
+ @Inject FlowController flowController;


I would not use controller inside of controllers, this is kind of strange and breaks some clean architecture rules.
Controllers are here to deals with the HTTP layer, by directly calling it's method out of an HTTP context you may breaks things.

Better to use the repository here.

loicmathieu · 2023-11-10T10:02:10Z

webserver/src/test/java/io/kestra/webserver/controllers/NamespaceFileControllerTest.java

@@ -2,6 +2,7 @@

 import io.kestra.core.storages.FileAttributes;
 import io.kestra.core.storages.StorageInterface;
+import io.kestra.core.utils.IdUtils;


Unused import

loicmathieu · 2023-11-10T10:03:24Z

webserver/src/main/java/io/kestra/webserver/controllers/NamespaceFileController.java

+ return storageInterface.list(tenantService.resolveTenant(), null)
+ .stream()


Already said (and explained why) it's a bad idea but I will not block the PR for that if nobody else object.

…oid using controllers

loicmathieu

Still think it's not a good idea to list all internal storage root directories like that but I will not argue anymore about it.

loicmathieu · 2023-11-10T10:44:28Z

webserver/src/main/java/io/kestra/webserver/controllers/NamespaceFileController.java

@@ -20,12 +21,15 @@
 import lombok.extern.slf4j.Slf4j;

 import java.io.BufferedInputStream;
-import java.io.FileNotFoundException;
+import java.io.ByteArrayInputStream;


This import didn't seems to be used anywhere

Skraye · 2023-11-15T10:02:49Z

It will probably be done in the next release as for now, there is no easy way to list namespaceFiles following the refacto

Skraye requested a review from brian-mulier-p November 6, 2023 20:54

Skraye self-assigned this Nov 6, 2023

Skraye force-pushed the feat/improve-files-editor-select branch from e5cb09d to 6365a2b Compare November 7, 2023 08:08

brian-mulier-p requested changes Nov 7, 2023

View reviewed changes

loicmathieu requested changes Nov 8, 2023

View reviewed changes

brian-mulier-p mentioned this pull request Nov 8, 2023

fix/recursive-dir-creation kestra-io/storage-s3#25

Merged

Skraye force-pushed the feat/improve-files-editor-select branch from 6365a2b to 15c0b98 Compare November 9, 2023 08:23

Skraye added 2 commits November 10, 2023 10:45

feat(webserver): improve namespace list in files editor

979836c

feat(): review changes

80c9188

Skraye force-pushed the feat/improve-files-editor-select branch from 15c0b98 to 80c9188 Compare November 10, 2023 09:56

loicmathieu requested changes Nov 10, 2023

View reviewed changes

fix(): review changes : created a service for storageInterface and av…

0992606

…oid using controllers

loicmathieu approved these changes Nov 10, 2023

View reviewed changes

tchiotludo force-pushed the develop branch from e64aeea to 34c2f8f Compare December 2, 2023 12:33

anna-geller modified the milestones: v0.14.0, v0.15.0, v0.16.0 Dec 4, 2023

tchiotludo force-pushed the develop branch from d88dc0c to 6699436 Compare December 8, 2023 20:59

tchiotludo force-pushed the develop branch 2 times, most recently from 2dc2842 to ab62ba9 Compare January 21, 2024 10:55

tchiotludo force-pushed the develop branch 3 times, most recently from febd835 to 51d21c8 Compare February 27, 2024 12:04

Skraye force-pushed the develop branch from fd9c986 to 60e1eca Compare March 12, 2024 17:57

tchiotludo force-pushed the develop branch from 95cd4de to 400b2f8 Compare March 21, 2024 22:44

anna-geller modified the milestones: v0.16.0, v0.17.0 Apr 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(webserver): improve namespace list in files editor #2457

feat(webserver): improve namespace list in files editor #2457

Skraye commented Nov 6, 2023 •

edited

brian-mulier-p Nov 7, 2023 •

edited

loicmathieu Nov 8, 2023

brian-mulier-p Nov 7, 2023

loicmathieu Nov 8, 2023

loicmathieu Nov 8, 2023

loicmathieu Nov 8, 2023

loicmathieu Nov 10, 2023

loicmathieu Nov 10, 2023

loicmathieu Nov 10, 2023

loicmathieu left a comment

loicmathieu Nov 10, 2023

Skraye commented Nov 15, 2023

		@Inject NamespaceFileController namespaceFileController;
		@Inject FlowController flowController;

		return storageInterface.list(tenantService.resolveTenant(), null)
		.stream()

feat(webserver): improve namespace list in files editor #2457

Are you sure you want to change the base?

feat(webserver): improve namespace list in files editor #2457

Conversation

Skraye commented Nov 6, 2023 • edited

brian-mulier-p Nov 7, 2023 • edited

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

loicmathieu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skraye commented Nov 15, 2023

Skraye commented Nov 6, 2023 •

edited

brian-mulier-p Nov 7, 2023 •

edited