Skip to content

[BB-1128] Add uhttp client#44

Merged
btipling merged 7 commits intomainfrom
add_uhttp_client
Jul 22, 2025
Merged

[BB-1128] Add uhttp client#44
btipling merged 7 commits intomainfrom
add_uhttp_client

Conversation

@aldevv
Copy link
Copy Markdown
Contributor

@aldevv aldevv commented Jul 21, 2025

Description

  • Bug fix
  • New feature

Useful links:

@aldevv aldevv requested review from a team and btipling July 21, 2025 15:35
cursor[bot]

This comment was marked as outdated.

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: LogBody Function Causes Panics and Data Loss

The logBody function reads the HTTP response body for logging. This introduces two issues: it does not check if response.Body is nil, which can cause a panic; and it consumes the body, making it unavailable for subsequent reads by other components (e.g., client.go, uhttp framework) that expect to process it.

pkg/onprem/helpers.go#L12-L25

func logBody(ctx context.Context, response *http.Response) {
if response == nil {
return
}
l := ctxzap.Extract(ctx)
body := make([]byte, 10000)
n, err := response.Body.Read(body)
if err != nil {
l.Error("error reading response body", zap.Error(err))
return
}
l.Info("response body: ", zap.String("body", string(body[:n])))
}

Fix in CursorFix in Web


Bug: API Error Masked as Success

The ListExternalGroupMembers function incorrectly returns a nil error and potentially invalid users data when the GitLab API responds with a 403 Forbidden status code. This misrepresents an API error as a successful operation, leading to consumers receiving corrupted or incomplete data.

pkg/connector/gitlab/groups_endpoints.go#L59-L76

func (o *Client) ListExternalGroupMembers(ctx context.Context, groupId string) ([]*gitlabSDK.PendingInvite, *gitlabSDK.Response, error) {
users, res, err := o.Invites.ListPendingGroupInvitations(groupId, &gitlabSDK.ListPendingInvitationsOptions{
ListOptions: gitlabSDK.ListOptions{},
},
gitlabSDK.WithContext(ctx),
)
if err != nil {
if res == nil {
return nil, nil, fmt.Errorf("gitlab-connector: error listing external group members: %w", err)
}
// handle the case where the user does not have permissions to the external group
if res.StatusCode != http.StatusForbidden {
return nil, res, err
}
}
return users, res, nil
}

Fix in CursorFix in Web


Was this report helpful? Give feedback by reacting with 👍 or 👎

Copy link
Copy Markdown
Contributor

@luisina-santos luisina-santos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should remove gitlabSDK usages for all versions as part of this change

@luisina-santos luisina-santos self-requested a review July 21, 2025 20:01
Copy link
Copy Markdown
Contributor

@luisina-santos luisina-santos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's okay as initial PR, we could remove the sdk in a next PR?

@btipling btipling merged commit abc0bc9 into main Jul 22, 2025
5 checks passed
@btipling btipling deleted the add_uhttp_client branch July 22, 2025 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants