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.

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

file: unzip introduced #2692

Merged
merged 2 commits into from
Aug 25, 2021
Merged

file: unzip introduced #2692

merged 2 commits into from
Aug 25, 2021

Conversation

tg44
Copy link
Contributor

@tg44 tg44 commented Jun 24, 2021

docs are missing, the basic test works, ideas welcome :)

@ennru
Copy link
Member

ennru commented Jul 18, 2021

Nice! Some docs would be required before merging.

@tg44
Copy link
Contributor Author

tg44 commented Jul 21, 2021

I added docs, but can't test it, for some reason the google protobuff things are not compile on my machine.

Copy link
Member

@ennru ennru left a comment

Choose a reason for hiding this comment

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

Great. Some API tidiness suggestion.

import java.io.{File, FileInputStream}
import java.util.zip.{ZipEntry, ZipInputStream}

case class ZipArchiveMetadata(name: String)
Copy link
Member

Choose a reason for hiding this comment

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

This class is user-facing API and should preferably live in akka.stream.alpakka.file instead. Add a getName for Java folks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@@ -106,6 +107,23 @@ public void flowShouldCreateZIPArchive() throws Exception {
Map<String, ByteString> unzip = archiveHelper.unzip(resultFileContent);

assertThat(inputFiles, is(unzip));
Path target = Files.createTempDirectory("alpakka-tar-");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Path target = Files.createTempDirectory("alpakka-tar-");
Path target = Files.createTempDirectory("alpakka-zip-");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@mkurz
Copy link
Contributor

mkurz commented Aug 21, 2021

Thanks for working on this @tg44! Can you have a look at the suggestions @ennru made? I would love get this merged as well, thanks!

Copy link
Member

@ennru ennru left a comment

Choose a reason for hiding this comment

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

LGTM.

@ennru ennru added this to the 3.0.4 milestone Aug 25, 2021
@ennru ennru merged commit f9214c8 into akka:master Aug 25, 2021
}
}

@InternalApi class ZipSource(f: File, chunkSize: Int)
Copy link
Contributor

Choose a reason for hiding this comment

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

@tg44 I see that ZipSource "only" handles Files. However what about this scenario: In a Play app a user uploads a zip file and I want to directly, without storing it on disk, unpack it and stream its content(s) to e.g. S3 buckets. This could probably be done in a Play Body parser. I guess ZipSource would need to handle Source to achieve that?
What do you think? Would it be somehow possible to adapt your code to support such a scenario?
Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current source works with file seek. I needed to parallel access files. Without File and seeking, you need to consume the "subSources", and you can't use parallelism, which is in my opinion makes the source hard to use, and easy to misuse. (I think the tar flow works like this right now.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I totally understand your approach. I had a look at the tar reading example now, and I think it would be good if we could have the same for zip reading as well (in addition to your implementation). What also bothers me a bit is that for writing zip files as well as for read/writing tar file we work with sources and sinks, but for reading zip files we only allow Files now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ofc. we can implement a tar reading like API too, I just haven't needed that :) I think you can easily merge the tar reading and zip file reading logic. (Sorry, but I'm really short on time nowadays, so I can't contribute that in the next weeks.)

@ennru ennru mentioned this pull request Feb 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants