-
Notifications
You must be signed in to change notification settings - Fork 5
Adding a new constraint to baton config schema #674
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdded CONSTRAINT_KIND_ALLOWED_OPTIONS to the proto, passed the full args struct into constraint validation, and implemented allowed-option enforcement for primary fields when specified secondary fields are present; tests added to cover AllowedOptions scenarios. (47 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
| string name = 4; // optional | ||
| string help_text = 5; // optional | ||
| bool is_field_group = 6; | ||
| repeated string allowed_option_values = 7; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StringField already has a StringRules that lets you specify a whitelist of allowed values.
baton-sdk/proto/c1/config/v1/rules.proto
Line 178 in 562e873
| repeated string in = 10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The StringRules.in field handles static validation... a fixed whitelist that applies unconditionally. Our case is different: we need conditional constraints where the allowed values for one field change based on the value of another field.
Concrete example from the GitHub connector: when creating a team, the privacy field normally allows secret or closed. But if the user sets parent (making it a child team), privacy must be restricted to only closed. This is a cross-field dependency that StringRules.in can't express.
More details and a video walkthrough: DIR-146
I implemented this with Constraint messages on the action schema:
Github PR with an example code is shared below:
Constraints: []*config.Constraint{
{
Kind: config.ConstraintKind_CONSTRAINT_KIND_ALLOWED_OPTIONS,
FieldNames: []string{"privacy"},
SecondaryFieldNames: []string{"parent"},
AllowedOptionValues: []string{"closed"}, // options limited only when parent is set
Name: "Privacy must be closed if parent is set",
HelpText: "Privacy must be closed if parent is set",
},
},
The alternative would be to add conditional logic directly to the rules, something like:
message StringRules {
// ... existing fields
// Conditionally override this rule when another field matches a value.
ConditionWhen when = 28;
}
message ConditionWhen {
// The field whose value triggers this condition.
string field_name = 1;
// The condition applies when the field matches any of these values.
repeated string field_values = 2;
}
But that couples validation rules to cross-field awareness, which gets complex quickly (nested conditions, multiple dependencies, etc.).
What do you think?
|
Add some unit tests. |
7c35e9a to
63e3bd3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/actions/actions.go`:
- Around line 608-637: The AllowedOptions branch (case
ConstraintKind_CONSTRAINT_KIND_ALLOWED_OPTIONS) currently skips enforcement when
c.GetSecondaryFieldNames() is empty; modify the logic that computes
anySecondaryPresent (derived from uniqueSecondaryFieldNames :=
deduplicateStrings(c.GetSecondaryFieldNames())) so that an empty secondary list
is treated as "always applicable" (i.e., set anySecondaryPresent = true when
len(uniqueSecondaryFieldNames) == 0), then proceed with building allowedValues
and validating args/uniqueFieldNames as before; update the check that loops
present[name] to only run when there are secondary names, otherwise fallback to
the always-applicable path so unconditional AllowedOptions are enforced.
🧹 Nitpick comments (1)
pkg/actions/actions_test.go (1)
468-571: Add a test for “no secondary fields” enforcement.
These tests cover conditional enforcement, but there’s no case ensuring AllowedOptions applies whensecondary_field_namesis empty (unconditional mode). Adding one will guard the expected behavior.➕ Suggested test
t.Run("Primary field absent passes regardless - AllowedOptions", func(t *testing.T) { @@ require.NoError(t, err) }) + +t.Run("No secondary fields enforces allowed options - AllowedOptions", func(t *testing.T) { + constraints := []*config.Constraint{ + config.Constraint_builder{ + Kind: config.ConstraintKind_CONSTRAINT_KIND_ALLOWED_OPTIONS, + FieldNames: []string{"field_a"}, + AllowedOptionValues: []string{"value_a", "value_b"}, + }.Build(), + } + args := &structpb.Struct{ + Fields: map[string]*structpb.Value{ + "field_a": structpb.NewStringValue("value_c"), + }, + } + err := validateActionConstraints(constraints, args) + require.Error(t, err) + require.Contains(t, err.Error(), "not allowed") +})
Introduces new constraint kind enum
CONSTRAINT_KIND_ALLOWED_OPTIONSandallowed_option_valuesto the constraint message proto. This allows us to expand the capability of our constraints to be able to limit string field options to only to allowed values based on a dependent (secondary_field_names) field selection.Summary by CodeRabbit