Skip to content

Fix confmap to preserve original type when a slice of a map is unmarshalled #12816

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

samuelarogbonlo
Copy link

Description

This PR fixes an issue where the confmap package fails to preserve the original type when a slice of a map is unmarshalled. This occurs specifically when using environment variables with numeric values (e.g., SPLUNK_ACCESS_TOKEN=12345) in configurations with slices of maps.
The error manifests as:
'headers[0].default_value' expected type 'string', got unconvertible type 'int', value: '12345'

The root cause is that while the confmap package correctly handles type conversion for regular maps, it doesn't process maps within slices. I've added a new hook function (sliceOfMapsHookFunc) that specifically converts numeric values to strings within maps that are nested in slices during the unmarshalling process.

Link to tracking issue

Fixes #12793

Testing

I've added a unit test TestSliceOfMapsUnmarshal which verifies that numeric values in maps within slices are correctly converted to strings during unmarshalling. The test creates a configuration that mimics the scenario described in the issue and confirms that the unmarshalling process works as expected.

Documentation

No documentation changes were needed as this is a bug fix for existing functionality.

@samuelarogbonlo samuelarogbonlo requested review from mx-psi, evan-bradley and a team as code owners April 9, 2025 15:30
Copy link

codecov bot commented Apr 9, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 14 lines in your changes missing coverage. Please review.

Project coverage is 91.39%. Comparing base (6b9d125) to head (b7ae9e2).
Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
confmap/confmap.go 50.00% 11 Missing and 3 partials ⚠️

❌ Your patch check has failed because the patch coverage (50.00%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12816      +/-   ##
==========================================
- Coverage   91.44%   91.39%   -0.06%     
==========================================
  Files         487      488       +1     
  Lines       26846    26906      +60     
==========================================
+ Hits        24549    24590      +41     
- Misses       1814     1829      +15     
- Partials      483      487       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I don't think this is the right way to solve the problem mentioned on the linked issue. I believe the problem stems from the use of *configopaque.String here, which we are not supporting properly.

I am against any solution that involves converting numeric values to strings indiscriminately: this leads to buggy and hard to understand behavior where something like 0123 ends up setting the string "83". Same goes for any other type with multiple possible representations.

@samuelarogbonlo
Copy link
Author

Thanks for the PR!

I don't think this is the right way to solve the problem mentioned on the linked issue. I believe the problem stems from the use of *configopaque.String here, which we are not supporting properly.

I am against any solution that involves converting numeric values to strings indiscriminately: this leads to buggy and hard to understand behavior where something like 0123 ends up setting the string "83". Same goes for any other type with multiple possible representations.

Thanks for the feedback! I understand your concerns about indiscriminately converting numeric values to strings, which could indeed lead to unexpected behavior.

I see that the issue is specifically related to configopaque.String rather than a general problem with maps in slices. I'll revise my approach to create a targeted hook specifically for configopaque.String types that preserves the original representation of values.

I'll work on implementing this more focused solution and update the PR soon.

@mx-psi
Copy link
Member

mx-psi commented Apr 11, 2025

Awesome! See my comments over at #12793 to help you get started

@samuelarogbonlo
Copy link
Author

I've implemented the solution suggested by @mx-psi by adding pointer type handling to isStringyStructure and useExpandValue. This should properly address the issue with configopaque.String types in a more targeted way. The existing tests are now passing, showing that the fix works correctly.

@samuelarogbonlo
Copy link
Author

I've implemented a more targeted solution as suggested by @mx-psi. Instead of broadly converting all numeric values in maps within slices, this fix specifically addresses the issue with configopaque.String types by adding a dedicated hook that converts numeric values to strings when the target type is configopaque.String (either direct or pointer). I've added a test that verifies this fix correctly handles the scenario from the issue.

@samuelarogbonlo samuelarogbonlo requested a review from mx-psi April 16, 2025 09:30
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.

[confmap] is not preserving the original type when a slice of a map is unmarshalled
2 participants