Skip to content

Fix/Multiple Reply in Git Provider #37

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.

Sign up for GitHub

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

Merged
Amazing-Stardom merged 2 commits into from
May 4, 2026
Merged

Fix/Multiple Reply in Git Provider #37

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4cddce5
Implement Conditional Webhook Registry Updates
Amazing-Stardom May 4, 2026
c8725f3
Propagate Foreign Key Errors in Webhook Removal
Amazing-Stardom May 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 51 additions & 2 deletions internal/api/connectors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,56 @@ func (s *Server) GetConnector(c echo.Context) error {

// DeleteConnector handles deletion of a git provider connection
func (s *Server) DeleteConnector(c echo.Context) error {
id := c.Param("id")
idStr := c.Param("id")
id, err := strconv.Atoi(idStr)
if err != nil {
Comment thread
Amazing-Stardom marked this conversation as resolved.
return c.JSON(http.StatusBadRequest, ErrorResponse{
Error: "Invalid connector ID",
})
}

// Validate connector ownership
if _, err := s.validateConnectorOwnership(c, id); err != nil {
Copy link
Copy Markdown
Contributor Author

@Amazing-Stardom Amazing-Stardom May 4, 2026

Choose a reason for hiding this comment

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

Severity: critical

Crucial security check: ensures a user can only delete connectors they own.

Suggestions:

  1. Verify validateConnectorOwnership covers all necessary authorization checks.

return err
}

// Fetch connector details for webhook removal
var provider, providerURL, patToken string
Comment thread
Amazing-Stardom marked this conversation as resolved.
query := `
SELECT provider, provider_url, pat_token
FROM integration_tokens
WHERE id = $1
`
err = s.db.QueryRow(query, id).Scan(&provider, &providerURL, &patToken)
if err != nil {
if err == sql.ErrNoRows {
return c.JSON(http.StatusNotFound, ErrorResponse{
Error: "Connector not found",
})
}
log.Printf("Failed to fetch connector details for deletion: %v", err)
return c.JSON(http.StatusInternalServerError, ErrorResponse{
Error: "Database error: " + err.Error(),
})
}

// Fetch projects to queue webhook removal
repositoryData, err := s.fetchAndCacheRepositoryData(id, false, false)
Comment thread
Amazing-Stardom marked this conversation as resolved.
if err != nil {
log.Printf("Failed to fetch repository data for connector %d during deletion: %v", id, err)
// We log the error but proceed with connector deletion so the user isn't permanently blocked
Comment thread
Amazing-Stardom marked this conversation as resolved.
} else if repositoryData.Error != "" {
log.Printf("Repository access error during deletion of connector %d: %s", id, repositoryData.Error)
// Again, proceed to delete the connector
} else {
// Queue webhook removal jobs for each project
ctx := c.Request().Context()
for _, projectPath := range repositoryData.Projects {
if err := s.jobQueue.QueueWebhookRemovalJob(ctx, id, projectPath, provider, providerURL, patToken, true); err != nil {
Comment thread
Amazing-Stardom marked this conversation as resolved.
log.Printf("Failed to queue webhook removal job for %s: %v", projectPath, err)
}
}
}

// Execute the delete query
result, err := s.db.Exec(`
Expand All @@ -455,7 +504,7 @@ func (s *Server) DeleteConnector(c echo.Context) error {
`, id)

if err != nil {
log.Printf("Failed to delete connector with ID %s: %v", id, err)
log.Printf("Failed to delete connector with ID %d: %v", id, err)
return c.JSON(http.StatusInternalServerError, ErrorResponse{
Error: "Database error: " + err.Error(),
})
Expand Down
2 changes: 1 addition & 1 deletion internal/api/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1630,7 +1630,7 @@ func (s *Server) DisableManualTriggerForAllProjects(c echo.Context) error {
var queueErrors []string

for _, projectPath := range repositoryData.Projects {
err := s.jobQueue.QueueWebhookRemovalJob(ctx, connectorId, projectPath, provider, providerURL, patToken)
Comment thread
Amazing-Stardom marked this conversation as resolved.
err := s.jobQueue.QueueWebhookRemovalJob(ctx, connectorId, projectPath, provider, providerURL, patToken, false)
if err != nil {
queueErrors = append(queueErrors, fmt.Sprintf("Failed to queue removal job for %s: %v", projectPath, err))
} else {
Expand Down
64 changes: 37 additions & 27 deletions internal/jobqueue/jobqueue.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,11 +153,12 @@ type WebhookInstallWorker struct {

// WebhookRemovalJobArgs represents the arguments for a webhook removal job
type WebhookRemovalJobArgs struct {
ConnectorID int `json:"connector_id"`
ProjectPath string `json:"project_path"`
Provider string `json:"provider"`
BaseURL string `json:"base_url"`
PAT string `json:"pat"`
ConnectorID int `json:"connector_id"`
ProjectPath string `json:"project_path"`
Provider string `json:"provider"`
BaseURL string `json:"base_url"`
PAT string `json:"pat"`
SkipRegistryUpdate bool `json:"skip_registry_update"`
Copy link
Copy Markdown
Contributor

@LinceMathew LinceMathew May 4, 2026

Choose a reason for hiding this comment

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

what is this new type?

Copy link
Copy Markdown
Contributor Author

@Amazing-Stardom Amazing-Stardom May 4, 2026

Choose a reason for hiding this comment

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

The webhook delete queue function had a DB update dependency.

  1. Delete webhook from provider
  2. Update webhook_registry table data.

Proposed API for Deleting Git Connector Have this.

  1. Get webhook from the DB and then create job to delete it.

    LiveReview/internal/api/connectors.go

    Line 494 in c8725f3

    if err := s.jobQueue.QueueWebhookRemovalJob(ctx, id, projectPath, provider, providerURL, patToken, true); err != nil {
  2. Delete the integration token.

    LiveReview/internal/api/connectors.go

    Line 501 in c8725f3

    result, err := s.db.Exec(`

    As it webhook_registry is a child table, it will be also deleted.
  3. Return response to UI

So, as I don't want to update webhook_registrythe table, I will skip it.

let me know if the variable name needs update.

Copy link
Copy Markdown
Contributor

@LinceMathew LinceMathew May 4, 2026

Choose a reason for hiding this comment

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

The variable name SkipRegistryUpdate is clear and accurately reflects its purpose. It's a good choice given the context you've provided about the webhook_registry table being automatically deleted when the parent integration_tokens (connector) is removed.

Copy link
Copy Markdown
Contributor

@LinceMathew LinceMathew May 4, 2026

Choose a reason for hiding this comment

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

okay

}

// Kind returns the job kind for River
Expand Down Expand Up @@ -1413,11 +1414,13 @@ func (w *WebhookRemovalWorker) handleGitLabWebhookRemoval(ctx context.Context, a

log.Printf("Successfully removed webhooks for project %s", args.ProjectPath)

// Update the webhook registry to mark as unconnected
err = w.updateWebhookRegistryForRemoval(ctx, args, projectID)
if err != nil {
log.Printf("Failed to update webhook registry for project %s: %v", args.ProjectPath, err)
return fmt.Errorf("failed to update webhook registry: %w", err)
// Update the webhook registry to mark as unconnected, unless skipped
if !args.SkipRegistryUpdate {
err = w.updateWebhookRegistryForRemoval(ctx, args, projectID)
if err != nil {
log.Printf("Failed to update webhook registry for project %s: %v", args.ProjectPath, err)
return fmt.Errorf("failed to update webhook registry: %w", err)
}
}

return nil
Expand All @@ -1443,11 +1446,13 @@ func (w *WebhookRemovalWorker) handleGitHubWebhookRemoval(ctx context.Context, a

log.Printf("Successfully removed webhooks for GitHub repository %s/%s", owner, repo)

// Update the webhook registry to mark as unconnected
err = w.updateWebhookRegistryForGitHubRemoval(ctx, args)
if err != nil {
log.Printf("Failed to update webhook registry for GitHub repository %s/%s: %v", owner, repo, err)
return fmt.Errorf("failed to update webhook registry: %w", err)
// Update the webhook registry to mark as unconnected, unless skipped
if !args.SkipRegistryUpdate {
err = w.updateWebhookRegistryForGitHubRemoval(ctx, args)
if err != nil {
log.Printf("Failed to update webhook registry for GitHub repository %s/%s: %v", owner, repo, err)
return fmt.Errorf("failed to update webhook registry: %w", err)
}
}

return nil
Expand Down Expand Up @@ -1873,10 +1878,12 @@ func (w *WebhookRemovalWorker) handleBitbucketWebhookRemoval(ctx context.Context
// Don't return error here - we still want to update the registry
}

// Update the webhook_registry to mark as removed
err = w.updateWebhookRegistryForBitbucketRemoval(ctx, args)
if err != nil {
return fmt.Errorf("failed to update webhook registry: %w", err)
// Update the webhook_registry to mark as removed, unless skipped
if !args.SkipRegistryUpdate {
err = w.updateWebhookRegistryForBitbucketRemoval(ctx, args)
if err != nil {
return fmt.Errorf("failed to update webhook registry: %w", err)
}
}

log.Printf("Bitbucket webhook removal completed for repository: %s/%s", workspace, repo)
Expand Down Expand Up @@ -2095,8 +2102,10 @@ func (w *WebhookRemovalWorker) handleGiteaWebhookRemoval(ctx context.Context, ar
// continue to registry update even on API failure
}

if err := w.updateWebhookRegistryForGiteaRemoval(ctx, args); err != nil {
return fmt.Errorf("failed to update webhook registry: %w", err)
if !args.SkipRegistryUpdate {
if err := w.updateWebhookRegistryForGiteaRemoval(ctx, args); err != nil {
return fmt.Errorf("failed to update webhook registry: %w", err)
}
}

log.Printf("Gitea webhook removal completed for repository: %s/%s", owner, repo)
Expand Down Expand Up @@ -2329,13 +2338,14 @@ func (jq *JobQueue) QueueWebhookInstallJob(ctx context.Context, connectorID int,
}

// QueueWebhookRemovalJob queues a webhook removal job
func (jq *JobQueue) QueueWebhookRemovalJob(ctx context.Context, connectorID int, projectPath, provider, baseURL, pat string) error {
func (jq *JobQueue) QueueWebhookRemovalJob(ctx context.Context, connectorID int, projectPath, provider, baseURL, pat string, skipRegistryUpdate bool) error {
args := WebhookRemovalJobArgs{
ConnectorID: connectorID,
ProjectPath: projectPath,
Provider: provider,
BaseURL: baseURL,
PAT: pat,
ConnectorID: connectorID,
ProjectPath: projectPath,
Provider: provider,
BaseURL: baseURL,
PAT: pat,
SkipRegistryUpdate: skipRegistryUpdate,
}

_, err := jq.client.Insert(ctx, args, nil)
Expand Down
Loading