Conversation
Keep the values of non-block strings are they are. For BlockString that end abruptly on EOF, set the Token End position. That prevents panics on cases like `union """` in the lexer.
ysmolski
requested review from
a team,
Noroth,
StarpTech and
devsergiy
as code owners
WalkthroughThe changes standardize and correct description strings for the Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
v2/pkg/lexer/lexer.go (1)
397-398: Consider removing unused variables.The variables
whitespaceCountandreachedFirstNonWhitespaceare tracked but don't appear to be used for adjusting token literal boundaries, which aligns with the PR objective of preserving whitespace in non-block strings. However, since they're not used, consider removing them for clarity.Apply this diff to remove unused variables:
escaped := false - whitespaceCount := 0 - reachedFirstNonWhitespace := falseAnd remove the related tracking logic in the switch cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
pkg/astprinter/testdata/starwars.schema.graphql(1 hunks)v2/pkg/astparser/parser_test.go(4 hunks)v2/pkg/astparser/testdata/starwars.schema.graphql(1 hunks)v2/pkg/astparser/testdata/todo.graphql(1 hunks)v2/pkg/astprinter/testdata/starwars.schema.graphql(1 hunks)v2/pkg/asttransform/baseschema.go(1 hunks)v2/pkg/astvalidation/operation_validation_test.go(1 hunks)v2/pkg/introspection/testdata/starwars.schema.graphql(1 hunks)v2/pkg/lexer/lexer.go(1 hunks)v2/pkg/lexer/lexer_test.go(5 hunks)v2/pkg/middleware/operation_complexity/operation_complexity_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
v2/pkg/astparser/parser_test.go (1)
v2/pkg/ast/ast_input_value_definition.go (1)
DefaultValue(16-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Linters
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Linters
🔇 Additional comments (15)
v2/pkg/astparser/testdata/todo.graphql (1)
59-59: Good whitespace normalization.Removing the leading space from the directive description improves consistency across schema files.
v2/pkg/astparser/testdata/starwars.schema.graphql (1)
175-175: Consistent whitespace correction.This change aligns with the whitespace normalization applied across multiple schema files.
v2/pkg/asttransform/baseschema.go (1)
151-151: Consistent formatting in base schema.This change ensures the base schema definition aligns with the formatting corrections made to the test schema files.
v2/pkg/lexer/lexer.go (2)
355-357: Good EOF handling for block strings.The explicit setting of token end position on EOF prevents panics when encountering unterminated block strings, which addresses the issue mentioned in the PR objectives.
403-426: Whitespace preservation behavior verifiedThe v2 test suite already includes a case confirming that leading/trailing spaces and tabs are kept in single-line strings:
- v2/pkg/lexer/lexer_test.go –
t.Run("read string with leading/trailing whitespace", …
run("" \tfoo\t "", mustRead(keyword.STRING, " \tfoo\t "))No further changes or additional tests are needed; the new logic is covered.
v2/pkg/introspection/testdata/starwars.schema.graphql (1)
181-182: Whitespace fix looks goodThe leading space was removed and the wording now matches the GraphQL spec examples. No further action required.
v2/pkg/middleware/operation_complexity/operation_complexity_test.go (1)
623-627: Schema string now consistent with specThe directive-argument description is now aligned with the other schema fixtures (
"Included when true."). 👍v2/pkg/astvalidation/operation_validation_test.go (1)
5078-5078: Doc-string trim looks correct and keeps tests in sync.The leading space served no functional purpose and was inconsistent with other schema files. 👍
v2/pkg/astparser/parser_test.go (4)
2371-2371: LGTM: Correctly tests whitespace preservation in non-block strings.The test now correctly expects the trailing space after the backslash to be preserved, which aligns with the PR objective of not trimming whitespace around non-block strings.
2438-2438: LGTM: Consistent whitespace preservation testing.This change maintains consistency with the whitespace preservation behavior across different parsing contexts.
2505-2505: LGTM: Proper testing of whitespace preservation in default values.The test correctly verifies that trailing whitespace is preserved in default input values for non-block strings, and the use of
assert.Equalprovides better error reporting.
2529-2529: LGTM: Comprehensive edge case testing.This test case properly verifies whitespace preservation with double backslashes, ensuring the feature works correctly in edge cases.
v2/pkg/lexer/lexer_test.go (3)
17-17: LGTM: Good practice for test performance.Adding
t.Parallel()is a good practice for independent lexer tests and will improve test suite performance.
37-37: LGTM: Significant improvement in test error reporting.Replacing
panic(fmt.Errorf(...))witht.Errorf(...)is a major improvement that:
- Provides better error messages and debugging experience
- Allows tests to continue and report multiple failures
- Follows Go testing best practices
- Integrates properly with the testing framework
This change enhances the overall quality and maintainability of the test suite.
Also applies to: 41-41, 57-57, 61-61, 63-63, 66-66, 75-75
177-221: LGTM: Comprehensive improvements to string parsing tests.These changes significantly enhance the test suite:
- Better readability: Raw string literals (backticks) eliminate escaping issues and improve readability
- Enhanced coverage: New test cases for block strings, including edge cases like incomplete blocks and whitespace handling
- Improved naming: Test names better describe what's being tested
- Alignment with PR goals: Tests for incomplete block strings and whitespace preservation support the PR's objectives
The additions provide robust coverage for string parsing behavior, especially around the changes made in this PR.
ysmolski
changed the title
…l-input-value-is-trimming-whitespace' into yury/eng-5916-string-literal-input-value-is-trimming-whitespace
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
v2/pkg/astnormalization/astnormalization.go (1)
418-435: Consider renaming the variable for clarity.The variable
spreadNameon line 433 actually contains the fragment definition name, not a spread name. This could be confusing for future maintainers.Apply this diff to improve clarity:
- spreadName := operation.FragmentDefinitionNameBytes(cycleFragIdx) - f.Walker.Report.AddExternalError(operationreport.ErrFragmentSpreadFormsCycle(spreadName)) + fragmentName := operation.FragmentDefinitionNameBytes(cycleFragIdx) + f.Walker.Report.AddExternalError(operationreport.ErrFragmentSpreadFormsCycle(fragmentName))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v2/pkg/astnormalization/astnormalization.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: SkArchon
PR: wundergraph/graphql-go-tools#1203
File: v2/pkg/engine/resolve/loader.go:63-67
Timestamp: 2025-07-02T15:28:02.073Z
Learning: In the graphql-go-tools codebase, result structs are consistently initialized with non-nil bytes.Buffer instances, making additional nil checks for res.out unnecessary defensive programming.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Linters
- GitHub Check: Linters
🔇 Additional comments (2)
v2/pkg/astnormalization/astnormalization.go (2)
211-211: Good placement for fragment cycle detection.Integrating fragment cycle detection early in the normalization pipeline ensures that cycles are caught before other transformations that might depend on valid fragment structures.
397-491: Well-implemented fragment cycle detection.The visitor correctly implements cycle detection using depth-first search with a recursion stack. The algorithm properly handles edge cases like undefined fragments and accurately identifies the cycle path for error reporting.
devsergiy
deleted the
yury/eng-5916-string-literal-input-value-is-trimming-whitespace
branch
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.199](v2.0.0-rc.198...v2.0.0-rc.199) (2025-07-07) ### Features * add support for aliases ([#1209](#1209)) ([9223351](9223351)) ### Bug Fixes * do not trim whitespaces around non-block strings ([#1211](#1211)) ([6f5046b](6f5046b)) --- 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 * **New Features** * Added support for aliases. * **Bug Fixes** * Resolved an issue to prevent trimming whitespaces around non-block strings. * **Documentation** * Updated the changelog with details for version 2.0.0-rc.199. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
3 participants
Keep the whitespace padded values of non-block strings are they are.
For a BlockString that ends abruptly on EOF, set the Token End position.
That prevents panics on cases like
`union """`in the lexer.fixes ENG-5916
fixes ENG-7491
fixes #937
fixes #1019
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
@includedirective'sifargument across multiple files.New Features
Tests
Chores
Checklist