From 69d10cbb1a2d1e505ed3248d3a7ed647bf194354 Mon Sep 17 00:00:00 2001 From: Gabor Kaszab Date: Thu, 17 Sep 2026 11:29:57 +0200 Subject: [PATCH] Core: ManifestListWriter cleanup around encryption keys Problems this solves: - There is a dependency between toManifestFile() and encryptionKeys(). - From toManifestFile()'s return type only keyId is used, but it's also present in the output of encryptionKeys() - manifestListFile member can be dropped - outputFile member can be dropped --- .../apache/iceberg/ManifestListWriter.java | 32 +++++++------------ .../org/apache/iceberg/SnapshotProducer.java | 3 +- .../iceberg/TestManifestListEncryption.java | 13 ++++---- 3 files changed, 19 insertions(+), 29 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/ManifestListWriter.java b/core/src/main/java/org/apache/iceberg/ManifestListWriter.java index af2c3cb4895b..2edcf80ae4cd 100644 --- a/core/src/main/java/org/apache/iceberg/ManifestListWriter.java +++ b/core/src/main/java/org/apache/iceberg/ManifestListWriter.java @@ -36,22 +36,21 @@ abstract class ManifestListWriter implements FileAppender { private final FileAppender writer; private final StandardEncryptionManager standardEncryptionManager; private final NativeEncryptionKeyMetadata manifestListKeyMetadata; - private final OutputFile outputFile; private boolean closed = false; - private ManifestListFile manifestListFile; private FileEncryptionKeys encryptionKeys; private ManifestListWriter( OutputFile file, EncryptionManager encryptionManager, Map meta) { + OutputFile outputFile; if (encryptionManager instanceof StandardEncryptionManager) { // ability to encrypt the manifest list key is introduced for standard encryption. this.standardEncryptionManager = (StandardEncryptionManager) encryptionManager; EncryptedOutputFile encryptedFile = this.standardEncryptionManager.encrypt(file); - this.outputFile = encryptedFile.encryptingOutputFile(); + outputFile = encryptedFile.encryptingOutputFile(); this.manifestListKeyMetadata = (NativeEncryptionKeyMetadata) encryptedFile.keyMetadata(); } else { this.standardEncryptionManager = null; - this.outputFile = file; + outputFile = file; this.manifestListKeyMetadata = null; } @@ -98,25 +97,16 @@ public Long nextRowId() { return null; } - public ManifestListFile toManifestListFile() { - Preconditions.checkState(closed, "Cannot build ManifestListFile, writer is not closed"); - if (manifestListFile == null) { - if (manifestListKeyMetadata != null && manifestListKeyMetadata.encryptionKey() != null) { - this.encryptionKeys = - standardEncryptionManager.registerKeyMetadata( - manifestListKeyMetadata.copyWithLength(writer.length())); - this.manifestListFile = - new BaseManifestListFile(outputFile.location(), encryptionKeys.fileKey().keyId()); - } else { - this.manifestListFile = new BaseManifestListFile(outputFile.location(), null); - } + FileEncryptionKeys encryptionKeys() { + Preconditions.checkState(closed, "Cannot build encryption keys, writer is not closed"); + if (encryptionKeys == null + && manifestListKeyMetadata != null + && manifestListKeyMetadata.encryptionKey() != null) { + this.encryptionKeys = + standardEncryptionManager.registerKeyMetadata( + manifestListKeyMetadata.copyWithLength(writer.length())); } - return manifestListFile; - } - - FileEncryptionKeys encryptionKeys() { - toManifestListFile(); return encryptionKeys; } diff --git a/core/src/main/java/org/apache/iceberg/SnapshotProducer.java b/core/src/main/java/org/apache/iceberg/SnapshotProducer.java index 976861cc44e4..c7357692231e 100644 --- a/core/src/main/java/org/apache/iceberg/SnapshotProducer.java +++ b/core/src/main/java/org/apache/iceberg/SnapshotProducer.java @@ -356,7 +356,6 @@ public Snapshot apply() { replacedRecords); } - ManifestListFile manifestListFile = writer.toManifestListFile(); this.manifestListEncryptionKeys = writer.encryptionKeys(); return new BaseSnapshot( @@ -370,7 +369,7 @@ public Snapshot apply() { manifestList.location(), nextRowId, assignedRows, - manifestListFile.encryptionKeyID()); + manifestListEncryptionKeys != null ? manifestListEncryptionKeys.fileKey().keyId() : null); } private void runValidations(Snapshot parentSnapshot) { diff --git a/core/src/test/java/org/apache/iceberg/TestManifestListEncryption.java b/core/src/test/java/org/apache/iceberg/TestManifestListEncryption.java index 61eeed0e083f..94fe4f615e34 100644 --- a/core/src/test/java/org/apache/iceberg/TestManifestListEncryption.java +++ b/core/src/test/java/org/apache/iceberg/TestManifestListEncryption.java @@ -353,15 +353,12 @@ private ManifestFile writeAndReadEncryptedManifestList(EncryptionManager em) thr SEQ_NUM, SNAPSHOT_FIRST_ROW_ID); try (writer) { - assertThatThrownBy(writer::toManifestListFile) + assertThatThrownBy(writer::encryptionKeys) .isInstanceOf(IllegalStateException.class) - .hasMessage("Cannot build ManifestListFile, writer is not closed"); + .hasMessage("Cannot build encryption keys, writer is not closed"); writer.add(TEST_MANIFEST); } - ManifestListFile manifestListFile = writer.toManifestListFile(); - assertThat(writer.toManifestListFile().encryptionKeyID()) - .isEqualTo(manifestListFile.encryptionKeyID()); FileEncryptionKeys encryptionKeys = writer.encryptionKeys(); // First try to read without decryption @@ -375,7 +372,11 @@ private ManifestFile writeAndReadEncryptedManifestList(EncryptionManager em) thr io, EncryptionTestHelpers.createEncryptionManager( List.of(encryptionKeys.keyEncryptionKey(), encryptionKeys.fileKey())))) { - List manifests = ManifestLists.read(readingIO.newInputFile(manifestListFile)); + List manifests = + ManifestLists.read( + readingIO.newInputFile( + new BaseManifestListFile( + outputFile.location(), encryptionKeys.fileKey().keyId()))); assertThat(manifests).hasSize(1); return manifests.get(0); }