🐛 Sort on json slices no longer supported. #881
Conversation
WalkthroughImports of the internal reflect utilities were updated to github.com/konveyor/tackle2-hub/model/reflect across multiple packages. In model/reflect/fields.go, slice handling was broadened to include all slice types (not just []byte) when reflecting fields. No exported signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as API (Sort)
participant RF as model/reflect.Fields
participant Sorter as Sort Logic
Client->>API: GET /... sort=labels
API->>RF: Fields(model)
Note over RF: Now includes all slice fields (e.g., labels []string)
RF-->>API: Field map incl. slice values
API->>Sorter: Compare using extracted fields
Sorter-->>Client: Sorted results
Note over API,Client: Error "labels not supported by sort" no longer occurs
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes(no out-of-scope changes identified) Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Signed-off-by: Jeff Ortel <[email protected]>
jortel
force-pushed
the
sort-json-slices
branch
from
6379ce7 to
1ad404f
Compare
| switch inst.(type) { | ||
| case []byte: | ||
| mp[ft.Name] = fv.Interface() | ||
| } |
There was a problem hiding this comment.
hint: In the past json fields stored as []byte. Now json fields are stored as regular Go types (Eg: []string). This changed and the sort package did not get updated.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
reflect/fields.go (4)
14-15: Doc nit: clarify slice/array handling for sortingNice to see sorting called out. Consider making the behavior explicit (slices included; arrays skipped). This helps future readers understand why sorting on labels (JSON slices) is supported now.
Apply this diff to the comment:
-// Fields returns a map of fields. -// Used for: -// - db.Updates() -// - sorting +// Fields returns a map of exported struct field names to their values. +// Used for: +// - db.Updates() +// - sorting (slice fields are included; array fields are skipped)
16-23: Avoid panic on nil pointers when inspectingIf Fields is called with a nil pointer,
mv.Elem()will panic. While call sites likely pass non-nil structs, adding a small guard improves robustness at little cost.- if mt.Kind() == reflect.Ptr { - mt = mt.Elem() - mv = mv.Elem() - } + if mt.Kind() == reflect.Ptr { + if mv.IsNil() { + return // nothing to collect from a nil pointer + } + mt = mt.Elem() + mv = mv.Elem() + }
65-73: NameOf:mv.IsNil()on non-pointers can panic
reflect.Value.IsNil()panics unless the kind is pointer/map/slice/chan/func/unsafe.Pointer. Guard by checking kind first.- mv := reflect.ValueOf(m) - if mv.IsNil() { - return - } - if mt.Kind() == reflect.Ptr { + mv := reflect.ValueOf(m) + if mt.Kind() == reflect.Ptr { + if mv.IsNil() { + return + } mt = mt.Elem() mv = mv.Elem() }
119-126: HasFields: nil embedded pointer structs can panic during recursive inspectFor anonymous pointer fields,
inspect(fv.Interface())will panic whenfv.IsNil(). Although the function has a recover, avoiding the panic is cleaner and prevents masking other issues.- case reflect.Ptr: - if ft.Anonymous { - inspect(fv.Interface()) - continue - } - inst := fv.Interface() - mp[ft.Name] = inst + case reflect.Ptr: + if ft.Anonymous { + if fv.IsNil() { + // Create a zero value of the element type to safely inspect field names. + inspect(reflect.New(ft.Type.Elem()).Interface()) + } else { + inspect(fv.Interface()) + } + continue + } + mp[ft.Name] = fv.Interface()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
reflect/fields.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: test-unit
🔇 Additional comments (3)
reflect/fields.go (3)
51-55: Including slices via default branch looks correct for restoring label sortingBy keeping arrays skipped and allowing slice kinds through the default path, JSON-backed slice fields (e.g., []string labels) will now be visible to the sorting layer. This aligns with the PR objective.
51-55: Nodb.Updates(Fields(...))Usages Found
A search forUpdates(Fields(returned no call sites in the codebase, soFields()isn’t used to build GORM update maps. Slice fields will not leak into anydb.Updates()calls—no further action is needed.
30-41: No pointer-to-slice fields found; skipping them poses no risk
Our search for*[]patterns in Go structs returned zero matches, so there are no pointer-to-slice fields in sortable models. No special-case handling is needed.
Signed-off-by: Jeff Ortel <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
task/manager.go (1)
23-23: LGTM: import path updated to model/reflectKeeps Manager.Update/next using reflect.Select consistently after the package move.
Same import-path verification script applies as noted in task/task.go.
🧹 Nitpick comments (1)
model/reflect/fields.go (1)
11-15: Fields used for sorting: verify slice coverage meets the labels use-caseNoting the doc update for sorting. With the current implementation, non-pointer slices are included in the map, which should allow api/sort to accept fields like Labels (typically a JSON slice) and avoid returning SortError.
Two follow-ups:
- Please confirm the model field for labels is a non-pointer slice (e.g., []string or []SomeType). Pointer-to-slice fields remain excluded in the Ptr branch.
- Add/extend unit tests for Fields to cover:
- []string and []struct fields are present
- arrays are skipped
- pointer-to-slice behavior is as intended
I can draft these tests if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
api/base.go(1 hunks)api/sort/sort.go(1 hunks)model/reflect/fields.go(1 hunks)task/manager.go(1 hunks)task/task.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- api/base.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: vet
- GitHub Check: build
- GitHub Check: test-unit
🔇 Additional comments (2)
task/task.go (1)
17-17: ✅ Import path update validated
- No stale imports of
github.com/konveyor/tackle2-hub/reflectremainSelectandFieldsfunctions confirmed inmodel/reflectAll set!
api/sort/sort.go (1)
7-7: LGTM: import path updated; with slice fields now recognized,sort=labelsshould no longer 400Sort.inspect relies on
reflect.Fieldsto validate user‐specified sort keys—sincereflect.Fieldsnow includes slice fields (e.g. theLabels []stringonDependency),"labels"won’t trigger aSortError, fixing the UI break reported in #880.Please verify end-to-end that:
sort=labelsandsort=d:labelsare accepted on the dependencies endpoint- the database orders results without error for the underlying column type (JSON/JSONB/TEXT)
If your
labelsare stored via a true many-to-many relation rather than a JSON/TEXT column, you’ll still need to alias a computed/flattened column (e.g. viaSort.Add) to prevent ORDER BY errors—let me know if you’d like help wiring that.
closes #880
Summary by CodeRabbit
Bug Fixes
Refactor