laskoviymishka commented on code in PR #1404:
URL: https://github.com/apache/iceberg-go/pull/1404#discussion_r3536300881
##########
types.go:
##########
@@ -558,7 +564,21 @@ func (m *MapType) UnmarshalJSON(b []byte) error {
return nil
}
-func FixedTypeOf(n int) FixedType { return FixedType{len: n} }
+func validateFixedLength(length int) error {
+ if length <= 0 {
+ return fmt.Errorf("invalid fixed length %d: must be greater
than 0", length)
+ }
+
+ return nil
+}
+
+func FixedTypeOf(n int) FixedType {
+ if err := validateFixedLength(n); err != nil {
+ panic(fmt.Errorf("%w: %w", ErrInvalidArgument, err))
Review Comment:
I think there's a real crash hiding here. `FixedLiteral.Type()` at
literals.go:1124 is `return FixedTypeOf(len(f))`, and `FixedLiteral` is an
exported `[]byte` whose `UnmarshalBinary` happily accepts empty input. So a
zero-length literal — the zero value, `FixedLiteral(nil)`, or anything from
`UnmarshalBinary([]byte{})` — now panics inside `.Type()`, which has no error
return, so callers can't catch it.
This is the same failure #1405 fixed for `DecimalLiteral`. I'd mirror it:
either change literals.go:1124 to `return FixedType{len: len(f)}` to bypass the
validator, or make `UnmarshalBinary` reject empty data with an error so
corruption gets caught through the normal channel (I'd lean toward the latter).
Either way this needs handling before merge.
##########
types.go:
##########
@@ -558,7 +564,21 @@ func (m *MapType) UnmarshalJSON(b []byte) error {
return nil
}
-func FixedTypeOf(n int) FixedType { return FixedType{len: n} }
+func validateFixedLength(length int) error {
+ if length <= 0 {
+ return fmt.Errorf("invalid fixed length %d: must be greater
than 0", length)
+ }
+
+ return nil
+}
+
+func FixedTypeOf(n int) FixedType {
Review Comment:
Making a public constructor panic is a real API choice, and I'm not sure
it's the one we want here. `table/arrow_utils.go:374` calls
`iceberg.FixedTypeOf(dt.ByteWidth)` — `ByteWidth` can be `0` for a degenerate
Arrow schema, so that path goes from silently producing a zero-width type to
crashing, and it's not updated in this PR.
I'd lean toward `FixedTypeOf(n int) (FixedType, error)`, or a
`MustFixedTypeOf` alongside it à la `regexp.MustCompile`, so callers deriving
`n` from external data have a channel to handle it. wdyt?
##########
types.go:
##########
@@ -189,7 +189,13 @@ func (t *typeIFace) UnmarshalJSON(b []byte) error {
return fmt.Errorf("%w: %s",
ErrInvalidTypeString, typename)
}
- n, _ := strconv.Atoi(matches[1])
+ n, err := strconv.Atoi(matches[1])
Review Comment:
Worth noting the regex `(\d+)` already guarantees digits, so this `err`
branch is only reachable via integer overflow (e.g.
`fixed[99999999999999999999]`) — which is real, and better than the pre-PR
behavior of silently mapping to `0`, but it's untested and surfaces a raw Go
parse error rather than a domain message.
I'd add a `fixed[99999999999999999999]` case and wrap the error a bit more
descriptively.
##########
types.go:
##########
@@ -189,7 +189,13 @@ func (t *typeIFace) UnmarshalJSON(b []byte) error {
return fmt.Errorf("%w: %s",
ErrInvalidTypeString, typename)
}
- n, _ := strconv.Atoi(matches[1])
+ n, err := strconv.Atoi(matches[1])
+ if err != nil {
+ return fmt.Errorf("%w: %s",
ErrInvalidTypeString, err)
+ }
+ if err := validateFixedLength(n); err != nil {
Review Comment:
I'm a little wary of gating this on the read path. The spec doesn't require
`fixed` length > 0 — Java's `FixedType.ofLength` + `FIXED` regex, PyIceberg's
`FixedType.__init__`, and iceberg-rust's `Fixed(u64)` all accept `fixed[0]`
with no check. So after this, any table metadata written by one of those
clients with a `fixed[0]` column fails to unmarshal here with
`ErrInvalidTypeString`, i.e. an unreadable table.
I'd keep the validation on the construction/write path but drop it from
`UnmarshalJSON`, unless we've confirmed via the community that the spec intends
to prohibit `fixed[0]`. Do we know either way?
##########
types.go:
##########
@@ -558,7 +564,21 @@ func (m *MapType) UnmarshalJSON(b []byte) error {
return nil
}
-func FixedTypeOf(n int) FixedType { return FixedType{len: n} }
+func validateFixedLength(length int) error {
+ if length <= 0 {
+ return fmt.Errorf("invalid fixed length %d: must be greater
than 0", length)
Review Comment:
Small thing while we're here: this message has its own colon, so wrapped it
reads `invalid argument: invalid fixed length 0: must be greater than 0` —
three colon segments. I'd flatten it to `fixed length must be greater than 0,
got %d`.
Also the `%w: %w` at line 577 is the only multi-`%w` in the package
(everything in exprs.go uses a single `%w` sentinel + string detail), and since
the inner error isn't a sentinel the second `%w` buys nothing for `errors.Is`.
`panic(fmt.Errorf("%w: fixed length must be greater than 0, got %d",
ErrInvalidArgument, n))` would keep it consistent.
##########
types_test.go:
##########
@@ -77,6 +77,39 @@ func TestFixedType(t *testing.T) {
assert.Equal(t, "fixed[5]", typ.String())
assert.True(t, typ.Equals(iceberg.FixedTypeOf(5)))
assert.False(t, typ.Equals(iceberg.FixedTypeOf(6)))
+
+ t.Run("FixedTypeOf validates length", func(t *testing.T) {
+ assert.PanicsWithError(t, "invalid argument: invalid fixed
length 0: must be greater than 0", func() {
+ iceberg.FixedTypeOf(0)
+ })
+ assert.PanicsWithError(t, "invalid argument: invalid fixed
length -1: must be greater than 0", func() {
+ iceberg.FixedTypeOf(-1)
+ })
+ })
+}
+
+func TestFixedTypeInvalidParse(t *testing.T) {
+ tests := []struct {
+ name string
+ data string
+ }{
+ {
+ name: "zero length",
+ data: `{"id": 1, "name": "f", "type": "fixed[0]",
"required": true}`,
+ },
+ {
+ name: "invalid syntax",
Review Comment:
`fixed[0]junk` fails the bracket regex and returns before ever reaching
`validateFixedLength`, so this exercises the pre-existing path, not the new
code, and the name is a bit misleading. I'd either rename it (it's really
"malformed suffix") or swap in the overflow case from the comment above so
we're actually covering the new branches.
##########
types_test.go:
##########
@@ -77,6 +77,39 @@ func TestFixedType(t *testing.T) {
assert.Equal(t, "fixed[5]", typ.String())
assert.True(t, typ.Equals(iceberg.FixedTypeOf(5)))
assert.False(t, typ.Equals(iceberg.FixedTypeOf(6)))
+
+ t.Run("FixedTypeOf validates length", func(t *testing.T) {
+ assert.PanicsWithError(t, "invalid argument: invalid fixed
length 0: must be greater than 0", func() {
Review Comment:
Matching the exact panic string makes this brittle — any wording tweak (like
the one above) breaks it. I'd assert on the sentinel instead: recover and
`assert.ErrorIs(recovered, iceberg.ErrInvalidArgument)`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
