Skip to content

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

Merged
kzys merged 1 commit into
containerd:mainfrom
Iceber:unmarshal
Apr 26, 2023
Merged

unmarshal does not return nil object when value is nil#41
kzys merged 1 commit into
containerd:mainfrom
Iceber:unmarshal

Conversation

@Iceber

@Iceber Iceber commented Apr 14, 2023

Copy link
Copy Markdown
Member

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

codecov-commenter commented Apr 14, 2023

Copy link
Copy Markdown

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
Comment thread types.go Outdated
}
}

if value == nil {

Copy link
Copy Markdown
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
Copy Markdown
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>

@fuweid fuweid left a comment

Copy link
Copy Markdown
Member

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

Iceber commented May 4, 2023

Copy link
Copy Markdown
Member Author

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

@fuweid

fuweid commented May 5, 2023

Copy link
Copy Markdown
Member

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

@Iceber

Iceber commented May 9, 2023

Copy link
Copy Markdown
Member Author

@fuweid Could we release 2.1.1 now? 😀

@fuweid

fuweid commented May 9, 2023

Copy link
Copy Markdown
Member

@Iceber Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants