Skip to content

fix: accept empty lists for non-null list items - #3098

Merged
devsergiy merged 4 commits into
mainfrom
jens/eng-9882-string-cannot-represent-value
Jul 17, 2026
Merged

fix: accept empty lists for non-null list items#3098
devsergiy merged 4 commits into
mainfrom
jens/eng-9882-string-cannot-represent-value

Conversation

@devsergiy

@devsergiy devsergiy commented Jul 17, 2026

Copy link
Copy Markdown
Member

fixes #3095

Summary by CodeRabbit

Summary by CodeRabbit

  • Chores

    • Updated the GraphQL tooling dependency to version 2.13.2 for both the router and router tests.
  • Tests

    • Added input validation coverage to confirm list variable default behavior when the variable is omitted and the default is an empty list, and verify correct results when a non-empty value is provided.

Checklist

Open Source AI Manifesto

This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.

@devsergiy
devsergiy requested a review from a team as a code owner July 17, 2026 20:00
@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2792de8d-b89c-4dcb-8ba6-d530cdac4881

📥 Commits

Reviewing files that changed from the base of the PR and between 826b267 and fdcef62.

📒 Files selected for processing (1)
  • router-tests/operations/variables_validation_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • router-tests/operations/variables_validation_test.go

Walkthrough

The pull request updates graphql-go-tools to v2.13.2 in both router modules and adds regression tests for omitted and explicitly provided GraphQL list variables with an empty-list default.

Changes

GraphQL tools validation fix

Layer / File(s) Summary
Align GraphQL tools module versions
router/go.mod, router-tests/go.mod
Both Go modules update github.com/wundergraph/graphql-go-tools/v2 to v2.13.2.
Cover empty list variable defaults and provided values
router-tests/operations/variables_validation_test.go
Adds subtests verifying that an omitted $names variable defaults to [] and that explicitly provided values are returned unchanged.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: accepting empty list defaults for non-null list items.
Linked Issues check ✅ Passed The dependency bump and new tests align with issue #3095 by covering valid queries using $names: [String!] = [] with and without provided values.
Out of Scope Changes check ✅ Passed The changes stay focused on the reported validation regression, with only the dependency update and targeted test additions.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"


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

@github-actions

github-actions Bot commented Jul 17, 2026

Copy link
Copy Markdown

❌ Internal Query Planner CI checks failed

The Internal Query Planner CI checks failed in the celestial repository, and this is going to stop the merge of this PR.
If you are part of the WunderGraph organization, you can see the PR with more details.

@codecov

codecov Bot commented Jul 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.83%. Comparing base (ef01bcf) to head (fdcef62).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3098      +/-   ##
==========================================
- Coverage   71.13%   61.83%   -9.31%     
==========================================
  Files         335      262      -73     
  Lines       49342    30810   -18532     
  Branches     6042        0    -6042     
==========================================
- Hits        35101    19050   -16051     
+ Misses      14215    10232    -3983     
- Partials       26     1528    +1502     

see 597 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@router-tests/operations/variables_validation_test.go`:
- Around line 19-24: Update the “empty list default for non-null list items”
subtest to omit the Variables field so the query uses its declared [] default,
and change the expected response to rootFieldWithListArg returning an empty
list. If retaining coverage for ["a"], move it into a separate subtest.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 60caf48a-ceb5-4f35-827d-d277df6e0c30

📥 Commits

Reviewing files that changed from the base of the PR and between f3ede64 and 0381507.

📒 Files selected for processing (1)
  • router-tests/operations/variables_validation_test.go

Comment thread router-tests/operations/variables_validation_test.go Outdated
@devsergiy
devsergiy merged commit 85f3294 into main Jul 17, 2026
45 of 46 checks passed
@devsergiy
devsergiy deleted the jens/eng-9882-string-cannot-represent-value branch July 17, 2026 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

String cannot represent value: []

3 participants