[DUCT-12852] baton-sdk: specify order in field #565
Conversation
WalkthroughAdds an Order int to SchemaField, enforces uniqueness for positive Order values during field validation, updates CLI schema output to sort fields by Order before marshaling, and adds an inline proto-side comment noting no validation rules for Order. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![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 (1)
proto/c1/config/v1/config.proto (1)
57-58: Consider adding inline documentation for the order field.While the field definition is correct, adding a brief comment would help users understand its purpose and usage. For example:
bool is_secret = 7; - int64 order = 8; + int64 order = 8; // Display order for the field in UI; 0 = unspecified, falls back to alphabeticalThis is especially helpful since some other fields in the proto have explanatory comments (e.g., lines 36, 37, 42, 50).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pb/c1/config/v1/config.pb.gois excluded by!**/*.pb.gopb/c1/config/v1/config_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (3)
pb/c1/config/v1/config.pb.validate.go(1 hunks)pkg/field/fields.go(1 hunks)proto/c1/config/v1/config.proto(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). (1)
- GitHub Check: go-test (1.25.2, windows-latest)
🔇 Additional comments (2)
pb/c1/config/v1/config.pb.validate.go (1)
503-504: LGTM! Generated validation code correctly handles the new Order field.The comment follows the established pattern for fields without validation rules, and the placement before the oneof switch is appropriate.
pkg/field/fields.go (1)
81-82: The Order field is currently unused—consider clarifying its purpose or removing if not needed.The
Orderfield is defined in the SchemaField struct with the comment "Displaying the SchemaField in order," but it's not accessed anywhere in the codebase. No sorting or ordering logic uses it. If this field is intended for future use to control field display order, document its semantics (e.g., how zero values are handled, sort order semantics). If it's not actively being used, consider whether it should be included or marked as reserved for future use.
| FieldGroups []SchemaFieldGroup | ||
|
|
||
| // Displaying the SchemaField in order. | ||
| Order int |
There was a problem hiding this comment.
not that it affects this PR, but how are we going to handle conflicts (two fields with same Order)?
There was a problem hiding this comment.
we should throw an error for this, similar to how field groups throws an error if default is set to multiple groups. @Bencheng21 what was account schema do?
There was a problem hiding this comment.
good point. Updated.
There was a problem hiding this comment.
We could check this when generating the config code, so it prevents same Order at go gen time
There was a problem hiding this comment.
sounds like we could call validate() while generating the config code, but I would put it in another PR if necessary.
proto/c1/config/v1/config.proto
Outdated
| bool is_ops = 6; | ||
| bool is_secret = 7; | ||
|
|
||
| int64 order = 8; |
There was a problem hiding this comment.
where are we going to use this order? I was expecting us to sort in baton-sdk after we make the list
baton-sdk/pkg/config/config.go
Line 129 in 3f6cd4f
we can also sort in c1 UI but figured baton-sdk would be easier
There was a problem hiding this comment.
sounds good. yeah, baton-sdk would be easier.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/field/validation.go (1)
355-377: Clarify positive-order uniqueness logic and make the error more actionableThe current code effectively enforces uniqueness only for
f.Order > 0, but that’s not obvious on first read because the duplicate check runs for all values:if _, ok := orderMap[f.Order]; ok { return fmt.Errorf("field Order should be unique") } if f.Order > 0 { orderMap[f.Order] = struct{}{} }This works, but:
- It’s a bit confusing that you look up
orderMap[f.Order]even whenf.Order <= 0, while those values are never inserted.- The error message doesn’t indicate which field or order value caused the failure, which makes debugging connector schemas harder.
You can make the intent clearer and the error more useful with a small refactor:
- if _, ok := orderMap[f.Order]; ok { - return fmt.Errorf("field Order should be unique") - } - - if f.Order > 0 { - orderMap[f.Order] = struct{}{} - } + if f.Order > 0 { + if _, ok := orderMap[f.Order]; ok { + return fmt.Errorf("field %q has a duplicate Order value %d", f.FieldName, f.Order) + } + orderMap[f.Order] = struct{}{} + }This keeps the behavior of only enforcing uniqueness for positive orders, but makes that contract explicit and surfaces enough detail to fix misconfigured schemas quickly.
pkg/cli/commands.go
Outdated
| // Sort fields by Order and then FieldName. | ||
| sort.Slice(confschema.Fields, func(i, j int) bool { | ||
| if confschema.Fields[i].Order < confschema.Fields[j].Order { | ||
| return true | ||
| } | ||
| return confschema.Fields[i].FieldName < confschema.Fields[j].FieldName | ||
| }) |
There was a problem hiding this comment.
Explicit Order semantics are inverted when some fields keep the default Order == 0
Right now all fields start at Order == 0, and you sort purely by Order then FieldName. That means any field with a positive Order value will appear after all the default-zero fields. If the intent is that smaller explicit orders come first (e.g., Order=1 → “top of the form”) and 0 means “unspecified, use default alphabetical after ordered fields”, this behavior is reversed for partially ordered schemas.
To make Order work intuitively while preserving alphabetical ordering for unspecified fields, consider treating Order == 0 as “unspecified” and placing those after any explicitly ordered fields:
- // Sort fields by Order and then FieldName.
- sort.Slice(confschema.Fields, func(i, j int) bool {
- if confschema.Fields[i].Order < confschema.Fields[j].Order {
- return true
- }
- return confschema.Fields[i].FieldName < confschema.Fields[j].FieldName
- })
+ // Sort fields by Order (explicit first) and then FieldName.
+ // Treat Order == 0 as "unspecified": they come after explicitly ordered fields,
+ // and are sorted alphabetically among themselves.
+ sort.Slice(confschema.Fields, func(i, j int) bool {
+ fi, fj := confschema.Fields[i], confschema.Fields[j]
+
+ switch {
+ case fi.Order == 0 && fj.Order == 0:
+ // Neither has explicit order: fall back to FieldName.
+ return fi.FieldName < fj.FieldName
+ case fi.Order == 0:
+ // Only j has explicit order → j comes first.
+ return false
+ case fj.Order == 0:
+ // Only i has explicit order → i comes first.
+ return true
+ case fi.Order != fj.Order:
+ // Both explicitly ordered → smaller Order first.
+ return fi.Order < fj.Order
+ default:
+ // Same explicit Order → stable, deterministic tie-breaker.
+ return fi.FieldName < fj.FieldName
+ }
+ })This keeps the previous “alphabetical by name” behavior for fields without an explicit order, while letting connectors opt into deterministic custom ordering for selected fields without having to assign Order to every single one.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Sort fields by Order and then FieldName. | |
| sort.Slice(confschema.Fields, func(i, j int) bool { | |
| if confschema.Fields[i].Order < confschema.Fields[j].Order { | |
| return true | |
| } | |
| return confschema.Fields[i].FieldName < confschema.Fields[j].FieldName | |
| }) | |
| // Sort fields by Order (explicit first) and then FieldName. | |
| // Treat Order == 0 as "unspecified": they come after explicitly ordered fields, | |
| // and are sorted alphabetically among themselves. | |
| sort.Slice(confschema.Fields, func(i, j int) bool { | |
| fi, fj := confschema.Fields[i], confschema.Fields[j] | |
| switch { | |
| case fi.Order == 0 && fj.Order == 0: | |
| // Neither has explicit order: fall back to FieldName. | |
| return fi.FieldName < fj.FieldName | |
| case fi.Order == 0: | |
| // Only j has explicit order → j comes first. | |
| return false | |
| case fj.Order == 0: | |
| // Only i has explicit order → i comes first. | |
| return true | |
| case fi.Order != fj.Order: | |
| // Both explicitly ordered → smaller Order first. | |
| return fi.Order < fj.Order | |
| default: | |
| // Same explicit Order → stable, deterministic tie-breaker. | |
| return fi.FieldName < fj.FieldName | |
| } | |
| }) |
🤖 Prompt for AI Agents
pkg/cli/commands.go around lines 677-683: the current comparator sorts purely by
Order then FieldName which causes default Order==0 fields to come before
explicitly ordered (positive) fields, inverting intended semantics; update the
comparator so fields with Order>0 come first ordered by ascending Order (and
FieldName tie-breaker), and fields with Order==0 are treated as unspecified and
placed after all explicitly ordered fields, sorted alphabetically by FieldName.
Bencheng21
force-pushed
the
ben/DUCT12852
branch
from
d8bc54a to
329b66f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/field/fields.go (1)
79-82: Consider documentingOrdersemantics onSchemaFieldThe
Order intaddition is straightforward, but its intended semantics (e.g.,>0means explicitly ordered and must be unique;0/negative means “unspecified / ignore for ordering”) are only inferable fromValidate. A short comment spelling that out here would make it easier for connector authors and future maintainers to use it correctly.pkg/field/validation.go (1)
355-361: Order uniqueness logic is sound; improve error context and make intent explicitThe
orderMapapproach correctly enforces uniqueness of positiveOrdervalues within the selected auth group, which addresses the prior concern about conflictingOrders. Right now, though:
- The error
"field Order should be unique"gives no indication which value or fields collided.- The uniqueness check runs for all
Ordervalues, even though only>0are actually tracked.You could make this easier to debug and more self-documenting by:
- Tracking the first field name per order value.
- Restricting the uniqueness check to positive orders for clarity.
For example:
@@ func Validate(c Configuration, v Configurable, opts ...Option) error { - var ( - present = make(map[string]int) - validationErrors = &ErrConfigurationMissingFields{} - - fieldGroupMap = c.FieldGroupFields(validateOpts.authGroup) - orderMap = make(map[int]struct{}, len(c.Fields)) - ) + var ( + present = make(map[string]int) + validationErrors = &ErrConfigurationMissingFields{} + + fieldGroupMap = c.FieldGroupFields(validateOpts.authGroup) + orderMap = make(map[int]string, len(c.Fields)) // positive Order -> field name + ) @@ - if _, ok := orderMap[f.Order]; ok { - return fmt.Errorf("field Order should be unique") - } - - if f.Order > 0 { - orderMap[f.Order] = struct{}{} - } + if f.Order > 0 { + if prev, ok := orderMap[f.Order]; ok { + return fmt.Errorf("field Order %d is used by both %s and %s", f.Order, prev, f.FieldName) + } + orderMap[f.Order] = f.FieldName + }This preserves the current behavior while making duplicate-order failures much easier to diagnose.
Also applies to: 371-377
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pb/c1/config/v1/config.pb.gois excluded by!**/*.pb.gopb/c1/config/v1/config_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (4)
pb/c1/config/v1/config.pb.validate.go(1 hunks)pkg/cli/commands.go(2 hunks)pkg/field/fields.go(1 hunks)pkg/field/validation.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/cli/commands.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). (1)
- GitHub Check: go-test (1.25.2, windows-latest)
🔇 Additional comments (1)
pb/c1/config/v1/config.pb.validate.go (1)
489-505: Generated validation: noting no rules forOrderis appropriateMarking
// no validation rules for Orderhere is consistent with handlingSchemaField.Orderpurely in our own validation (pkg/field/validation.go) rather than via PGV rules. Given this file is generated, the change looks correct as long as it comes from the updated proto.
|
|
||
| if _, ok := orderMap[f.Order]; ok { | ||
| return fmt.Errorf("field Order should be unique") | ||
| } |
There was a problem hiding this comment.
Hmm, there are cases where some fields have an order and some don’t. If a field doesn’t specify an order, it’ll be shown first.
There was a problem hiding this comment.
Seems reasonable to me IMO (or they could be last).
Today, we sort the fields alphabetically implicitly in c1.
We add more flexibilities to config schema and we allow specifying order in field schema.
Test.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.