Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
CGMES loading from zipped profiles inside a folder #3309
base: main
Are you sure you want to change the base?
CGMES loading from zipped profiles inside a folder #3309
Changes from 8 commits
6029bae
3272b91
5de9a2d
987c38f
b21b4d2
c3298e4
0bc78e1
30b3c30
1d4eb31
5e2c1d4
cd48e7c
f864862
563de70
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In
emptyZipErrorTest()
, there's no need to create an input stream for each profile since they are not read.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a performance issue here. We're reading the whole unzipped file to detect if the zip extraction is safe but we only need the first tag (for namespaces definition or the base attribute). I think we need to do something smarter to get safely the first characters only, not unzipping the complete file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose to check just first zip entry safety, checking only the compression ratio
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a simpler way is to use a
SafeZipInputStream
class wrapping theZipInputStream
instead of theZipInputStream
itself:That way you'll only read the first lines, not unzipping the full file, which might be more than 1GB uncompressed. Besides we could restrict maxBytesRead a lot in this PR (a few kB, or 1MB?), as we only read the first tag anyway (we stop at first
START_ELEMENT
in the 3 use cases).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed as it's not needed anymore (in this PR at least!) we should not keep it