Fix various input normalization issues#6257
Merged
Merged
Conversation
Not doing this meant that we could accidentally return an empty struct instead of the zero value to module clients. This is similar in style to a previous fix that did this for user module types. Signed-off-by: Justin Chadwell <me@jedevc.com>
Without this, we were returning the normalized name back to the SDK - this only seemed to work because we were being case-insensitive in unmarshalling. We should make sure that modules receive their inputs exactly as they requested them. Signed-off-by: Justin Chadwell <me@jedevc.com>
sipsma
approved these changes
Dec 12, 2023
Contributor
sipsma
left a comment
sipsma
left a comment
There was a problem hiding this comment.
Great finds on all accounts, LGTM!
I'm not quite sure of a test that can make this break neatly
I think you could probably call the dag.CurrentFunctionCall() in the actual function implementation itself to get all the raw values provided to the SDK. If that does work and is easy to write a test with then I'd say go for it, but up to you, not worth blocking on.
This was referenced Dec 15, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6252 (the equivalent #6227, but for core types instead of user module types).
In #6167, it looks like we missed another
nilcheck, which manifests as core module types not being set to their zero value but to an empty struct.Additionally, it looks we regressed on #6057, and we weren't correctly converting field names back into the field names that the module requested. Thankfully, this is a pretty simple fix, and I've verified manually this works. I'm not quite sure of a test that can make this break neatly, since it didn't really (
encoding/json"prefer[s] an exact match but also accept[s] a case-insensitive match").