refactor(provider/client): use protobuf BidScreeningResponse for Vali…#244
refactor(provider/client): use protobuf BidScreeningResponse for Vali…#244
Conversation
…date Signed-off-by: Artur Troian <troian@users.noreply.github.com>
WalkthroughThe changes replace a custom Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
go/provider/client/client.go (1)
411-413: Useprotojsoninstead ofencoding/jsonfor protobuf message decoding.At line 412,
BidScreeningResponse(a protobuf message) is decoded usingencoding/json. The standard JSON encoder does not honor protobuf JSON field mappings—specifically, theResourceOfferfield has a protobufjson_nameofresourceOfferbut the Go struct tag expectsresource_offer. If the validate endpoint returns protobuf-formatted JSON, this mismatch will cause the field to be silently dropped. Useprotojson.Unmarshalinstead.🔧 Suggested change
import ( "bytes" "context" "crypto" "crypto/tls" "crypto/x509" "encoding/json" "errors" "fmt" "io" "math/big" "net/http" "net/url" "strconv" "strings" "time" "github.com/golang-jwt/jwt/v5" "github.com/gorilla/websocket" "k8s.io/client-go/tools/remotecommand" + "google.golang.org/protobuf/encoding/protojson" ... ) ... var obj providerv1.BidScreeningResponse -if err = json.NewDecoder(buf).Decode(&obj); err != nil { +if err = protojson.UnmarshalOptions{DiscardUnknown: true}.Unmarshal(buf.Bytes(), &obj); err != nil { return nil, err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/provider/client/client.go` around lines 411 - 413, The code decodes a protobuf message using encoding/json which doesn’t respect protobuf JSON mappings; replace the json.NewDecoder(buf).Decode(&obj) call for providerv1.BidScreeningResponse with protojson.Unmarshal on the buffer bytes (e.g., protojson.Unmarshal(buf.Bytes(), &obj)) and add the google.golang.org/protobuf/encoding/protojson import so the ResourceOffer and other protobuf-json mapped fields are decoded correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@go/provider/client/client.go`:
- Around line 411-413: The code decodes a protobuf message using encoding/json
which doesn’t respect protobuf JSON mappings; replace the
json.NewDecoder(buf).Decode(&obj) call for providerv1.BidScreeningResponse with
protojson.Unmarshal on the buffer bytes (e.g., protojson.Unmarshal(buf.Bytes(),
&obj)) and add the google.golang.org/protobuf/encoding/protojson import so the
ResourceOffer and other protobuf-json mapped fields are decoded correctly.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
go/provider/client/client.gogo/provider/client/types.go
💤 Files with no reviewable changes (1)
- go/provider/client/types.go
…date
📝 Description
[Explain what this PR does in 2-3 sentences. Include context about the feature or problem being solved]
🔧 Purpose of the Change
📌 Related Issues
✅ Checklist
📎 Notes for Reviewers
[Include any additional context, architectural decisions, or specific areas to focus on]