Skip to content

refactor: replace deprecated grpc.DialContext with NewClient#2

Open
AR21SM wants to merge 1 commit intokrkn-chaos:mainfrom
AR21SM:refactor/grpc-newclient-migration
Open

refactor: replace deprecated grpc.DialContext with NewClient#2
AR21SM wants to merge 1 commit intokrkn-chaos:mainfrom
AR21SM:refactor/grpc-newclient-migration

Conversation

@AR21SM
Copy link

@AR21SM AR21SM commented Feb 5, 2026

Signed-off-by: AR21SM <mahajanashishar21sm@gmail.com>
@qodo-code-review
Copy link

Review Summary by Qodo

Replace deprecated grpc.DialContext with NewClient

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Replace deprecated grpc.DialContext with grpc.NewClient
• Remove unnecessary grpc.WithBlock() option
• Modernize gRPC client initialization approach
Diagram
flowchart LR
  A["grpc.DialContext<br/>with WithBlock"] -- "migrate to" --> B["grpc.NewClient<br/>without WithBlock"]
  B -- "result" --> C["Modern gRPC client"]
Loading

Grey Divider

File Changes

1. internal/api/handlers.go ✨ Enhancement +1/-3

Migrate gRPC client to NewClient API

• Replaced grpc.DialContext() call with grpc.NewClient() in callGetNodesGRPC() method
• Removed deprecated grpc.WithBlock() option from gRPC connection configuration
• Maintained context timeout and transport credentials configuration
• Simplified gRPC client initialization to use current recommended API

internal/api/handlers.go


Grey Divider

Qodo Logo

@qodo-code-review
Copy link

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (1) 📎 Requirement gaps (0)

Grey Divider


Action required

1. Raw gRPC error exposed 📘 Rule violation ⛨ Security
Description
callGetNodesGRPC returns raw gRPC library errors from grpc.NewClient, which may include
  internal connection details.
• The HTTP handler builds a user-facing response that concatenates err.Error(), so internal error
  details can be exposed to clients.
• User-facing messages should be generic, with detailed errors logged only to internal logs.
Code

internal/api/handlers.go[R313-318]

+	conn, err := grpc.NewClient(
		h.grpcServerAddr,
		grpc.WithTransportCredentials(insecure.NewCredentials()),
-		grpc.WithBlock(),
	)
	if err != nil {
		return nil, err
Evidence
PR Compliance ID 4 requires avoiding internal detail leakage in user-facing errors. The changed code
now produces gRPC client-creation errors that are returned raw, and the handler composes a response
including err.Error(), exposing internal details to the end-user.

Rule 4: Generic: Secure Error Handling
internal/api/handlers.go[313-318]
internal/api/handlers.go[161-166]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A raw gRPC error string can be returned to end-users via the HTTP response, potentially leaking internal implementation/connection details.

## Issue Context
`callGetNodesGRPC` propagates underlying gRPC errors. The `GetNodes` handler currently concatenates `err.Error()` into a user-facing message.

## Fix Focus Areas
- internal/api/handlers.go[161-167]
- internal/api/handlers.go[313-318]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Dial timeout no longer applied 🐞 Bug ⛯ Reliability
Description
callGetNodesGRPC still documents “Create gRPC connection with timeout” and creates a 10s timeout
  context, but the updated client creation (grpc.NewClient) does not use that context, so the
  timeout no longer bounds connection establishment at creation time.
• This changes failure mode/latency characteristics: connection problems may now only surface when
  the first RPC is invoked (rather than during client creation), which can make error handling and
  debugging less predictable for the HTTP handler.
Code

internal/api/handlers.go[R310-316]

	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
	defer cancel()

-	conn, err := grpc.DialContext(
-		ctx,
+	conn, err := grpc.NewClient(
		h.grpcServerAddr,
		grpc.WithTransportCredentials(insecure.NewCredentials()),
-		grpc.WithBlock(),
	)
Evidence
The helper creates a timeout context and claims it is for the connection, but the new code path
creates the gRPC client connection without using that context; the only use of ctx is for the
subsequent RPC. The HTTP handler directly depends on this helper for request responsiveness and
error reporting.

internal/api/handlers.go[307-316]
internal/api/handlers.go[325-332]
internal/api/handlers.go[160-167]
go.mod[5-13]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`callGetNodesGRPC` creates a 10s timeout context and comments that it is for the **gRPC connection**, but the code now uses `grpc.NewClient(...)` which does not consume that context. As a result, connection establishment is not bounded at client-creation time, and connectivity failures may only surface when the RPC is invoked.

### Issue Context
The HTTP handler (`GetNodes`) relies on `callGetNodesGRPC` to complete promptly and return actionable errors. If you want to preserve the prior “fail fast during dial/connect” behavior, you need an explicit connect/wait-for-ready step when using `grpc.NewClient`.

### Fix Focus Areas
- internal/api/handlers.go[307-336]
- internal/api/handlers.go[160-168]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

@AR21SM
Copy link
Author

AR21SM commented Feb 9, 2026

@tsebastiani

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.

Replace deprecated grpc.DialContext with `grpc.NewClient

1 participant