Skip to content

Conversation

@mx-psi
Copy link
Member

@mx-psi mx-psi commented May 8, 2025

Description

If we enable DecodeNil as true, we may have untyped nils being passed. Unfortunately, these are not valid values for reflect, which leads to surprising behavior such as golang/go/issues/51649.

Unfortunately, the default hooks from mapstructure do not deal with this properly. To account for this, we:

  • Vendor and change the ComposeDecodeHookFunc function so that this case is accounted for the kinds of hooks that just won't work with untyped nils
  • Create a safe wrapper for the hooks that do work with untyped nils. This wrapper is used in all hooks, but in the interest of keeping as close to what I would imagine upstream will accept, I did not add this to the compose function.

This should not have any end-user observable behavior.

Link to tracking issue

Attempt to work around #12996 (comment)

Working towards #12981

@github-actions github-actions bot requested a review from evan-bradley May 8, 2025 14:11
@codecov
Copy link

codecov bot commented May 8, 2025

Codecov Report

Attention: Patch coverage is 76.00000% with 18 lines in your changes missing coverage. Please review.

Project coverage is 91.62%. Comparing base (6bd77b3) to head (94dd45c).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...p/internal/third_party/composehook/compose_hook.go 76.19% 14 Missing and 1 partial ⚠️
confmap/confmap.go 75.00% 2 Missing and 1 partial ⚠️

❌ Your patch status has failed because the patch coverage (76.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   #13001      +/-   ##
==========================================
- Coverage   91.67%   91.62%   -0.05%     
==========================================
  Files         503      504       +1     
  Lines       27819    27888      +69     
==========================================
+ Hits        25502    25553      +51     
- Misses       1828     1844      +16     
- Partials      489      491       +2     

☔ 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.

@mx-psi mx-psi force-pushed the mx-psi/reimplement-as-value-hooks branch from b9830b5 to ac3a9f2 Compare May 8, 2025 14:49
@mx-psi mx-psi marked this pull request as ready for review May 8, 2025 14:50
@mx-psi mx-psi requested a review from a team as a code owner May 8, 2025 14:50
@mx-psi mx-psi requested a review from yurishkuro May 8, 2025 14:52
@mx-psi mx-psi requested review from jmacd and yurishkuro May 9, 2025 12:19
@mx-psi mx-psi changed the title [chore] Make all hooks be mapstructure.DecodeHookFuncValue [chore] Make mapstructure hooks safe against untyped nils May 9, 2025
Copy link
Contributor

@evan-bradley evan-bradley left a comment

Choose a reason for hiding this comment

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

Is there a good way to test the added functionality in this PR, or does it just feed into #12996 and we can test it there?

@mx-psi
Copy link
Member Author

mx-psi commented May 9, 2025

Is there a good way to test the added functionality in this PR, or does it just feed into #12996 and we can test it there?

I think this should not make any difference, I was relying on the existing test suite, including the tests I added on #12998. We will see a behavioral difference with #12996 and we can test it further there

@mx-psi mx-psi requested a review from evan-bradley May 9, 2025 14:19
@mx-psi mx-psi enabled auto-merge May 9, 2025 14:27
@mx-psi mx-psi added this pull request to the merge queue May 9, 2025
Merged via the queue into open-telemetry:main with commit 5800834 May 9, 2025
56 of 57 checks passed
@mx-psi mx-psi deleted the mx-psi/reimplement-as-value-hooks branch May 9, 2025 14:58
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.

4 participants