This is an automated email from the ASF dual-hosted git repository.
laskoviymishka pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-go.git
The following commit(s) were added to refs/heads/main by this push:
new bf1536b7a refactor(puffin): centralize writer state checks (#2365)
bf1536b7a is described below
commit bf1536b7ab70e1685f27a329da1560c7c11ec1bc
Author: Minh Vu <[email protected]>
AuthorDate: Fri Oct 30 09:22:14 2026 +0100
refactor(puffin): centralize writer state checks (#2365)
---
puffin/puffin_writer.go | 48 +++++++++++-----------
puffin/puffin_writer_state_test.go | 84 ++++++++++++++++++++++++++++++++++++++
2 files changed, 107 insertions(+), 25 deletions(-)
diff --git a/puffin/puffin_writer.go b/puffin/puffin_writer.go
index a5836662f..113206c11 100644
--- a/puffin/puffin_writer.go
+++ b/puffin/puffin_writer.go
@@ -90,15 +90,25 @@ func NewWriter(w io.Writer) (*Writer, error) {
}, nil
}
+// checkState keeps the writer's finalized and failed-state checks consistent
+// across operations. State checks must run before validating any input.
+func (w *Writer) checkState(action string) error {
+ if w.done {
+ return fmt.Errorf("puffin: cannot %s: writer already
finalized", action)
+ }
+ if w.failed != nil {
+ return fmt.Errorf("puffin: cannot %s: writer previously failed:
%w", action, w.failed)
+ }
+
+ return nil
+}
+
// SetProperties merges the provided properties into the file-level properties
// written to the footer. Repeated calls overwrite matching keys; use
// ClearProperties to remove previously set custom properties.
func (w *Writer) SetProperties(props map[string]string) error {
- if w.done {
- return errors.New("puffin: cannot set properties: writer
already finalized")
- }
- if w.failed != nil {
- return fmt.Errorf("puffin: cannot set properties: writer
previously failed: %w", w.failed)
+ if err := w.checkState("set properties"); err != nil {
+ return err
}
maps.Copy(w.props, props)
@@ -107,11 +117,8 @@ func (w *Writer) SetProperties(props map[string]string)
error {
// ClearProperties removes all file-level properties.
func (w *Writer) ClearProperties() error {
- if w.done {
- return errors.New("puffin: cannot clear properties: writer
already finalized")
- }
- if w.failed != nil {
- return fmt.Errorf("puffin: cannot clear properties: writer
previously failed: %w", w.failed)
+ if err := w.checkState("clear properties"); err != nil {
+ return err
}
w.props = make(map[string]string)
@@ -121,11 +128,8 @@ func (w *Writer) ClearProperties() error {
// SetCreatedBy overrides the default "created-by" property written to the
footer.
// The default value is "iceberg-go". Example: "MyApp version 1.2.3".
func (w *Writer) SetCreatedBy(createdBy string) error {
- if w.done {
- return errors.New("puffin: cannot set created-by: writer
already finalized")
- }
- if w.failed != nil {
- return fmt.Errorf("puffin: cannot set created-by: writer
previously failed: %w", w.failed)
+ if err := w.checkState("set created-by"); err != nil {
+ return err
}
if createdBy == "" {
return errors.New("puffin: cannot set created-by: value cannot
be empty")
@@ -139,11 +143,8 @@ func (w *Writer) SetCreatedBy(createdBy string) error {
// Returns the complete BlobMetadata including the computed Offset and Length.
// The input.Type is required; use constants like ApacheDataSketchesThetaV1.
func (w *Writer) AddBlob(input BlobMetadataInput, data []byte) (BlobMetadata,
error) {
- if w.done {
- return BlobMetadata{}, errors.New("puffin: cannot add blob:
writer already finalized")
- }
- if w.failed != nil {
- return BlobMetadata{}, fmt.Errorf("puffin: cannot add blob:
writer previously failed: %w", w.failed)
+ if err := w.checkState("add blob"); err != nil {
+ return BlobMetadata{}, err
}
if input.Type == "" {
return BlobMetadata{}, errors.New("puffin: cannot add blob:
type is required")
@@ -216,11 +217,8 @@ func (w *Writer) AddBlob(input BlobMetadataInput, data
[]byte) (BlobMetadata, er
// Must be called exactly once after all blobs are written.
// After Finish returns, no further operations are allowed on the writer.
func (w *Writer) Finish() error {
- if w.done {
- return errors.New("puffin: cannot finish: writer already
finalized")
- }
- if w.failed != nil {
- return fmt.Errorf("puffin: cannot finish: writer previously
failed: %w", w.failed)
+ if err := w.checkState("finish"); err != nil {
+ return err
}
// Build footer
diff --git a/puffin/puffin_writer_state_test.go
b/puffin/puffin_writer_state_test.go
new file mode 100644
index 000000000..8e66695d5
--- /dev/null
+++ b/puffin/puffin_writer_state_test.go
@@ -0,0 +1,84 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package puffin
+
+import (
+ "errors"
+ "io"
+ "testing"
+)
+
+func TestWriterStateGuardErrors(t *testing.T) {
+ ops := []struct {
+ name string
+ action string
+ call func(*Writer) error
+ }{
+ {"SetProperties", "set properties", func(w *Writer) error {
+ return w.SetProperties(map[string]string{"key":
"value"})
+ }},
+ {"ClearProperties", "clear properties", func(w *Writer) error {
+ return w.ClearProperties()
+ }},
+ {"SetCreatedBy", "set created-by", func(w *Writer) error {
+ return w.SetCreatedBy("")
+ }},
+ {"AddBlob", "add blob", func(w *Writer) error {
+ _, err := w.AddBlob(BlobMetadataInput{}, nil)
+
+ return err
+ }},
+ {"Finish", "finish", func(w *Writer) error {
+ return w.Finish()
+ }},
+ }
+
+ for _, op := range ops {
+ t.Run(op.name, func(t *testing.T) {
+ finalized, err := NewWriter(io.Discard)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if err := finalized.Finish(); err != nil {
+ t.Fatal(err)
+ }
+
+ wantFinalized := "puffin: cannot " + op.action + ":
writer already finalized"
+ if got := op.call(finalized); got == nil || got.Error()
!= wantFinalized {
+ t.Fatalf("finalized: got %v, want %q", got,
wantFinalized)
+ }
+
+ failed, err := NewWriter(io.Discard)
+ if err != nil {
+ t.Fatal(err)
+ }
+ underlying := errors.New("write failed")
+ failed.failed = underlying
+ wantFailed := "puffin: cannot " + op.action + ": writer
previously failed: write failed"
+ if got := op.call(failed); got == nil || got.Error() !=
wantFailed || !errors.Is(got, underlying) {
+ t.Fatalf("failed: got %v, want error wrapping
%q", got, wantFailed)
+ }
+
+ // When both flags are set, finalization still takes
precedence.
+ failed.done = true
+ if got := op.call(failed); got == nil || got.Error() !=
wantFinalized {
+ t.Fatalf("finalized and failed: got %v, want
%q", got, wantFinalized)
+ }
+ })
+ }
+}
