feat(UI): File output preview #1770

Skraye · 2023-07-18T10:09:52Z

loicmathieu

I think the behaviour should change: we should only supported a fixed set of extensions and if a file is not of this extension didn't show the button and generates an error in the backend. We can revisit this later but it is preferable for security, performance (some file may not be multi-line) and usability (binary may crash the browser).

Also, I think the preview may be better on a modal on the centre instead of the "coming from right" modal but it can wait for some redesign.

loicmathieu · 2023-07-18T10:43:19Z

ui/src/translations.json

-    "dependency task": "Task {taskId} is a dependency of another task"
+    "dependency task": "Task {taskId} is a dependency of another task",
+    "preview": "Preview",
+    "open": "Open"


Show is better than Open

loicmathieu · 2023-07-18T10:45:16Z

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

+
+        String extension = FilenameUtils.getExtension(path.toString());
+
+        if (ImageFileExtension.isImageFileExtension(extension)) {


You can use a simple List of extensions as you didn't really use the enum

loicmathieu · 2023-07-18T10:46:41Z

ui/src/components/executions/FilePreview.vue

+                case "png":
+                case "svg":
+                    return "image";
+                default:


You should not default to text, this should be an extension like the other and default to not preview.
This can be a security risk to allow any extension file.
And this can crash Kestra UI for example if a binary format with non-displayable characters is used.

I don't see how it can be a security risk as I just display character. The only difference is that if I don't know the extension, there will be no syntax highlighting.

Well security issues can hide in multiple way.

Anyway, as I commented somewhere else, binary format may crash the browser, and some files may also be one long line that (avro, parquet, ...) so better only display what we are sure can be displayed.

loicmathieu · 2023-07-18T10:47:29Z

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

+            return httpResponse;
+        }
+
+        InputStream fileStream = storageInterface.get(path);


You should restrict the loading of files only to supported extensions and throws an error for the other.

webserver/src/main/java/io/kestra/webserver/utils/ImageFileExtension.java

ui/src/components/executions/FilePreview.vue

loicmathieu · 2023-07-18T11:27:06Z

ui/src/components/executions/FilePreview.vue

+                case "svg":
+                    return "data:image/" + this.extension + ";base64," + this.content;
+                default:
+                    return this.content;


not a good idea as some files may not be displayable.

ui/src/components/executions/FilePreview.vue

loicmathieu · 2023-07-18T11:31:40Z

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

+
+        StringBuilder contentBuilder = new StringBuilder();
+
+        while (line != null && lineCount < 100) {


Not all files are multi-line, you can have a huge file with a one-in JSON in it.
I'm not sure we can do something better than that anyway

One solution would be to limit the size of the content return, limit to a size of 2mb for example so even if my file is a heavy one-line, I will return limited content

Agree that we need an limit in MB or number of lines, the first that will cut of

Skraye · 2023-07-18T14:13:05Z

@loicmathieu
Added size limit so no issue with one line file
I tried with file like binary, it's just not readable but no issue

loicmathieu · 2023-07-31T10:06:03Z

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

+            }
+        }
+
+        return HttpResponse.ok(PreviewResponse.builder().extension(extension).content(truncateStringSize(contentBuilder.toString())).build());


Truncating the file after it has been read is not a good idea as the file will still be loaded entirely in memory.

loicmathieu · 2023-07-31T10:07:29Z

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

+
+    public String truncateStringSize(String content) {
+        // Equivalent to 2MB
+        int maxSizeInBytes = 2097152;


Please use a constant.
A good practice is to declare it something like:

private int TWO_MB = 2 * 1024 * 1024;

ui/src/translations.json

tchiotludo

please:

move the rendering of the data on the server side
fix an issue where multiple drawer will be open if I have multiple output preview on the same execution

close #1767

Skraye requested review from loicmathieu and brian-mulier-p July 18, 2023 10:09

Skraye self-assigned this Jul 18, 2023

loicmathieu requested changes Jul 18, 2023

View reviewed changes

loicmathieu reviewed Jul 18, 2023

View reviewed changes

Skraye force-pushed the feat/file-preview branch 2 times, most recently from 7b47dc3 to 5d68498 Compare July 18, 2023 14:29

brian-mulier-p removed their request for review July 20, 2023 15:13

Skraye force-pushed the feat/file-preview branch from 5d68498 to 9bb2f3c Compare July 26, 2023 14:27

Skraye requested a review from loicmathieu July 26, 2023 14:28

loicmathieu requested changes Jul 31, 2023

View reviewed changes

tchiotludo force-pushed the feat/file-preview branch from 81e4e93 to 567495f Compare August 7, 2023 21:48

tchiotludo requested changes Aug 7, 2023

View reviewed changes

tchiotludo force-pushed the feat/file-preview branch 3 times, most recently from e2261fc to a03c61d Compare August 8, 2023 08:58

Skraye force-pushed the feat/file-preview branch 2 times, most recently from d71c407 to ac78a83 Compare August 16, 2023 14:15

Skraye and others added 5 commits August 17, 2023 22:28

feat(UI): File output preview

d0ee725

close #1767

fix(): review changes

7ccfa21

feat(): added preview on input files

2c472d8

layout fix

1f20a64

feat(): Now build displayed data on backend

8a00d5a

tchiotludo force-pushed the feat/file-preview branch from ac78a83 to 8a00d5a Compare August 17, 2023 20:29

cleanup

99eef9c

tchiotludo force-pushed the feat/file-preview branch from 69ed71e to 99eef9c Compare August 17, 2023 20:59

tchiotludo merged commit eab705b into develop Aug 17, 2023

tchiotludo deleted the feat/file-preview branch August 17, 2023 21:40

yuri1969 mentioned this pull request Oct 2, 2023

Improve the constraint of file preview #2222

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(UI): File output preview #1770

feat(UI): File output preview #1770

Skraye commented Jul 18, 2023

loicmathieu left a comment

loicmathieu Jul 18, 2023

loicmathieu Jul 18, 2023

loicmathieu Jul 18, 2023

Skraye Jul 18, 2023

loicmathieu Jul 18, 2023

loicmathieu Jul 18, 2023

loicmathieu Jul 18, 2023

loicmathieu Jul 18, 2023

Skraye Jul 18, 2023

tchiotludo Aug 7, 2023

Skraye commented Jul 18, 2023

loicmathieu Jul 31, 2023

tchiotludo Aug 7, 2023

loicmathieu Jul 31, 2023

tchiotludo left a comment


		String extension = FilenameUtils.getExtension(path.toString());

		if (ImageFileExtension.isImageFileExtension(extension)) {


		StringBuilder contentBuilder = new StringBuilder();

		while (line != null && lineCount < 100) {

feat(UI): File output preview #1770

feat(UI): File output preview #1770

Conversation

Skraye commented Jul 18, 2023

loicmathieu 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

Choose a reason for hiding this comment

Skraye commented Jul 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchiotludo left a comment

Choose a reason for hiding this comment