Skip to content

[COMPRESS-639] Key the entry metadata of ZipArchiveOutputStream by identity - #826

Open
sharanggupta wants to merge 2 commits into
apache:masterfrom
sharanggupta:COMPRESS-639-identical-entries
Open

sharanggupta wants to merge 2 commits into
apache:masterfrom
sharanggupta:COMPRESS-639-identical-entries

Conversation

@sharanggupta

Copy link
Copy Markdown

COMPRESS-639

What

ZipArchiveOutputStream keeps the local header offset of each entry in a HashMap keyed by the ZipArchiveEntry, and looks it up when it writes the central directory. ZipArchiveEntry.hashCode() only uses the name, but equals(Object) compares the times, sizes, CRC, extra fields and more, and some of these change while an entry is written. So entries with the same name, time, size and CRC, such as the entries that ParallelScatterZipCreator adds in the issue, can share one map entry:

  • If two entries are equal when they are written, the second put replaces the offset of the first, and both central directory records point to the second local file header. The archive silently loses the first entry.
  • If the extra fields of one entry change when the central directory is written, as they do with Zip64Mode.Always, the lookup of the other entry finds nothing, and close() throws the NullPointerException from the issue.

How

The map is now an IdentityHashMap. Each entry is put and looked up with the same ZipArchiveEntry instance, so every entry keeps its own offset, whatever equals(Object) says.

Tests

  • ParallelScatterZipCreatorTest.sameZipArchiveEntryNullPointerException, which was disabled for COMPRESS-639, is enabled. It now also reads the archive back and checks that the 100 entries have 100 different local header offsets. It fails on master with the NullPointerException.
  • ZipArchiveOutputStreamTest.testEqualEntriesKeepTheirOwnLocalHeaders writes two equal STORED entries and checks with ZipFile that the first entry's local header is at offset 0 and the second one's right after the first entry's data. On master, both point to the second local header.

changes.xml has an entry. mvn and mvn changes:changes-validate -DfailOnError=true pass locally on JDK 17.

Before you push a pull request, review this list:

  • Read the contribution guidelines for this project.
  • Read the ASF Generative Tooling Guidance if you use Artificial Intelligence (AI).
  • I used AI to create any part of, or all of, this pull request. Which AI tool was used to create this pull request, and to what extent did it contribute? Claude (Anthropic) helped analyse the issue and draft the change and tests; I reviewed them against the code and ran the build.
  • Run a successful build using the default Maven goal with mvn; that's mvn on the command line by itself.
  • Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best practice.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Each commit in the pull request should have a meaningful subject line and body. Note that a maintainer may squash commits during the merge process.

…entity

ZipArchiveOutputStream looked up the local header offset of each entry
in a HashMap keyed by ZipArchiveEntry, whose equals(Object) depends on
fields that change while an entry is written. Equal entries, such as
the ones ParallelScatterZipCreator adds for the same name, then shared
one map entry: the central directory pointed both at the same local
header, or close() threw a NullPointerException. Use an IdentityHashMap
and enable the test that was disabled for COMPRESS-639.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant