Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improvements to archive handling and storages #7129

Merged
merged 5 commits into from
Jun 19, 2023
Merged

Improvements to archive handling and storages #7129

merged 5 commits into from
Jun 19, 2023

Conversation

sschuberth
Copy link
Member

Please have a look at the individual commit messages for the details.

@sschuberth sschuberth requested a review from a team as a code owner June 14, 2023 12:24
@github-actions
Copy link

Qodana

It seems all right 👌

No new problems found according to the checks applied
💡 Qodana analysis was run in the pull request mode: only the changed files were checked

View the detailed Qodana report

To be able to view the detailed Qodana report, you can either:

  1. Register at Qodana Cloud and configure the action
  2. Use GitHub Code Scanning with Qodana
  3. Host Qodana report at GitHub Pages
  4. Inspect and use qodana.sarif.json (see the Qodana SARIF format for details)
Contact Qodana team

Contact us at qodana-support@jetbrains.com

@codecov
Copy link

codecov bot commented Jun 14, 2023 •
edited
Loading

Codecov Report

Patch coverage: 68.00% and project coverage change: -0.02 ⚠️

Comparison is base (0dd0d6f) 64.28% compared to head (992cd8f) 64.27%.

Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #7129      +/-   ##
============================================
- Coverage     64.28%   64.27%   -0.02%     
  Complexity     1970     1970              
============================================
  Files           333      333              
  Lines         16681    16667      -14     
  Branches       2386     2389       +3     
============================================
- Hits          10724    10713      -11     
+ Misses         4918     4912       -6     
- Partials       1039     1042       +3
Flag Coverage Δ
funTest-non-docker 28.54% <32.00%> (-0.05%) ⬇️
test 40.05% <52.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...src/main/kotlin/utils/FileProvenanceFileStorage.kt 63.63% <50.00%> (-9.45%) ⬇️
model/src/main/kotlin/utils/FileArchiver.kt 85.71% <54.54%> (+2.38%) ⬆️
...main/kotlin/utils/PostgresProvenanceFileStorage.kt 92.68% <100.00%> (+5.44%) ⬆️
scanner/src/main/kotlin/utils/FileListResolver.kt 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@fviernau fviernau changed the title Imrpovements to archive handling and storages Improvements to archive handling and storages Jun 14, 2023
@sschuberth
Copy link
Member Author

After this PR, the ProvenanceFileStorage and FileStorage interface look awfully similar. They should probably be combined via a generic type, and aligning the semantics for getData / read if the key does not exist (return a nullable type vs, throwing an exception).

mnonnenmacher
mnonnenmacher requested changes Jun 19, 2023
View reviewed changes
utils/common/src/test/kotlin/ArchiveUtilsTest.kt Show resolved Hide resolved
utils/common/src/main/kotlin/ArchiveUtils.kt Outdated Show resolved Hide resolved
model/src/funTest/kotlin/utils/PostgresProvenanceFileStorageFunTest.kt Show resolved Hide resolved
scanner/src/main/kotlin/utils/FileListResolver.kt
putFile(provenance, tempFile)

tempFile.delete()
putData(provenance, fileList.toYaml().byteInputStream())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice if we could avoid serializing the list to memory first and instead directly serialize to the stream. But IIRC Jackson can only serialize to an output stream and converting that to an input stream requires to use the PipedInputStream where I had problems the last time I tried to use it.

scanner/src/main/kotlin/utils/FileListResolver.kt Outdated Show resolved Hide resolved
@sschuberth sschuberth force-pushed the archive-storage-streams branch 2 times, most recently from 4aaacb1 to 727dc3d Compare June 19, 2023 11:26
@github-actions
Copy link

Qodana

It seems all right 👌

No new problems found according to the checks applied
💡 Qodana analysis was run in the pull request mode: only the changed files were checked

View the detailed Qodana report

To be able to view the detailed Qodana report, you can either:

  1. Register at Qodana Cloud and configure the action
  2. Use GitHub Code Scanning with Qodana
  3. Host Qodana report at GitHub Pages
  4. Inspect and use qodana.sarif.json (see the Qodana SARIF format for details)
Contact Qodana team

Contact us at qodana-support@jetbrains.com

1 similar comment
@github-actions
Copy link

Qodana

It seems all right 👌

No new problems found according to the checks applied
💡 Qodana analysis was run in the pull request mode: only the changed files were checked

View the detailed Qodana report

To be able to view the detailed Qodana report, you can either:

  1. Register at Qodana Cloud and configure the action
  2. Use GitHub Code Scanning with Qodana
  3. Host Qodana report at GitHub Pages
  4. Inspect and use qodana.sarif.json (see the Qodana SARIF format for details)
Contact Qodana team

Contact us at qodana-support@jetbrains.com

@sschuberth
refactor(common-utils): Ensure a primary extension for ArchiveTypes
78b605f 
Ensure that always a distinguished primary extension is specified first.
This can be used to automatically determine the best extension to use for
a type.

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
@sschuberth
test(common-utils): Parameterize ArchiveUtilsTest
1bab47c 
This reduces code duplication and eases adding tests for new types.

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
@sschuberth
chore(common-utils): Improve ArchiveUtils (test) logging
5c967ad 
Help to better understand the archive type detected for a stream.

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
@sschuberth
refactor(common-utils): Improve the work-around for mis-detected arch…
03aa926 
…ives

The issue mentioned in 4e7671f actually only applies to TAR-based
archives: All BZIP2-, GZ-, XZ-compressed archives may be detected as
empty uncompressed TAR archives. To avoid that, simply trying
compressed TAR archives first by reordering the `ArchiveType` enum entries
is enough. Then throwing the `IOException` which cannot distinguish
between unsupported and empty archives can be limited to TAR archives.

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
@sschuberth
refactor: Make ProvenanceFileStorage operate on stream instead of f…
992cd8f 
…iles

This avoids the temporary creation of files as well as potential confusion
about persisting the file name, and paves the way for storages that
cannot operate on files but on streams.

Resolves #7118.

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
@sschuberth sschuberth enabled auto-merge (rebase) June 19, 2023 15:13
@github-actions
Copy link

Qodana

It seems all right 👌

No new problems found according to the checks applied
💡 Qodana analysis was run in the pull request mode: only the changed files were checked

View the detailed Qodana report

To be able to view the detailed Qodana report, you can either:

  1. Register at Qodana Cloud and configure the action
  2. Use GitHub Code Scanning with Qodana
  3. Host Qodana report at GitHub Pages
  4. Inspect and use qodana.sarif.json (see the Qodana SARIF format for details)
Contact Qodana team

Contact us at qodana-support@jetbrains.com

@sschuberth sschuberth merged commit 47bd691 into main Jun 19, 2023
20 checks passed
@sschuberth sschuberth deleted the archive-storage-streams branch June 19, 2023 15:58
@sschuberth sschuberth mentioned this pull request Jun 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mnonnenmacher mnonnenmacher mnonnenmacher approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sschuberth @mnonnenmacher