feature/#392_Unpack_gz_files_and_#391follow_redirects #405

A-M-A-X · 2023-07-21T08:39:37Z

[FEATURE] HTTPExtractor should follow redirects #391

Added 'gz' file-type to Archive-Interpreter
- Implemented with zlib
- Added Testfile thermoelectricMaterials.jv
  - only showcases that File-Picker receives a file - error on csv interpreter, as underlying file is in json format
    Closes [FEATURE] Unpack gz files #392

[FEATURE] Unpack gz files #392

Added redirect logic to HTTP-Extractor
- Used follow-redirects -package
- Used default number of redirects from package (max redirects = 21)
- followRedirects attribute in HTTP-Extractor defaults to true
- Added Testfile materials.jv
  Closes [FEATURE] HTTPExtractor should follow redirects #391

relates to jvalue#381

Update docs to be in line with working code

…pective logic to http-extractor-executor. Added jv test file for redirects and added file to automated tests.

… feature/392-Unpack-gz-files

…p-extractor on redirects

Feature/392 unpack gz files

georg-schwarz · 2023-07-21T08:49:36Z

@f3l1x98 can you provide guidance on how the failing example file can be turned into a unit test?

rhazn

I know you did not tag me in this to review but it popped up in my email. I left some spontaneous commments. For future work, it would be good to have only one PR per issue please.

rhazn · 2023-07-21T08:45:29Z

example/thermoelectricMaterials.jv

I don't think we need new examples in the Jayvee repository right now, it would be better to move this to the SWC repo.

Also please don't provide examples that do not work. These files are meant as documentation for users of Jayvee, not to test implementation ;).

rhazn · 2023-07-21T08:45:33Z

example/materials.jv

I don't think we need new examples in the Jayvee repository right now, it would be better to move this to the SWC repo.

rhazn · 2023-07-21T08:50:43Z

libs/extensions/std/exec/src/archive-interpreter-executor.ts

    return R.err({
      message: `Archive is not a zip-archive`,
      diagnostic: { node: context.getCurrentNode(), property: 'name' },
    });
  }

+  private async loadGzFileToInMemoryFileSystem(


This function should duplicate a lot of the loadZipFileToInMemoryFileSystem logic. I assume only the extraction of files would be different, putting them into our in memory filesystem etc is all repeated logic.

It would be better to implement it that way, e.g., by extracting the shared logic of putting files into our in-memory file system into one function and reduce these two functions to just extracting the relevant archives and then using the new function to put files into a file system.

rhazn · 2023-07-21T08:51:36Z

libs/extensions/std/exec/src/archive-interpreter-executor.ts

+        archivedObject,
+      );
+
+      const addedFile = fs.putFile(


I am very confused how this works because I can only see one putFile call and no loops or iterations. Does this actually correctly expand gz archives with multiple files in them?

To my understanding gzip is used to compress individual files. To compress multiple files with gzip, they have to be concated first, e.g to a .tar file. This I would implement in a different feature. Happy to further look into this, if you disagree.

rhazn · 2023-07-21T08:52:18Z

libs/extensions/std/lang/src/archive-interpreter-meta-inf.ts

@@ -19,7 +19,7 @@ export class ArchiveInterpreterMetaInformation extends BlockMetaInformation {
        archiveType: {
          type: PrimitiveValuetypes.Text,
          docs: {
-            description: 'The archive type to be interpreted, e.g., `"zip"`.',
+            description: 'The archive type to be interpreted, e.g., `"zip" or "gz`.',


Suggested change

description: 'The archive type to be interpreted, e.g., `"zip" or "gz`.',

description: 'The archive type to be interpreted, e.g., `"zip" or "gz".',

Changed to 'The archive type to be interpreted, e.g., "zip" or "gz".',

rhazn · 2023-07-21T08:54:32Z

libs/extensions/std/lang/src/http-extractor-meta-inf.ts

+          type: PrimitiveValuetypes.Boolean,
+          defaultValue: true,
+          docs: {
+            description: 'Indicates, whether to follow redirects on get requests. If `false`, Redirects are disabled. Default `true`',


Redirects are not disabled, just not followed ;).

Suggested change

description: 'Indicates, whether to follow redirects on get requests. If `false`, Redirects are disabled. Default `true`',

description: 'Indicates, whether to follow redirects on get requests. If `false`, redirects are not followed. Default `true`',

Changed as suggested

rhazn · 2023-07-21T08:55:31Z

libs/extensions/std/exec/src/http-extractor-executor.ts

@@ -126,6 +126,17 @@ export class HttpExtractorExecutor extends AbstractBlockExecutor<
          );
        }

+        if (responseCode === 302) {


301 is also a redirect code I assume.

Changed to >= 301, as 300 is used for 'multiple choices'. The remaining 300 < response codes < 400 point to redirect location or are not used in get context.

Please do not change it to >= 301, that makes it dependent on other code to ensure this does not handle 400+. You can change it to >=301 and <400 though.

rhazn · 2023-07-21T13:00:40Z

Linting is failing for this PR btw, you can run the linter locally with npm run lint.

f3l1x98 · 2023-07-21T13:48:33Z

@f3l1x98 can you provide guidance on how the failing example file can be turned into a unit test?

If help is needed/wanted, yes.
Though we currently do not have any tests yet, where this could be tested.
I think this would be part of either the test for the CSVInterpreterBlockExecutor or End-to-End tests, but would have to look into it.

incorporated feedback from PR

rhazn · 2023-07-26T16:42:56Z

@A-M-A-X (in case you want me to review this again, you'll need to re-request the review on the top right, little icon next to my name).

rhazn

Small changes to the function still but otherwise looks good 👍

rhazn · 2023-07-28T08:45:01Z

libs/extensions/std/exec/src/archive-interpreter-executor.ts

    }
  }
+
+  private createBinaryAndPutFile(


I think there are two problems with this:

Methods that have an "and" in their name are almost always to large and can be split up in an obvious way ;). In this case, I'd split this into createBinary and putFile theoretically.

Changing objects that are parameters to a function as a sideeffect is often confusing and leads to errors. Here I would not change the inmemoryfilesystem as sideeffect of the function.

Combined, I'd rename the method to createFileFromArchive (or similar) and just have line 120 to 128 in it, return the new BinaryFile. Then habe the fs.putfile and assert outside of the function on it's own, I don't think they need their own function.

Moved fs.putFile and assertion outside of function.

Renamed function to createFileFromArchive and have it return the new BinaryFile.

Removed now unused parameters.

…ive. Moved putFile logic outside

rhazn · 2023-07-28T11:13:46Z

Wait

rhazn · 2023-07-28T11:14:27Z

Ah, nvm I am blind. It's fine, you can merge this into dev.

A-M-A-X · 2023-07-28T11:35:03Z

@rhazn I think I am missing write acess to do the merge myself, or am I blind as well?

rhazn · 2023-07-28T12:08:46Z

Ah yeah might be on me. It's fine, I can merge these finally.

rhazn and others added 13 commits June 28, 2023 09:53

Update docs to be in line with working code

71f22c7

relates to jvalue#381

Merge pull request jvalue#382 from jvalue/update-docs-381

1124884

Update docs to be in line with working code

Added 'followRedirects' property to http-extractor-meta and added res…

3a13303

…pective logic to http-extractor-executor. Added jv test file for redirects and added file to automated tests.

changed formatting

e97b0a6

initial commit. ExampleJV was only initially set up

a83e6f6

Merge branch 'feature/391-HTTPExtractor-should-follow-redirects' into…

561c088

… feature/392-Unpack-gz-files

Added gzip decompress to archive-interpreter and fixed issue with htt…

6149013

…p-extractor on redirects

added to description, format and tests

7b963ad

removed unused imports

a23642a

Merge pull request #1 from A-M-A-X/feature/392-Unpack-gz-files

1eb6e1b

Feature/392 unpack gz files

Merge remote-tracking branch 'jvalue/jayvee/origin/dev'

b119c5e

formatted code

5f55fb9

format

8d0d124

rhazn reviewed Jul 21, 2023

View reviewed changes

removed jv example files

9fe5645

A-M-A-X added 5 commits July 26, 2023 14:19

formatted and refactored code

88d480a

incorporated feedback from PR

refactored to reduce repetition in code

35aef3f

fixed error in previous commit

b1f1b0d

.

5194dbe

added missing type to variable

be91eda

A-M-A-X requested a review from rhazn July 28, 2023 07:41

rhazn requested changes Jul 28, 2023

View reviewed changes

A-M-A-X added 2 commits July 28, 2023 12:46

added pr feedback - simplified createAndPutFile to createFileFromArch…

729788b

…ive. Moved putFile logic outside

Removed whitespaces

9499a52

A-M-A-X requested a review from rhazn July 28, 2023 11:09

rhazn approved these changes Jul 28, 2023

View reviewed changes

Merge remote-tracking branch 'DevTest/dev'

d705a2b

rhazn merged commit a9abd0b into jvalue:dev Jul 28, 2023
2 checks passed

github-actions bot locked and limited conversation to collaborators Jul 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature/#392_Unpack_gz_files_and_#391follow_redirects #405

feature/#392_Unpack_gz_files_and_#391follow_redirects #405

A-M-A-X commented Jul 21, 2023

georg-schwarz commented Jul 21, 2023

rhazn left a comment

rhazn Jul 21, 2023

rhazn Jul 21, 2023

rhazn Jul 21, 2023

rhazn Jul 21, 2023

A-M-A-X Jul 21, 2023

rhazn Jul 21, 2023

A-M-A-X Jul 21, 2023

rhazn Jul 21, 2023

A-M-A-X Jul 21, 2023

rhazn Jul 21, 2023

A-M-A-X Jul 21, 2023

rhazn Jul 21, 2023

A-M-A-X Jul 21, 2023

rhazn commented Jul 21, 2023

f3l1x98 commented Jul 21, 2023

rhazn commented Jul 26, 2023

rhazn left a comment

rhazn Jul 28, 2023

A-M-A-X Jul 28, 2023

rhazn commented Jul 28, 2023

rhazn commented Jul 28, 2023

A-M-A-X commented Jul 28, 2023

rhazn commented Jul 28, 2023

	description: 'The archive type to be interpreted, e.g., `"zip" or "gz`.',
	description: 'The archive type to be interpreted, e.g., `"zip" or "gz".',

	description: 'Indicates, whether to follow redirects on get requests. If `false`, Redirects are disabled. Default `true`',
	description: 'Indicates, whether to follow redirects on get requests. If `false`, redirects are not followed. Default `true`',

feature/#392_Unpack_gz_files_and_#391follow_redirects #405

feature/#392_Unpack_gz_files_and_#391follow_redirects #405

Conversation

A-M-A-X commented Jul 21, 2023

[FEATURE] HTTPExtractor should follow redirects #391

[FEATURE] Unpack gz files #392

georg-schwarz commented Jul 21, 2023

rhazn 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhazn commented Jul 21, 2023

f3l1x98 commented Jul 21, 2023

rhazn commented Jul 26, 2023

rhazn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhazn commented Jul 28, 2023

rhazn commented Jul 28, 2023

A-M-A-X commented Jul 28, 2023

rhazn commented Jul 28, 2023