Revamp Retool connector to use API v2 instead of direct DB queries#32
Revamp Retool connector to use API v2 instead of direct DB queries#32c1-dev-bot[bot] wants to merge 2 commits into
Conversation
Replace the entire Postgres database client (pgx/scany) with an HTTP REST client that uses the Retool API v2. This eliminates the need for direct database access and uses the officially supported API endpoints. Key changes: - Replace database connection with API token + base URL configuration - Rewrite pkg/client/ to use net/http REST calls to Retool API v2 - Add Account provisioning capability (create users via POST /api/v2/users) - Use Permissions API for app/resource access grants instead of direct SQL - Groups API returns members inline (no separate member queries needed) - Remove pgx, pgxscan, and related database dependencies - Update README with new setup instructions API endpoints used: - GET/POST /api/v2/users - List and create users - GET /api/v2/groups, POST/DELETE members - Group management - POST /api/v2/permissions/grant|revoke - Permission management - GET /api/v2/organization - Organization info - GET /api/v2/apps, /api/v2/resources - App and resource listing Fixes: CXH-1263
|
Claude finished @c1-dev-bot[bot]'s task in 2m 57s —— View job PR Review: Revamp Retool connector to use API v2
Overall AssessmentThis is a well-structured rewrite that cleanly replaces the Postgres database driver with a REST API client. The generic However, there are several bugs and issues that should be addressed before merging: Bugs (must fix)
Issues (should fix)
Positive Notes
SummaryThe architecture is sound and the API client is well-designed. The critical path issue is #1 (broken |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if client.IsConflict(err) { | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
Bug: Per CLAUDE.md conventions, "already exists" on Grant should return the GrantAlreadyExists annotation, not bare nil. The current code silently swallows the conflict.
| if client.IsConflict(err) { | |
| return nil, nil | |
| } | |
| if client.IsConflict(err) { | |
| return annotations.New(&v2.GrantAlreadyExists{}), nil | |
| } |
| func IsNotFound(err error) bool { | ||
| if apiErr, ok := err.(*APIError); ok { | ||
| return apiErr.StatusCode == http.StatusNotFound | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
Bug: Direct type assertion won't match wrapped errors. Since doRequest wraps API errors with fmt.Errorf("...: %w", err), callers like groups.go and pages.go further wrap them. By the time IsNotFound/IsConflict are called, the *APIError is nested and this assertion fails — meaning conflict/not-found detection is silently broken.
Use errors.As instead:
| func IsNotFound(err error) bool { | |
| if apiErr, ok := err.(*APIError); ok { | |
| return apiErr.StatusCode == http.StatusNotFound | |
| } | |
| return false | |
| } | |
| func IsNotFound(err error) bool { | |
| var apiErr *APIError | |
| if errors.As(err, &apiErr) { | |
| return apiErr.StatusCode == http.StatusNotFound | |
| } | |
| return false | |
| } |
Same fix needed for IsConflict below. You'll need to add "errors" to the imports.
| return nil, nil, nil, fmt.Errorf("baton-retool: failed to create user account: %w", err) | ||
| } | ||
|
|
||
| _ = user |
There was a problem hiding this comment.
Nit: The created user result is discarded. Consider using the returned user to populate the CreateAccountResponse with relevant info (e.g., the new user's ID), or at minimum remove the assignment to avoid the dead-code smell.
| _ = user | |
| _, err := c.client.CreateUser(ctx, &client.CreateUserRequest{ |
| return ret, nextPageToken, nil, nil | ||
| } | ||
|
|
||
| func (s *pageSyncer) Grant(ctx context.Context, resource *v2.Resource, entitlement *v2.Entitlement) ([]*v2.Grant, annotations.Annotations, error) { |
There was a problem hiding this comment.
Nit: The first parameter is named resource but it's actually the principal (the entity being granted access). This is confusing — the SDK convention names this parameter principal. Consider renaming to principal to match the groupSyncer.Grant signature at groups.go:129.
| splitV := strings.Split(entitlement.Id, ":") | ||
| if len(splitV) != 4 { | ||
| return nil, nil, fmt.Errorf("unexpected entitlement ID format while processing page grant: %s", entitlement.Id) | ||
| } | ||
| accessLevel := splitV[len(splitV)-1] |
There was a problem hiding this comment.
Fragile: This splits the entitlement ID by : and assumes exactly 4 parts. If the ID format ever changes (e.g., resource IDs containing colons), this breaks silently by extracting the wrong access level. Consider extracting the slug from entitlement.Slug directly if available, or at minimum using the last element without asserting exact length:
parts := strings.Split(entitlement.Id, ":")
accessLevel := parts[len(parts)-1]
| Id: fmt.Sprintf("grant:%s:%s", eID, principalID.Resource), | ||
| }) | ||
| newGrant := grant.NewGrant(resource, level, principalID) | ||
| newGrant.Id = fmt.Sprintf("grant:%s:%s", eID, principalID.Resource) |
There was a problem hiding this comment.
Fragile: Manually overriding the SDK-generated grant ID. If the format doesn't match what the SDK expects during revocation, grants may become unrevocable. Prefer using the SDK's grant.NewGrant() defaults or ensure the format is tested.
| ret = append(ret, &v2.Resource{ | ||
| DisplayName: fmt.Sprintf("%s %s", o.GetFirstName(), o.GetLastName()), | ||
| DisplayName: displayName, | ||
| Id: &v2.ResourceId{ | ||
| ResourceType: s.resourceType.Id, | ||
| Resource: resourceID, | ||
| }, | ||
| ExternalId: &v2.ExternalId{ | ||
| Id: o.ID.String(), | ||
| }, | ||
| ParentResourceId: parentResourceID, | ||
| Annotations: annos, | ||
| }) |
There was a problem hiding this comment.
Good: ExternalId is properly set using the native Retool user ID. This is critical for provisioning operations (Grant/Revoke) to work correctly. Well done.
| } | ||
|
|
||
| resource, err := rs.NewResource(o.GetName(), s.resourceType, formatObjectID(resourceTypeGroup.Id, o.ID), options...) | ||
| resource, err := rs.NewResource(o.GetName(), s.resourceType, formatObjectID(resourceTypeGroup.Id, o.GetID()), options...) |
There was a problem hiding this comment.
Missing ExternalId: Groups support Grant/Revoke operations but don't set WithExternalID() during sync. While the current code works by extracting the native ID from the formatted resource ID, this is fragile and inconsistent with the user syncer which properly sets ExternalId. Per CLAUDE.md: "WithExternalID() - REQUIRED for Grant/Revoke to work."
- Use errors.As instead of type assertion for wrapped error handling - Handle resp.Body.Close() return value - Fix struct field alignment in UserModel - Fix import ordering in resources.go
|
Claude finished @c1-dev-bot[bot]'s task in 2m 2s —— View job PR Review: Revamp Retool connector to use API v2
Overall AssessmentThis is a clean, well-structured rewrite that replaces ~81k lines of vendored Postgres dependencies with a focused REST API client. The generic Remaining Bugs1. Group Grant missing On conflict, returns // Current (line 151):
return nil, nil
// Should be:
return annotations.New(&v2.GrantAlreadyExists{}), nil2. Both functions index // parseObjectID - add guard:
func parseObjectID(id string) (int64, error) {
if len(id) < 2 {
return 0, fmt.Errorf("baton-retool: invalid object ID: %q", id)
}
return strconv.ParseInt(id[1:], 10, 64)
}3. Missing The func (c *ConnectorImpl) Close() error {
if c.client != nil {
return c.client.Close()
}
return nil
}Issues (should fix)4. Missing Groups support Grant/Revoke but don't set 5. First parameter is named 6. Fragile entitlement ID parsing — Splits entitlement ID by 7. Manually overridden grant IDs — Overriding SDK-generated grant IDs is fragile. If the format doesn't match what the SDK expects during revocation, grants become unrevocable. Prefer using 8. Dead assignment in
Positive Notes
SummaryThe architecture is sound. Three bugs need fixing before merge: the missing |
| if client.IsConflict(err) { | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
Bug: Per CLAUDE.md, "already exists" on Grant should return the GrantAlreadyExists annotation, not bare nil. The current code silently swallows the conflict signal.
| if client.IsConflict(err) { | |
| return nil, nil | |
| } | |
| if client.IsConflict(err) { | |
| return annotations.New(&v2.GrantAlreadyExists{}), nil | |
| } |
| func parseObjectIDString(id string) string { | ||
| return id[1:] | ||
| } |
There was a problem hiding this comment.
Same bounds-check issue as parseObjectID — will panic on empty input.
| func parseObjectIDString(id string) string { | |
| return id[1:] | |
| } | |
| func parseObjectIDString(id string) string { | |
| if len(id) < 2 { | |
| return "" | |
| } | |
| return id[1:] |
| return nil, nil, nil, fmt.Errorf("baton-retool: failed to create user account: %w", err) | ||
| } | ||
|
|
||
| _ = user |
There was a problem hiding this comment.
Nit: user is assigned then immediately discarded. Either use the returned user to populate the response (e.g. with the new user's ID) or discard at the call site:
| _ = user | |
| _, err := c.client.CreateUser(ctx, &client.CreateUserRequest{ |
| } | ||
|
|
||
| resource, err := rs.NewResource(o.GetName(), s.resourceType, formatObjectID(resourceTypeGroup.Id, o.ID), options...) | ||
| resource, err := rs.NewResource(o.GetName(), s.resourceType, formatObjectID(resourceTypeGroup.Id, o.GetID()), options...) |
There was a problem hiding this comment.
Missing WithExternalID(): Groups support Grant/Revoke but don't set ExternalId during sync. Users correctly set it at users.go:84. Per CLAUDE.md this is required for provisioning to work reliably. Consider adding:
rs.WithExternalID(o.ID.String()),to the options slice.
| return ret, nextPageToken, nil, nil | ||
| } | ||
|
|
||
| func (s *pageSyncer) Grant(ctx context.Context, resource *v2.Resource, entitlement *v2.Entitlement) ([]*v2.Grant, annotations.Annotations, error) { |
There was a problem hiding this comment.
Nit: First parameter named resource is actually the principal (the entity being granted access). This is inconsistent with groupSyncer.Grant at groups.go:129 which correctly names it principal, and with the SDK convention.
| splitV := strings.Split(entitlement.Id, ":") | ||
| if len(splitV) != 4 { | ||
| return nil, nil, fmt.Errorf("unexpected entitlement ID format while processing page grant: %s", entitlement.Id) | ||
| } | ||
| accessLevel := splitV[len(splitV)-1] |
There was a problem hiding this comment.
Fragile: Asserts exactly 4 parts after splitting by :. If the ID format ever changes (e.g. resource IDs containing colons), this breaks silently. Consider extracting the last element without asserting exact length:
| splitV := strings.Split(entitlement.Id, ":") | |
| if len(splitV) != 4 { | |
| return nil, nil, fmt.Errorf("unexpected entitlement ID format while processing page grant: %s", entitlement.Id) | |
| } | |
| accessLevel := splitV[len(splitV)-1] | |
| splitV := strings.Split(entitlement.Id, ":") | |
| if len(splitV) < 2 { | |
| return nil, nil, fmt.Errorf("unexpected entitlement ID format while processing page grant: %s", entitlement.Id) | |
| } | |
| accessLevel := splitV[len(splitV)-1] |
| Id: fmt.Sprintf("grant:%s:%s", eID, principalID.Resource), | ||
| }) | ||
| newGrant := grant.NewGrant(resource, level, principalID) | ||
| newGrant.Id = fmt.Sprintf("grant:%s:%s", eID, principalID.Resource) |
There was a problem hiding this comment.
Fragile: Manually overriding the SDK-generated grant ID. If this format doesn't match what the SDK expects during revocation, grants may become unrevocable. Prefer using grant.NewGrant() defaults unless there's a specific reason for the override.
Summary
Replaces the entire Postgres database client with an HTTP REST client that uses the Retool API v2, eliminating the need for direct database access. Also adds Account provisioning capability.
Changes
pgx/pgxscandatabase driver withnet/httpREST client targeting Retool API v2 endpoints--connection-string(DB DSN) with--api-token+--api-url(API authentication)AccountManagerinterface to create new users viaPOST /api/v2/users/api/v2/permissions/grant|revoke) for app/resource access instead of direct SQL ongroup_pages/group_resourcestablesGET /api/v2/organizationreplaces theorganizationstable querypgx,pgxscan,pgconn,pgtype,puddleand related PostgreSQL dependenciesAPI Endpoints Used
GET /api/v2/organizationGET /api/v2/usersPOST /api/v2/usersDELETE /api/v2/users/:idGET /api/v2/groupsGET /api/v2/groups/:idPOST /api/v2/groups/:id/membersDELETE /api/v2/groups/:id/members/:userIdPOST /api/v2/permissions/grantPOST /api/v2/permissions/revokeGET /api/v2/permissions/apps/:idGET /api/v2/permissions/resources/:idGET /api/v2/appsGET /api/v2/resourcesRequired API Token Scopes
Fixes: CXH-1263
Automated PR Notice
This PR was automatically created by c1-dev-bot as a potential implementation.
This code requires: