Skip to content
New issue

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

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

Already on GitHub? # to your account

Huge refactor and stabilize #13

Merged
merged 8 commits into from
Dec 6, 2018
Merged

Huge refactor and stabilize #13

merged 8 commits into from
Dec 6, 2018

Conversation

oli-h
Copy link

@oli-h oli-h commented Dec 4, 2018

Solves #12

Various issues addressed:

  • Update Vertx to 3.5.3
  • introduced ZipIterator (as a replacement for ZipFileEntryIterator) which
    1. does not hide Exceptions in RuntimeExceptions
    2. behaves like a java.util.Iterator
    3. ... but formally does not implement java.util.Iterator (as there is throws IOException in method signature)
  • introduced BufferWrapperInputStream to directly feed ZipInputStream from Vertx Buffers. No need to duplicate Vertx Buffer as byte[] in-memory
  • Split code in ResourceMirrorHandler into three classes:
    1. ResourceMirrorHandler is still is the entry-point for all Mirror-Requests
    2. But now it delegates each single MirrorRequest to one instance of (the new class) MirrorRequestHandler. This enables us to have instance variables (state, config) and avoids methods with large number of parameters
    3. Task to iterate over the ZIP and PUT each entry is extracted to class ZipEntryPutter
  • clarified http-response-code behaviour: "200 OK" only when the whole MirrorRequest was successful without any problems

Oliver Henning added 6 commits December 4, 2018 16:09
- does not hide Exceptions in RuntimeExceptions
- behaves like a java.util.Iterator
- formally does not implement java.util.Iterator
We now can feed ZipInputStream from Vertx Buffers. No need to duplicate Vertx Buffer as byte[] in-memory
- ResourceMirrorHandler still is the entry-point for ALL Mirror-Requests
- it spins of one instance of MirrorRequestHandler for each single MirrorRequest. This enables us to have instance variables (a 'state' resp. 'config') and avoids methods with large number of parameters
- Task to iterate over the ZIP and PUT each entry is extracted to class ZipEntryPutter

Additionally:
- simplified execution paths
- clear http-response-code: "200 OK" only when whole MirrorRequest was successful without any problems
if (pos >= buffer.length()) {
return -1;
}
return buffer.getByte(pos++) & 0xFF;
Copy link
Member

Choose a reason for hiding this comment

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

What do we need this bitmask (0xFF) for? Is it required somehow?

According to vertx Buffer doc getByte returns a byte and IMO cannot return any values where this filter would change something.

Am I missing the point?

Copy link
Author

Choose a reason for hiding this comment

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

a byte is -128 ... +127 in Java.
But when InputStream#read() returns -1 this indicates 'end of stream' - so we must not return -1 when this is a 'read byte'

return false;
}
String filename = ze.getName();
if (!filename.endsWith("/")) {
Copy link
Contributor

@ljucam ljucam Dec 5, 2018

Choose a reason for hiding this comment

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

Can we really have filenames in a zip stream that represents not files but folders?
And yeah, I know that for e.g. ZipOutputStream can handle empty directories by adding a forward-slash / after the folder name. But can we really have this here?

Copy link
Author

Choose a reason for hiding this comment

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

It was so in the old implementation - 'directories' (identified by trailing slash) where explicit skipped.
And obviously yes, 'directories' can be listed in a ZIP - for whatever reason

@oli-h oli-h requested a review from mcweba December 6, 2018 09:36
@mcweba mcweba merged commit b5d029d into swisspost:develop Dec 6, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants