Add Support for User Attributes API to Schema. - #72
Conversation
|
There's a problem with the integration tests, when updating the User Attribute. I get a I don't see anything in my Permit API Log. |
There was a problem hiding this comment.
Pull request overview
Adds first-class SDK support for the Permit User Attributes (schema) CRUD endpoints, enabling ABAC user-trait schema management via the Go client (resolves #71).
Changes:
- Introduces a new
Api.UserAttributessub-client with List/Get/Create/Update/Delete helpers and defaultresource_id="__user". - Adds
UserAttributeCreate,UserAttributeUpdate, andUserAttributeReadmodels, and extendsAttributeTypewithobjectandobject_array. - Updates the generated OpenAPI
UserAttributesservice types to use the new user-attribute models and adds integration coverage.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/api/api.go | Wires the new UserAttributes sub-client into PermitApiClient. |
| pkg/api/userAttributes.go | New high-level SDK client for user schema attributes CRUD. |
| pkg/openapi/api_user_attributes.go | Updates OpenAPI client method/request/response types for user attributes. |
| pkg/models/model_user_attribute_create.go | New create payload model for user attributes. |
| pkg/models/model_user_attribute_update.go | New patch payload model for user attributes. |
| pkg/models/model_user_attribute_read.go | New read model for user attributes responses. |
| pkg/models/model_attribute_type.go | Adds object and object_array attribute types to the enum. |
| pkg/tests/integration_test.go | Adds integration test coverage for user attributes CRUD. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| attrs, _, err := a.client.UserAttributesApi.ListUserAttributes(ctx, a.config.Context.GetProject(), a.config.Context.GetEnvironment()). | ||
| ResourceId(resourceID).Page(int32(page)).PerPage(int32(perPage)).Execute() | ||
| if err != nil { | ||
| a.logger.Error("error listing user attributes", zap.Error(err)) | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
UserAttributes API methods are discarding the *http.Response from the OpenAPI client and returning the raw error. This bypasses errors.HttpErrorHandle (used across most other pkg/api clients) and can cause callers to miss structured PermitError types/status codes. Capture the response from Execute() and pass (err, httpRes) through errors.HttpErrorHandle in List/Get/Create/Update/Delete (incl. the ForResource variants).
| import ( | ||
| "encoding/json" | ||
| "time" | ||
| ) | ||
|
|
||
| // UserAttributeRead is a user schema attribute as returned by the V2 API. | ||
| type UserAttributeRead struct { | ||
| Type AttributeType `json:"type"` | ||
| Description *string `json:"description,omitempty"` | ||
| Key string `json:"key"` | ||
| Id string `json:"id"` | ||
| ResourceId string `json:"resource_id"` | ||
| ResourceKey string `json:"resource_key"` | ||
| OrganizationId string `json:"organization_id"` | ||
| ProjectId string `json:"project_id"` | ||
| EnvironmentId string `json:"environment_id"` | ||
| CreatedAt time.Time `json:"created_at"` | ||
| UpdatedAt time.Time `json:"updated_at"` | ||
| BuiltIn bool `json:"built_in"` | ||
| } | ||
|
|
||
| func NewUserAttributeReadWithDefaults() *UserAttributeRead { | ||
| return &UserAttributeRead{} | ||
| } | ||
|
|
||
| func (o *UserAttributeRead) GetKey() string { | ||
| if o == nil { | ||
| return "" | ||
| } | ||
| return o.Key | ||
| } | ||
|
|
||
| func (o *UserAttributeRead) GetId() string { | ||
| if o == nil { | ||
| return "" | ||
| } | ||
| return o.Id | ||
| } | ||
|
|
||
| func (o *UserAttributeRead) GetType() AttributeType { | ||
| if o == nil { | ||
| return "" | ||
| } | ||
| return o.Type | ||
| } | ||
|
|
||
| func (o *UserAttributeRead) GetBuiltIn() bool { | ||
| if o == nil { | ||
| return false | ||
| } | ||
| return o.BuiltIn | ||
| } | ||
|
|
||
| func (o *UserAttributeRead) GetDescription() string { | ||
| if o == nil || IsNil(o.Description) { | ||
| return "" | ||
| } | ||
| return *o.Description | ||
| } | ||
|
|
||
| func (o UserAttributeRead) MarshalJSON() ([]byte, error) { | ||
| m := map[string]interface{}{ | ||
| "type": o.Type, "key": o.Key, "id": o.Id, | ||
| "resource_id": o.ResourceId, "resource_key": o.ResourceKey, | ||
| "organization_id": o.OrganizationId, "project_id": o.ProjectId, | ||
| "environment_id": o.EnvironmentId, "created_at": o.CreatedAt, | ||
| "updated_at": o.UpdatedAt, "built_in": o.BuiltIn, | ||
| } | ||
| if !IsNil(o.Description) { | ||
| m["description"] = o.Description | ||
| } | ||
| return json.Marshal(m) | ||
| } |
There was a problem hiding this comment.
These UserAttribute* models duplicate the existing ResourceAttributeCreate/Update/Read schema almost 1:1. To avoid drift and keep the generated helper methods/Nullable wrappers consistent across the SDK, consider making these types aliases/wrappers around the existing ResourceAttribute* models (or regenerating from the OpenAPI spec) instead of maintaining a separate parallel struct set.
| import ( | |
| "encoding/json" | |
| "time" | |
| ) | |
| // UserAttributeRead is a user schema attribute as returned by the V2 API. | |
| type UserAttributeRead struct { | |
| Type AttributeType `json:"type"` | |
| Description *string `json:"description,omitempty"` | |
| Key string `json:"key"` | |
| Id string `json:"id"` | |
| ResourceId string `json:"resource_id"` | |
| ResourceKey string `json:"resource_key"` | |
| OrganizationId string `json:"organization_id"` | |
| ProjectId string `json:"project_id"` | |
| EnvironmentId string `json:"environment_id"` | |
| CreatedAt time.Time `json:"created_at"` | |
| UpdatedAt time.Time `json:"updated_at"` | |
| BuiltIn bool `json:"built_in"` | |
| } | |
| func NewUserAttributeReadWithDefaults() *UserAttributeRead { | |
| return &UserAttributeRead{} | |
| } | |
| func (o *UserAttributeRead) GetKey() string { | |
| if o == nil { | |
| return "" | |
| } | |
| return o.Key | |
| } | |
| func (o *UserAttributeRead) GetId() string { | |
| if o == nil { | |
| return "" | |
| } | |
| return o.Id | |
| } | |
| func (o *UserAttributeRead) GetType() AttributeType { | |
| if o == nil { | |
| return "" | |
| } | |
| return o.Type | |
| } | |
| func (o *UserAttributeRead) GetBuiltIn() bool { | |
| if o == nil { | |
| return false | |
| } | |
| return o.BuiltIn | |
| } | |
| func (o *UserAttributeRead) GetDescription() string { | |
| if o == nil || IsNil(o.Description) { | |
| return "" | |
| } | |
| return *o.Description | |
| } | |
| func (o UserAttributeRead) MarshalJSON() ([]byte, error) { | |
| m := map[string]interface{}{ | |
| "type": o.Type, "key": o.Key, "id": o.Id, | |
| "resource_id": o.ResourceId, "resource_key": o.ResourceKey, | |
| "organization_id": o.OrganizationId, "project_id": o.ProjectId, | |
| "environment_id": o.EnvironmentId, "created_at": o.CreatedAt, | |
| "updated_at": o.UpdatedAt, "built_in": o.BuiltIn, | |
| } | |
| if !IsNil(o.Description) { | |
| m["description"] = o.Description | |
| } | |
| return json.Marshal(m) | |
| } | |
| // UserAttributeRead is a user schema attribute as returned by the V2 API. | |
| // It is an alias of the canonical ResourceAttributeRead model to avoid | |
| // duplication and drift in field definitions and helper methods. | |
| type UserAttributeRead = ResourceAttributeRead | |
| func NewUserAttributeReadWithDefaults() *UserAttributeRead { | |
| return (*UserAttributeRead)(NewResourceAttributeReadWithDefaults()) | |
| } |
zeevmoney
left a comment
zeevmoney
left a comment
There was a problem hiding this comment.
Thank you for your contribution. See comments.
| if err != nil { | ||
| a.logger.Error("error listing user attributes", zap.Error(err)) | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
HTTP response discarded — missing errors.HttpErrorHandle call
Every other API wrapper in this codebase (see resources.go, roles.go, tenants.go, users.go, etc.) captures the *http.Response return value and passes it through errors.HttpErrorHandle(err, httpRes). This converts HTTP status codes into the SDK's typed error hierarchy (PermitNotFoundError, PermitConflictError, PermitForbiddenError, etc.).
In userAttributes.go, all five methods (ListForResource, GetForResource, CreateForResource, UpdateForResource, DeleteForResource) discard the HTTP response with _, so callers receive raw OpenAPI errors instead of typed PermitError values. This breaks error handling for any consumer that uses type assertions or checks ErrorCode on the returned error.
Suggestion: Capture the *http.Response and call errors.HttpErrorHandle, matching the pattern used everywhere else:
attrs, httpRes, err := a.client.UserAttributesApi.ListUserAttributes(...).Execute()
err = errors.HttpErrorHandle(err, httpRes)
if err != nil {
a.logger.Error("error listing user attributes", zap.Error(err))
return nil, err
}Apply the same fix to all five ForResource methods.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| attrs, _, err := a.client.UserAttributesApi.ListUserAttributes(ctx, a.config.Context.GetProject(), a.config.Context.GetEnvironment()). |
There was a problem hiding this comment.
Same errors.HttpErrorHandle gap applies to all five *ForResource methods in this file (lines 50, 72, 106, 126, 146). Each discards the *http.Response with _ instead of passing it through errors.HttpErrorHandle(err, httpRes) like every other API method in the codebase.
See the comment on line 55 for the full explanation and suggested fix.
| m["description"] = o.Description | ||
| } | ||
| return json.Marshal(m) | ||
| } |
There was a problem hiding this comment.
Missing HasDescription() method — inconsistent with ResourceAttributeCreate
The existing ResourceAttributeCreate model provides HasDescription() (and *Ok() tuple-return methods) that allow callers to distinguish between "not set" and "set to empty string" for optional fields. UserAttributeCreate only has Get/Set methods and is missing HasDescription(). Since these two types serve the same role (attribute creation) for different resource types, callers will expect the same API surface.
Suggestion: Add HasDescription() to match the existing pattern:
func (o *UserAttributeCreate) HasDescription() bool {
return o != nil && !IsNil(o.Description)
}| m["description"] = o.Description | ||
| } | ||
| return json.Marshal(m) | ||
| } |
There was a problem hiding this comment.
Custom MarshalJSON without matching UnmarshalJSON — potential data loss on round-trip
UserAttributeRead defines a custom MarshalJSON but relies on the default struct-tag-based UnmarshalJSON. While this works for basic deserialization from the API, it creates an asymmetry: the custom MarshalJSON serializes Description conditionally (only when non-nil), but if someone marshals and then unmarshals a UserAttributeRead with a nil Description, the behavior is correct only by coincidence of omitempty tag alignment.
More concretely, ResourceAttributeRead (the analogous type) does not define a custom MarshalJSON at all — it uses the generated code pattern with toSerialize map. The new UserAttributeRead introduces a divergent serialization style.
Suggestion: Either remove the custom MarshalJSON and rely on struct tags (since the json tags already handle the serialization correctly with omitempty), or document why a custom implementation is needed. The struct tags as defined are sufficient for correct JSON output.
| uAttrCreate.SetDescription("integration test user attribute") | ||
| uAttr, err := permitClient.Api.UserAttributes.Create(ctx, *uAttrCreate) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, uAttrKey, uAttr.Key) |
There was a problem hiding this comment.
No nil-check on returned pointer before field access
After Create returns (uAttr, err), the code immediately accesses uAttr.Key and uAttr.GetType() without checking uAttr != nil. If the API call fails (e.g. due to network or auth issues), assert.NoError(t, err) will log the error but not stop the test — assert (unlike require) continues execution. The subsequent uAttr.Key access on a nil pointer will panic, crashing the test runner and hiding the actual error.
The same pattern repeats for gotUAttr (line 261) and updatedUAttr (line 267).
Suggestion: Use require.NoError(t, err) instead of assert.NoError(t, err) for API calls where the returned value is immediately dereferenced. require stops the test on failure, preventing nil pointer panics:
require.NoError(t, err)
assert.Equal(t, uAttrKey, uAttr.Key)
Resolves #71