Fix: Bad characters when using the !include function #1249
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1249 +/- ##
==========================================
+ Coverage 33.56% 33.70% +0.14%
==========================================
Files 226 226
Lines 24219 24269 +50
==========================================
+ Hits 8130 8181 +51
+ Misses 14875 14871 -4
- Partials 1214 1217 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughThe changes introduce logic to ensure YAML string values starting with Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant YAMLParser
participant NodeProcessor
User->>YAMLParser: Provide YAML input (may contain # at start)
YAMLParser->>NodeProcessor: Unmarshal into yaml.Node
NodeProcessor->>NodeProcessor: Recursively process nodes
NodeProcessor->>YAMLParser: Set single-quoted style for # strings
YAMLParser->>User: Return parsed Go value
sequenceDiagram
participant User
participant IncludeHandler
participant FileReader
User->>IncludeHandler: Use !include with YAML containing # string
IncludeHandler->>FileReader: Read included file
FileReader->>IncludeHandler: Return content
IncludeHandler->>IncludeHandler: Detect string starting with #
IncludeHandler->>IncludeHandler: Set single-quoted style in node
IncludeHandler->>User: Return processed value
Assessment against linked issues
Suggested labels
✨ Finishing Touches
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. 🪧 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
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
pkg/utils/yq_utils.go (3)
54-58: Minor: misleading var name
yamlDatareads well, but it shadows the importedyamlpackage symbol only indirectly. No functional bug here, yet a different identifier likesrcYAMLkeeps autocomplete lists cleaner.
80-90: Avoid the marshal → unmarshal round-tripAfter
processYAMLNodeyou can decode the node directly instead of serialising back to bytes only to parse again:- resultBytes, err := yaml.Marshal(&node) - if err != nil { - return nil, fmt.Errorf("EvaluateYqExpression: failed to marshal processed node: %w", err) - } - - res, err := UnmarshalYAML[any](string(resultBytes)) + var res any + if err := node.Decode(&res); err != nil { + return nil, fmt.Errorf("EvaluateYqExpression: failed to decode processed node: %w", err) + }This trims CPU, avoids another allocation, and keeps error handling simpler.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 83-84: pkg/utils/yq_utils.go#L83-L84
Added lines #L83 - L84 were not covered by tests
[warning] 89-90: pkg/utils/yq_utils.go#L89-L90
Added lines #L89 - L90 were not covered by tests
104-116: Duplicate helper across packages
processYAMLNodenow lives in bothpkg/utilsandpkg/filetype. Extracting it into a shared util (e.g.,pkg/yamlutil) eliminates duplication and keeps future tweaks—such as handling>-or|literals—in one place.pkg/filetype/filetype_test.go (1)
70-76: Add a multi-line hash test for full coverageStatic analysis flagged uncovered lines in the production code that handle multi-line
#strings. A quick extra case here would exercise that branch:{ name: "multi-line string with hash", input: "key: |\n #line1\n #line2\n", expected: map[string]any{"key": "#line1\n#line2\n"}, },This keeps the fast test suite while boosting confidence.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pkg/filetype/filetype.go(1 hunks)pkg/filetype/filetype_test.go(1 hunks)pkg/utils/yaml_include_test.go(1 hunks)pkg/utils/yaml_utils.go(2 hunks)pkg/utils/yq_utils.go(3 hunks)pkg/utils/yq_utils_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/utils/yq_utils.go (1)
pkg/utils/yaml_utils.go (2)
ConvertToYAML(85-91)UnmarshalYAML(221-223)
🪛 GitHub Check: codecov/patch
pkg/utils/yaml_utils.go
[warning] 191-192: pkg/utils/yaml_utils.go#L191-L192
Added lines #L191 - L192 were not covered by tests
[warning] 197-198: pkg/utils/yaml_utils.go#L197-L198
Added lines #L197 - L198 were not covered by tests
pkg/utils/yq_utils.go
[warning] 83-84: pkg/utils/yq_utils.go#L83-L84
Added lines #L83 - L84 were not covered by tests
[warning] 89-90: pkg/utils/yq_utils.go#L89-L90
Added lines #L89 - L90 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (8)
pkg/utils/yaml_utils.go (1)
98-98: Improved code readability with range-based loop.The change from index-based to range-based loop makes the code more concise and less error-prone.
pkg/filetype/filetype.go (2)
87-104: Effective approach for preserving YAML structure.The two-step process of unmarshaling to a yaml.Node first, then processing the node structure before decoding to a Go value is a clean solution that properly maintains the original structure while ensuring special characters are handled correctly.
106-118: Well-implemented recursive node processing.The
processYAMLNodefunction efficiently traverses the YAML node tree and applies the single-quoted style to string nodes starting with '#'. The null check at the beginning is a good defensive programming practice.pkg/utils/yaml_include_test.go (1)
13-81: Comprehensive test coverage for '#' character handling.This test thoroughly verifies that strings starting with '#' are properly handled with the !include function, testing various scenarios including unquoted, single-quoted, and double-quoted strings. The test setup with temporary files and directory changes is particularly well-structured.
pkg/utils/yq_utils_test.go (3)
10-19: Good error case test.Testing the error handling for invalid YAML input ensures robustness.
21-55: Thorough tests for hash detection function.The test cases cover all important scenarios for the
isSimpleStringStartingWithHashhelper, including positive and negative cases with various string contents.
57-171: Excellent node processing test coverage.These tests thoroughly verify the YAML node processing functionality:
- Simple scalar nodes with and without '#'
- Complex nested structures with mappings and sequences
- Edge case handling for nil nodes
The assertions check both that strings starting with '#' get the single-quoted style and that regular strings remain unchanged.
pkg/utils/yq_utils.go (1)
38-46:❓ Verification inconclusive
Global logger mutation is not goroutine–safe
configureYqLoggertweaks the package-global logger every timeEvaluateYqExpressionis called. Concurrent calls with differentAtmosConfigurationvalues could race, leaving the logger in an unexpected state (e.g., one goroutine turns tracing on while another silently disables it).Consider guarding the call with a
sync.Once, a dedicated mutex, or—preferably—passing an explicit logger instance toyqlibinstead of mutating the global.
Sure thing—let’s inspect the imports and the logger definitions to confirm whether
GetLoggerandSetBackendmutate global state.
🏁 Script executed:
#!/bin/bash # Show package and imports in yq_utils.go sed -n '1,50p' pkg/utils/yq_utils.go # Look for a local yqlib package rg "package yqlib" -A5 # Locate GetLogger implementation rg "func GetLogger" -A5 # Locate SetBackend method on the logger type rg "SetBackend" -A5Length of output: 1851
Verify YQ logger’s global mutation safety
I couldn’t locate
GetLoggerandSetBackendimplementations in the local repo (they live ingithub.com/mikefarah/yq/v4/pkg/yqlib), so please confirm:
- Whether
yqlib.GetLogger()returns a singleton/global logger.- Whether
logger.SetBackend(...)mutates that shared instance.If both are true, concurrent calls to
EvaluateYqExpressioncan race, flipping the backend on and off unpredictably. To avoid this:• Consider guarding the setup with
sync.Onceor a mutex.
• Or refactor to pass a dedicated logger instance into yqlib instead of mutating its global.
|
💥 This pull request now has conflicts. Could you fix it @Cerebrovinny? 🙏 |
what
#character were being interpreted as comments and returning null values#by setting them to use single-quoted style in YAML nodeswhy
!includefunction to return null values#characterreferences
!includefunction #1205Summary by CodeRabbit
Bug Fixes
#to ensure they are correctly preserved and not misinterpreted as comments, including when using custom tags or YQ expressions.Tests
#.