Skip to content

fix(table): validate sort field source IDs#1313

Open
fallintoplace wants to merge 2 commits into
apache:mainfrom
fallintoplace:fix/sort-field-source-id-validation
Open

fix(table): validate sort field source IDs#1313
fallintoplace wants to merge 2 commits into
apache:mainfrom
fallintoplace:fix/sort-field-source-id-validation

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

Summary

  • reject sort fields that omit both source-id and source-ids
  • reject zero or negative sort source IDs for both JSON metadata and NewSortOrder
  • add regression coverage for missing, empty, zero, negative, and mixed invalid source IDs

Why

SortField.UnmarshalJSON treated a missing source-id as the Go zero value and accepted it as source ID 0. That allowed invalid sort metadata to enter the table model and fail later during compatibility checks or other sort-order use.

Testing

  • go test ./table -run 'Test(NewSortOrderRejectsInvalidSourceIDs|SortFieldMultiArgSourceIDs|UnmarshalSortOrderDefaults|UnmarshalInvalidSort)' -count=1
  • go test ./table -count=1
  • go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.11.4 run --timeout=10m ./table/...
  • git diff --check

@fallintoplace fallintoplace requested a review from zeroshade as a code owner June 24, 2026 21:28

@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.

Thanks - validating source-id shape (presence + positivity) at unmarshal and in NewSortOrder is a solid addition, and the table tests are thorough. Requesting changes on one related gap with multi-arg source-ids - inline.

Comment thread table/sorting.go
fields = []SortField{}
}
for idx, field := range fields {
if err := validateSortSourceIDs(field.SourceIDs); err != nil {

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.

This validates that every entry in SourceIDs is present and positive - good. The remaining gap is existence in the schema: SortOrder.CheckCompatibility only looks up field.SourceID(), i.e. the first source id (sorting.go:298), so a multi-arg sort field such as SourceIDs: [1, 999] still passes when 999 isn't in the schema. Worth iterating all of field.SourceIDs there (looking each up via FindFieldByID and checking it's primitive), with a regression case pairing a valid id and a nonexistent one.

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.

3 participants