Conversation
WalkthroughAdds utilities and a public API to build an "update_profile" action schema: a helper to convert snake_case to Title Case display names and 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
Comment |
phoebesimon
changed the title
pkg/actions/actions.go
Outdated
|
|
||
| // Validate constraints | ||
| if schema != nil { | ||
| if err := validateActionConstraints(schema.GetConstraints(), args); err != nil { |
There was a problem hiding this comment.
I'm pretty sure some of our connectors have incorrect schemas in existing actions. How can we find these before breaking stuff?
There was a problem hiding this comment.
I thought about this and we should just validate schemas all the time. Hopefully our connectors have CI tests that invoke actions, and so upgrading baton-sdk in them would cause the test to fail and force us to fix the schema.
| IsRequired: true, | ||
| ResourceIdField: &config.ResourceIdField{ | ||
| Rules: &config.ResourceIDRules{ | ||
| AllowedResourceTypeIds: []string{"user"}, |
There was a problem hiding this comment.
I think we have some connectors with resource type IDs other than "user" that have the user trait. If this action is registered on a specific resource type, wouldn't the input here always be a resource of that type?
There was a problem hiding this comment.
not necessarily, this is just a user_id field on the schema, not the resource type on the schema itself. These can be anything
| Name: "custom_attributes", | ||
| DisplayName: "Custom Attributes", | ||
| Description: "Additional custom attributes to set on the user", | ||
| StringMapField: &config.StringMapField{}, |
There was a problem hiding this comment.
How are custom attributes supposed to be handled? eg: If I make an action and use NewUpdateProfileSchema(true, ["employee_id"]) for the schema, then invoke the update_profile action with args...
{
"employee_id": "1234",
"custom_attributes": {
"employee_id": "5678"
}
}...what is supposed to happen? It's fine if the answer is, "Don't do that."
Also how does this schema handle updating a subset of attributes? If it only adds/updates attributes that are provided (and doesn't delete the ones not present), then I don't think there's a way to delete an attribute. They can only be set to empty string.
There was a problem hiding this comment.
@ggreer as for what is supposed to happen, that's sorta connector dependent. For example, in the demo connector, if you provided an email and a custom_attributes.email, we would set the email of the user to the former, and we have a profile map object where we would include the latter. There may be other connectors where a configuration like that is nonsensical and yeah, the answer would be "Don't do that". Although, now that I'm thinking about it more, C1 might not even let you configure that because of the name conflict. In which case, you could only do that if you invoke the action directly.
As of now, my intention was that connectors should only update the set of attributes they learn about, which does mean you can only empty-string those values. That felt like the safest path, but we still have the opportunity to revisit that. It's also not enforced in any way, just guidance. C1 sends every field for the user it can calculate a value for, even if it's the empty string (iirc), so again, a connector could decide to treat missing values as "delete-this-field" (whatever that means for that particular connector)
| } | ||
|
|
||
| // validateActionConstraints validates that the provided args satisfy the schema constraints. | ||
| func validateActionConstraints(constraints []*config.Constraint, args *structpb.Struct) error { |
There was a problem hiding this comment.
Any chance we could use the fields.Validate() function for this? I think it would involve implementing the Configurable interface for the args struct.
![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)
pkg/actions/actions_test.go (1)
590-600: Clarify the test name for readability.The test name "schema without custom attributes disabled" uses a double negative that's confusing. Consider renaming to something clearer like "schema with custom attributes disabled".
💡 Suggested fix
- t.Run("schema without custom attributes disabled", func(t *testing.T) { + t.Run("schema with custom attributes disabled", func(t *testing.T) {
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/actions/actions.gopkg/actions/actions_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-02T17:21:01.723Z
Learnt from: ggreer
Repo: ConductorOne/baton-sdk PR: 616
File: pkg/synccompactor/compactor_test.go:44-52
Timestamp: 2026-01-02T17:21:01.723Z
Learning: In tests that verify cleanup behavior (e.g., ensuring temporary artifacts are removed or directories are empty after an operation), treat cleanup failures as test failures by asserting on the cleanup call (e.g., require.NoError(t, err) or assert.NoError(t, err)). This ensures that the cleanup path is explicitly tested and any cleanup error fails the test, confirming correct behavior of the cleanup code.
Applied to files:
pkg/actions/actions_test.go
🧬 Code graph analysis (1)
pkg/actions/actions.go (6)
pb/c1/connector/v2/action.pb.go (6)
BatonActionSchema(148-161)BatonActionSchema(174-174)BatonActionSchema_builder(276-288)ActionType(82-82)ActionType(136-138)ActionType(140-142)pb/c1/config/v1/config.pb.go (10)
Constraint(309-319)Constraint(332-332)Field(549-572)Field(585-585)ResourceIdField(1479-1485)ResourceIdField(1498-1498)StringField(2069-2079)StringField(2092-2092)StringMapField(1902-1908)StringMapField(1921-1921)vendor/google.golang.org/grpc/rpc_util.go (1)
Errorf(973-975)vendor/golang.org/x/text/cases/cases.go (1)
Title(82-84)pb/c1/config/v1/rules.pb.go (2)
ResourceIDRules(1430-1435)ResourceIDRules(1448-1448)pb/c1/config/v1/rules_protoopaque.pb.go (2)
ResourceIDRules(1467-1472)ResourceIDRules(1485-1485)
🔇 Additional comments (11)
pkg/actions/actions.go (9)
8-8: LGTM!The new imports are appropriate for the added functionality:
stringsfor string manipulation,configfor constraint types, andcases/languagefor title-case conversion.Also applies to: 12-12, 18-19
425-425: LGTM!The constraint validation is correctly placed after handler lookup and before action execution. The nil-check for schema handles backward compatibility with handlers that may have been registered without schemas.
Also applies to: 432-437
501-518: LGTM!The resource action validation correctly fetches the schema and validates constraints. Using
codes.Internalfor missing schemas is appropriate since handlers and schemas should always be registered together for resource actions.
550-573: LGTM!The function correctly handles nil/empty constraints with an early return and properly builds the presence map by filtering out null values. The nil-safe
args.GetFields()call handles nil args gracefully.
575-616: LGTM!The constraint validation logic correctly implements all constraint kinds:
REQUIRED_TOGETHER: All-or-nothing semanticsMUTUALLY_EXCLUSIVE: At most one allowedAT_LEAST_ONE: Minimum one requiredDEPENDENT_ON: Primary fields require secondary fieldsThe deduplication of field names prevents incorrect validation when duplicates are present in the constraint definition.
618-629: LGTM!Clean and efficient deduplication implementation with O(n) time complexity. Preserving insertion order is a good choice for consistent error messages.
631-634: LGTM!Concise null value check. The type assertion safely handles nil values, returning false appropriately.
636-641: LGTM!Simple and effective snake_case to Title Case conversion using the
golang.org/x/text/casespackage with proper locale handling.
643-688: LGTM!The schema builder correctly implements the update_profile action conventions:
- Required
user_idwith proper resource type constraint- Dynamic attribute fields as optional StringFields
- Optional
custom_attributesmap for extensibilityThe function is well-documented and follows the builder pattern consistently.
pkg/actions/actions_test.go (2)
201-468: LGTM!Comprehensive test coverage for constraint validation including:
- All constraint kinds with both passing and failing cases
- Edge cases like null values, nil args, and duplicate field names
Consider adding a test for unknown constraint kinds to verify the error path, but this is optional as the default case is a defensive measure.
603-622: LGTM!Clean table-driven tests covering various snake_case patterns including the empty string edge case. The test cases align well with expected profile attribute names.
phoebesimon
force-pushed
the
phoebeyu/expand-action-schema
branch
from
5d678f0 to
0801d1b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/actions/actions.go`:
- Around line 653-683: In NewUpdateProfileSchema, filter attributeNames to
remove duplicates and any reserved names ("user_id", "custom_attributes") before
building fields so you don't append conflicting or duplicate config.Field
entries; specifically dedupe the slice (preserve first occurrence) and skip any
name equal to "user_id" or "custom_attributes" before the loop that appends
config.Field_builder entries, and ensure the conditional block that appends the
"custom_attributes" field remains unaffected.
… with conventions used for attribute push
phoebesimon
force-pushed
the
phoebeyu/expand-action-schema
branch
from
0801d1b to
66e5333
Compare
2 participants
Adds a helper to create action schemas compatible with the conventions we want to use for attribute push actions
See: ConductorOne/baton-demo#92
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.