fix: preserve nullable nested input presence in gRPC compiler

[fengyuwusong]

Summary

• treat omitted and null nested input messages as absent during protobuf compilation
• preserve explicit empty objects instead of conflating them with null
• add a regression test covering omitted, null, empty-object and explicit-value cases

Testing

• go test ./v2/pkg/engine/datasource/grpc_datasource

Refs wundergraph/cosmo#2650

Summary by CodeRabbit

• Bug Fixes

  • More accurate detection of present vs absent nested inputs, stricter enforcement of required fields, and correct skipping of empty optional scalar values during gRPC datasource compilation.

• Tests

  • Added tests exercising omitted, null, empty-object, and explicit nested input scenarios to validate compiler behavior.

[coderabbitai]

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: f70c3a83-30f0-4e2c-8f74-ae29c0e365c9

📥 Commits

Reviewing files that changed from the base of the PR and between 8a818e7 and 34075dc.

📒 Files selected for processing (1)

• v2/pkg/engine/datasource/grpc_datasource/compiler_input_presence_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• v2/pkg/engine/datasource/grpc_datasource/compiler_input_presence_test.go

---

📝 Walkthrough

Walkthrough

Centralizes retrieval of field data (fieldData := data.Get(rpcField.JSONPath)), uses a new isNonNullValue(gjson.Result) bool guard instead of repeated .Exists() checks, adjusts handling for optional vs required fields (returning errors for missing required fields, skipping absent optional scalar-wrapped fields), propagates fieldData through recursive calls, and adds tests for optional nested protobuf inputs.

Changes

Cohort / File(s)	Summary
gRPC datasource compiler v2/pkg/engine/datasource/grpc_datasource/compiler.go	Introduce helper isNonNullValue(data gjson.Result) bool; obtain and propagate a single fieldData := data.Get(rpcField.JSONPath) as the source of truth; replace .Exists() checks with isNonNullValue(...); enforce required-field presence with early errors; skip processing for absent optional scalar-wrapped fields; minor control-flow adjustments for recursive calls.
Optional nested input tests v2/pkg/engine/datasource/grpc_datasource/compiler_input_presence_test.go	Add tests covering optional nested protobuf inputs: omitted field, explicit null, empty object, and populated nested values; asserts presence/absence and contents of conditions (nested message) and params (string-encoded nested object).

Sequence Diagram(s)

sequenceDiagram
    participant Client as Client (JSON input)
    participant Compiler as ProtoCompiler
    participant Resolver as FieldResolver (uses gjson)
    participant Builder as RPCMessage Builder

    Client->>Compiler: Provide JSON input for RPC payload
    Compiler->>Resolver: data.Get(rpcField.JSONPath) -> fieldData
    Resolver-->>Compiler: fieldData (gjson.Result)
    alt isNonNullValue(fieldData)
        Compiler->>Builder: recurse/process field using fieldData
        Builder-->>Compiler: built field value
    else not non-null
        alt field is required
            Compiler-->>Client: return error (missing required field)
        else optional
            Compiler->>Builder: skip or leave unset
        end
    end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• fix: invalid data resolution for null parent types #1428 — Adds early-return guards in resolveListDataForPath/resolveDataForPath to avoid resolving from null/invalid parent types in the same compiler area.
• fix: skip invocation for empty context in field resolvers #1413 — Similar changes around preventing building/invoking RPC payloads when input/context data is absent or null in the gRPC datasource compiler.

🚥 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 describes the main change: fixing how nullable nested input presence is handled in the gRPC compiler, which aligns with the PR objectives of treating omitted/null as absent and preserving explicit empty objects.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

📝 Coding Plan

• Generate coding plan for human review comments

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

Tip

CodeRabbit can generate a title for your PR based on the changes.

Add @coderabbitai placeholder anywhere in the title of your PR and CodeRabbit will replace it with a title based on the changes in the PR. You can change the placeholder by changing the reviews.auto_title_placeholder setting.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

v2/pkg/engine/datasource/grpc_datasource/compiler_input_presence_test.go (1)

33-107: Add one required-field negative case to lock the new error path.

This suite is strong for optional semantics. Since compiler.go now errors on missing required nested inputs, add a case that marks conditions as non-optional and asserts errors for {} and {"conditions":null}.

🧪 Suggested test addition

+func TestCompileRequiredNestedInputRejectsNullOrOmitted(t *testing.T) {
+	compiler, err := NewProtoCompiler(protoSchemaWithOptionalNestedInputs, nil)
+	require.NoError(t, err)
+
+	inputMessage, ok := compiler.doc.MessageByName("UpdateRuleRequest")
+	require.True(t, ok)
+
+	requiredConditions := &RPCMessage{
+		Name: "UpdateRuleRequest",
+		Fields: RPCFields{
+			{
+				Name:          "conditions",
+				ProtoTypeName: DataTypeMessage,
+				JSONPath:      "conditions",
+				Optional:      false,
+				Message: &RPCMessage{
+					Name: "ConditionsInput",
+					Fields: RPCFields{
+						{
+							Name:          "key",
+							ProtoTypeName: DataTypeString,
+							JSONPath:      "key",
+							Optional:      true,
+						},
+					},
+				},
+			},
+		},
+	}
+
+	for _, input := range []string{`{}`, `{"conditions":null}`} {
+		_, err := compiler.buildProtoMessage(inputMessage, requiredConditions, gjson.Parse(input))
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "field conditions is required")
+	}
+}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@v2/pkg/engine/datasource/grpc_datasource/compiler_input_presence_test.go`
around lines 33 - 107, Add a negative test case to
TestCompileOptionalNestedInputsTreatsNullAsAbsent that exercises the new
"required nested input" error path by using a schema where the conditions field
is non-optional and asserting that building the message errors for both `{}` and
`{"conditions":null}`; update the test to instantiate NewProtoCompiler with a
modified schema (copy/variant of protoSchemaWithOptionalNestedInputs where
conditions is required), call compiler.buildProtoMessage with
optionalNestedInputsMessage() and the two inputs, and require an error
(require.Error) instead of a successful build, referencing the existing test
harness (TestCompileOptionalNestedInputsTreatsNullAsAbsent, NewProtoCompiler,
protoSchemaWithOptionalNestedInputs, optionalNestedInputsMessage,
buildProtoMessage, UpdateRuleRequest) so the new failing path is locked in.

🤖 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/compiler.go`:
- Around line 1060-1062: The comment above the continue in the block that checks
hasConcreteValue(fieldData) is misleading: replace “provide a null message” with
wording that correctly states the field will be left absent/omitted (no protobuf
presence) when we don't have a value; update the comment near the
hasConcreteValue(fieldData) check in compiler.go to say it skips the field
resulting in an absent field rather than a null message.

---

Nitpick comments:
In `@v2/pkg/engine/datasource/grpc_datasource/compiler_input_presence_test.go`:
- Around line 33-107: Add a negative test case to
TestCompileOptionalNestedInputsTreatsNullAsAbsent that exercises the new
"required nested input" error path by using a schema where the conditions field
is non-optional and asserting that building the message errors for both `{}` and
`{"conditions":null}`; update the test to instantiate NewProtoCompiler with a
modified schema (copy/variant of protoSchemaWithOptionalNestedInputs where
conditions is required), call compiler.buildProtoMessage with
optionalNestedInputsMessage() and the two inputs, and require an error
(require.Error) instead of a successful build, referencing the existing test
harness (TestCompileOptionalNestedInputsTreatsNullAsAbsent, NewProtoCompiler,
protoSchemaWithOptionalNestedInputs, optionalNestedInputsMessage,
buildProtoMessage, UpdateRuleRequest) so the new failing path is locked in.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 06092c95-9523-41c2-8ed8-f81b5678d444

📥 Commits

Reviewing files that changed from the base of the PR and between 310c26a and c045937.

📒 Files selected for processing (2)

• v2/pkg/engine/datasource/grpc_datasource/compiler.go
• v2/pkg/engine/datasource/grpc_datasource/compiler_input_presence_test.go

[Noroth]

Just some nits

[fengyuwusong]

@Noroth I've pushed a commit addressing your review comments — renamed hasConcreteValue to isNonNullValue, fixed the misleading comment, and added t.Parallel(). Could you take another look? Thanks!

[fengyuwusong]

@Noroth Could you please re-trigger the CI? I've fixed the tparallel lint issue by adding t.Parallel() to the subtests. Thanks!