Skip to content

fix: invalid data resolution for null parent types#1428

Merged
Noroth merged 2 commits intomasterfrom
ludwig/eng-9091-check-invalid-data-for-message-types-in-proto-compiler
Mar 5, 2026
Merged

fix: invalid data resolution for null parent types#1428
Noroth merged 2 commits intomasterfrom
ludwig/eng-9091-check-invalid-data-for-message-types-in-proto-compiler

Conversation

@Noroth
Copy link
Copy Markdown
Contributor

@Noroth Noroth commented Mar 5, 2026

When resolving scalar value data from non list types in the context we can run into a panic. This PR adds additional checks to ensure we don't resolve data from an invalid context.

Summary by CodeRabbit

  • New Features

    • Added a top-level category query and featuredCategory resolution for subcategories to fetch single categories and featured relations.
  • Bug Fixes

    • Improved robustness by short-circuiting on invalid/null messages and adding explicit error for missing object fields.
  • Tests

    • Added tests and tracing to verify category query behavior and that field resolvers are not invoked for null category results.

Checklist

  • I have discussed my proposed changes in an issue and have received approval to proceed.
  • I have followed the coding standards of the project.
  • Tests or benchmarks have been added or updated.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 5, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 9160824f-8951-4a5f-8ef8-6aee370dc9da

📥 Commits

Reviewing files that changed from the base of the PR and between f8eb4d3 and 87268e1.

📒 Files selected for processing (1)
  • v2/pkg/engine/datasource/grpc_datasource/mapping_test_helper.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • v2/pkg/engine/datasource/grpc_datasource/mapping_test_helper.go

📝 Walkthrough

Walkthrough

Adds category support to the gRPC datasource: new QueryCategory and ResolveSubcategoryFeaturedCategory RPCs and proto messages, GraphQL schema entry, mapping and mock service updates, RPC-call tracing in tests, and defensive checks to avoid nil dereferences during path resolution and JSON flattening.

Changes

Cohort / File(s) Summary
Proto & GraphQL schema
v2/pkg/grpctest/product.proto, v2/pkg/grpctest/testdata/products.graphqls
Added QueryCategory and ResolveSubcategoryFeaturedCategory RPCs and supporting request/response/context/args/result messages; added category(id: ID!): Category to GraphQL Query.
GRPC mapping & test helper
v2/pkg/engine/datasource/grpc_datasource/mapping_test_helper.go, v2/pkg/grpctest/mapping/mapping.go
Wired new category QueryRPC and featuredCategory resolve mapping for Subcategory, including argument name mappings (idid, includeChildreninclude_children).
Mock service
v2/pkg/grpctest/mockservice_enums.go
Added QueryCategory implementation returning a Category with requested id, derived name, and kind.
Datasource spy tests
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_spy_test.go
Added RPC-tracing counters and hook, new QueryCategory spy method and field-resolver spy methods, and test asserting field resolvers are not invoked when Category is null.
Path resolution & JSON builder
v2/pkg/engine/datasource/grpc_datasource/compiler.go, v2/pkg/engine/datasource/grpc_datasource/json_builder.go
Added early validity checks in resolveDataForPath/resolveListDataForPath to short-circuit on invalid messages; flattenObject now errors if target field is missing to avoid nil dereference.
Module manifest
go.mod
Indirect change listed in manifest (no public API changes).

Sequence Diagram(s)

mermaid
sequenceDiagram
participant Client
participant GraphQL_Server as GraphQL Server
participant GRPC_DS as gRPC Datasource
participant ProductSvc as ProductService
participant MockSvc as MockService (test)
Client->>GraphQL_Server: GraphQL query (category/id)
GraphQL_Server->>GRPC_DS: Resolve Query -> QueryCategory RPC
GRPC_DS->>ProductSvc: QueryCategory(Request{id})
ProductSvc->>MockSvc: (test) delegate to MockService.QueryCategory
MockSvc-->>ProductSvc: QueryCategoryResponse(Category or nil)
ProductSvc-->>GRPC_DS: Response with Category
GRPC_DS-->>GraphQL_Server: Mapped Category data
alt Category != null and has sub-resolvers
GraphQL_Server->>GRPC_DS: Resolve field (e.g., popularityScore)
GRPC_DS->>ProductSvc: ResolveCategoryPopularityScore RPC
ProductSvc-->>GRPC_DS: Field response
GRPC_DS-->>GraphQL_Server: Field value
else Category == null
Note over GraphQL_Server,GRPC_DS: Field resolvers not invoked
end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the primary change: adding validity checks for null parent types in data resolution, which aligns with the core fixes in compiler.go, json_builder.go, and test coverage for null category handling.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch ludwig/eng-9091-check-invalid-data-for-message-types-in-proto-compiler

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@v2/pkg/engine/datasource/grpc_datasource/mapping_test_helper.go`:
- Around line 522-531: The gci formatting error is caused by the map literal
block containing entries like "categories", "category" and "categoriesByKind" in
mapping_test_helper.go; run the project's formatting tools (gci and
gofmt/goimports) on that file to reorder/imports and fix spacing so the
FieldArgumentMap and TargetName entries are properly formatted, then re-run CI
to ensure the gci lint passes.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 586c0d72-020b-4107-b6b0-a3f967f03c67

📥 Commits

Reviewing files that changed from the base of the PR and between 5940d31 and f8eb4d3.

⛔ Files ignored due to path filters (2)
  • v2/pkg/grpctest/productv1/product.pb.go is excluded by !**/*.pb.go
  • v2/pkg/grpctest/productv1/product_grpc.pb.go is excluded by !**/*.pb.go
📒 Files selected for processing (8)
  • v2/pkg/engine/datasource/grpc_datasource/compiler.go
  • v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_spy_test.go
  • v2/pkg/engine/datasource/grpc_datasource/json_builder.go
  • v2/pkg/engine/datasource/grpc_datasource/mapping_test_helper.go
  • v2/pkg/grpctest/mapping/mapping.go
  • v2/pkg/grpctest/mockservice_enums.go
  • v2/pkg/grpctest/product.proto
  • v2/pkg/grpctest/testdata/products.graphqls

Comment thread v2/pkg/engine/datasource/grpc_datasource/mapping_test_helper.go Outdated
@Noroth Noroth merged commit b7863be into master Mar 5, 2026
11 checks passed
@Noroth Noroth deleted the ludwig/eng-9091-check-invalid-data-for-message-types-in-proto-compiler branch March 5, 2026 13:43
Noroth pushed a commit that referenced this pull request Mar 5, 2026
🤖 I have created a release *beep* *boop*
---


##
[2.0.0-rc.260](v2.0.0-rc.259...v2.0.0-rc.260)
(2026-03-05)


### Bug Fixes

* invalid data resolution for null parent types
([#1428](#1428))
([b7863be](b7863be))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Bug Fixes**
  * Fixed invalid data resolution when handling null parent types.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
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.

2 participants