Skip to content
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

unmarshal does not return nil object when value is nil #41

Merged
merged 1 commit into from
Apr 26, 2023

Conversation

Iceber
Copy link
Member

@Iceber Iceber commented Apr 14, 2023

issue: containerd/containerd#8392

For unmarshal, thevalue param is nil, it does not mean that typeurl is also empty.
When value is nil, the unmarshal returns nil, nil directly, which becomes cumbersome to use and skips the check for typeurl.

Always have unmarshal return a non-nil object when err is nil, which is consistent with the behavior of v1 typeurl

@codecov-commenter
Copy link

codecov-commenter commented Apr 14, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (2a94991) 68.90% compared to head (7f96cad) 68.90%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #41   +/-   ##
=======================================
  Coverage   68.90%   68.90%           
=======================================
  Files           1        1           
  Lines         119      119           
=======================================
  Hits           82       82           
  Misses         28       28           
  Partials        9        9           
Impacted Files Coverage Δ
types.go 68.90% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Iceber Iceber force-pushed the unmarshal branch 2 times, most recently from 5f76736 to 1d95db6 Compare April 14, 2023 11:08
@Iceber Iceber changed the title unmarshal does not return nil object when value is empty unmarshal does not return nil object when value is nil Apr 14, 2023
@Iceber Iceber marked this pull request as draft April 14, 2023 11:35
@Iceber Iceber marked this pull request as ready for review April 14, 2023 14:21
@Iceber Iceber requested a review from kzys April 19, 2023 06:27
types.go Outdated
@@ -216,6 +212,10 @@ func unmarshal(typeURL string, value []byte, v interface{}) (interface{}, error)
}
}

if value == nil {
Copy link
Member

Choose a reason for hiding this comment

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

IIUC, it should be if len(value) == 0.

Copy link
Member Author

Choose a reason for hiding this comment

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

After some thought, I decided to remove this judgment and bring the behavior in here back to be consistent with v1typeurl.

For proto, the empty value(nil, []byte{}) needs to set all the fields of the incoming v parameter to empty, because maybe the incoming v object already has some fields set

For json, it should not be nil or []byte{}, a valid null value is "null" or '{}', '[]', if value is nil or []byte{}, then json.Marshal will return an error

Signed-off-by: Iceber Gu <wei.cai-nat@daocloud.io>
Copy link
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

@kzys kzys merged commit 7ef6316 into containerd:main Apr 26, 2023
@Iceber
Copy link
Member Author

Iceber commented May 4, 2023

@fuweid @kzys Are we ready to release v2.1.1? I want to update to containerd and release/1.7

@fuweid
Copy link
Member

fuweid commented May 5, 2023

I will push a tag next Monday if no one takes this. @Iceber

@Iceber
Copy link
Member Author

Iceber commented May 9, 2023

@fuweid Could we release 2.1.1 now? 😀

@fuweid
Copy link
Member

fuweid commented May 9, 2023

@Iceber Done.

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