Skip to content

fix(types): safe compare defaults in NestedField.Equals#1383

Merged
zeroshade merged 1 commit into
apache:mainfrom
fallintoplace:fix/nestedfield-equals-safe-defaults
Jul 4, 2026
Merged

fix(types): safe compare defaults in NestedField.Equals#1383
zeroshade merged 1 commit into
apache:mainfrom
fallintoplace:fix/nestedfield-equals-safe-defaults

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

Summary

NestedField.Equals compared InitialDefault and WriteDefault with ==, which can panic when defaults contain non-comparable values (maps/slices/etc).

This changes equality to use reflect.DeepEqual for defaults so schema/field comparisons stay safe and correct with nested defaults.

Also adds a regression test that verifies:

  • non-comparable defaults no longer panic in Equals, and
  • equivalent deep structures compare equal while different defaults compare false.

Testing

  • go test ./... -run 'TestNestedFieldEqualsWithNonComparableDefaults|TestUnknownTypeInNestedStructs|TestTypesBasic' -count=1

@fallintoplace fallintoplace requested a review from zeroshade as a code owner July 4, 2026 16:18
@fallintoplace fallintoplace force-pushed the fix/nestedfield-equals-safe-defaults branch from a3d900b to be1925e Compare July 4, 2026 16:21
@fallintoplace fallintoplace force-pushed the fix/nestedfield-equals-safe-defaults branch from be1925e to b9170fe Compare July 4, 2026 16:26

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Switching to reflect.DeepEqual makes NestedField.Equals panic-safe for non-comparable defaults while preserving raw-value equality; the non-comparable-defaults test covers it.

@zeroshade zeroshade merged commit 79f571c into apache:main Jul 4, 2026
15 checks passed
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.

2 participants