Skip to content

Support omitzero #729

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
May 22, 2025
+243 −8
Merged

Support omitzero #729

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ea34559
support omitzero
goccy May 9, 2025
283ef1b
format
goccy May 9, 2025
a632829
rename internal method
goccy May 17, 2025
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 76 additions & 6 deletions encode.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ type Encoder struct {
anchorNameMap map[string]struct{}
anchorCallback func(*ast.AnchorNode, interface{}) error
customMarshalerMap map[reflect.Type]func(interface{}) ([]byte, error)
omitZero bool
omitEmpty bool
autoInt bool
useLiteralStyleIfMultiline bool
Expand Down Expand Up @@ -728,7 +729,7 @@ type IsZeroer interface {
IsZero() bool
}

func (e *Encoder) isZeroValue(v reflect.Value) bool {
func (e *Encoder) isOmittedByOmitZero(v reflect.Value) bool {
kind := v.Kind()
if z, ok := v.Interface().(IsZeroer); ok {
if (kind == reflect.Ptr || kind == reflect.Interface) && v.IsNil() {
Expand All @@ -737,13 +738,74 @@ func (e *Encoder) isZeroValue(v reflect.Value) bool {
return z.IsZero()
}
switch kind {
case reflect.String:
return len(v.String()) == 0
case reflect.Interface, reflect.Ptr, reflect.Slice, reflect.Map:
return v.IsNil()
case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
return v.Int() == 0
case reflect.Float32, reflect.Float64:
return v.Float() == 0
case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr:
return v.Uint() == 0
case reflect.Bool:
return !v.Bool()
case reflect.Struct:
vt := v.Type()
for i := v.NumField() - 1; i >= 0; i-- {
if vt.Field(i).PkgPath != "" {
continue // private field
}
if !e.isOmittedByOmitZero(v.Field(i)) {
return false
}
}
return true
}
return false
}

func (e *Encoder) isOmittedByOmitEmptyOption(v reflect.Value) bool {
switch v.Kind() {
case reflect.String:
return len(v.String()) == 0
case reflect.Interface, reflect.Ptr:
return v.IsNil()
case reflect.Slice:
case reflect.Slice, reflect.Map:
return v.Len() == 0
case reflect.Map:
case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
return v.Int() == 0
case reflect.Float32, reflect.Float64:
return v.Float() == 0
case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr:
return v.Uint() == 0
case reflect.Bool:
return !v.Bool()
}
return false
}

// The current implementation of the omitempty tag combines the functionality of encoding/json's omitempty and omitzero tags.
// This stems from a historical decision to respect the implementation of gopkg.in/yaml.v2, but it has caused confusion,
// so we are working to integrate it into the functionality of encoding/json. (However, this will take some time.)
// In the current implementation, in addition to the exclusion conditions of omitempty,
// if a type implements IsZero, that implementation will be used.
// Furthermore, for non-pointer structs, if all fields are eligible for exclusion,
// the struct itself will also be excluded. These behaviors are originally the functionality of omitzero.
func (e *Encoder) isOmittedByOmitEmptyTag(v reflect.Value) bool {
kind := v.Kind()
if z, ok := v.Interface().(IsZeroer); ok {
if (kind == reflect.Ptr || kind == reflect.Interface) && v.IsNil() {
return true
}
return z.IsZero()
}
switch kind {
case reflect.String:
return len(v.String()) == 0
case reflect.Interface, reflect.Ptr:
return v.IsNil()
case reflect.Slice, reflect.Map:
return v.Len() == 0
case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
return v.Int() == 0
Expand All @@ -759,7 +821,7 @@ func (e *Encoder) isZeroValue(v reflect.Value) bool {
if vt.Field(i).PkgPath != "" {
continue // private field
}
if !e.isZeroValue(v.Field(i)) {
if !e.isOmittedByOmitEmptyTag(v.Field(i)) {
return false
}
}
Expand Down Expand Up @@ -818,8 +880,16 @@ func (e *Encoder) encodeStruct(ctx context.Context, value reflect.Value, column
}
fieldValue := value.FieldByName(field.Name)
sf := fieldMap[field.Name]
if (e.omitEmpty || sf.IsOmitEmpty) && e.isZeroValue(fieldValue) {
// omit encoding
if (e.omitZero || sf.IsOmitZero) && e.isOmittedByOmitZero(fieldValue) {
// omit encoding by omitzero tag or OmitZero option.
continue
}
if e.omitEmpty && e.isOmittedByOmitEmptyOption(fieldValue) {
// omit encoding by OmitEmpty option.
continue
}
if sf.IsOmitEmpty && e.isOmittedByOmitEmptyTag(fieldValue) {
// omit encoding by omitempty tag.
continue
}
ve := e
Expand Down
147 changes: 147 additions & 0 deletions encode_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -584,6 +584,111 @@ func TestEncoder(t *testing.T) {
nil,
},

// omitzero flag.
{
"a: 1\n",
struct {
A int `yaml:"a,omitzero"`
B int `yaml:"b,omitzero"`
}{1, 0},
nil,
},
{
"{}\n",
struct {
A int `yaml:"a,omitzero"`
B int `yaml:"b,omitzero"`
}{0, 0},
nil,
},
{
"a:\n \"y\": \"\"\n",
struct {
A *struct {
X string `yaml:"x,omitzero"`
Y string
}
}{&struct {
X string `yaml:"x,omitzero"`
Y string
}{}},
nil,
},
{
"a: {}\n",
struct {
A *struct {
X string `yaml:"x,omitzero"`
Y string `yaml:"y,omitzero"`
}
}{&struct {
X string `yaml:"x,omitzero"`
Y string `yaml:"y,omitzero"`
}{}},
nil,
},
{
"a: {x: 1}\n",
struct {
A *struct{ X, y int } `yaml:"a,omitzero,flow"`
}{&struct{ X, y int }{1, 2}},
nil,
},
{
"{}\n",
struct {
A *struct{ X, y int } `yaml:"a,omitzero,flow"`
}{nil},
nil,
},
{
"a: {x: 0}\n",
struct {
A *struct{ X, y int } `yaml:"a,omitzero,flow"`
}{&struct{ X, y int }{}},
nil,
},
{
"a: {x: 1}\n",
struct {
A struct{ X, y int } `yaml:"a,omitzero,flow"`
}{struct{ X, y int }{1, 2}},
nil,
},
{
"{}\n",
struct {
A struct{ X, y int } `yaml:"a,omitzero,flow"`
}{struct{ X, y int }{0, 1}},
nil,
},
{
"a: 1.0\n",
struct {
A float64 `yaml:"a,omitzero"`
B float64 `yaml:"b,omitzero"`
}{1, 0},
nil,
},
{
"a: 1\nb: []\n",
struct {
A int
B []string `yaml:"b,omitzero"`
}{
1, []string{},
},
nil,
},
{
"{}\n",
struct {
A netip.Addr `yaml:"a,omitzero"`
B struct{ X, y int } `yaml:"b,omitzero"`
}{},
nil,
},

// OmitEmpty global option.
{
"a: 1\n",
Expand All @@ -605,6 +710,48 @@ func TestEncoder(t *testing.T) {
yaml.OmitEmpty(),
},
},
{
"a: \"\"\nb: {}\n",
struct {
A netip.Addr `yaml:"a"`
B struct{ X, y int } `yaml:"b"`
}{},
[]yaml.EncodeOption{
yaml.OmitEmpty(),
},
},

// OmitZero global option.
{
"a: 1\n",
struct {
A int
B int
}{1, 0},
[]yaml.EncodeOption{
yaml.OmitZero(),
},
},
{
"{}\n",
struct {
A int
B int
}{0, 0},
[]yaml.EncodeOption{
yaml.OmitZero(),
},
},
{
"{}\n",
struct {
A netip.Addr `yaml:"a"`
B struct{ X, y int } `yaml:"b"`
}{},
[]yaml.EncodeOption{
yaml.OmitZero(),
},
},

// Flow flag.
{
Expand Down
15 changes: 13 additions & 2 deletions option.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,15 +215,26 @@ func AutoInt() EncodeOption {
}
}

// OmitEmpty forces the encoder to assume an `omitempty` struct tag is
// set on all the fields. See `Marshal` commentary for the `omitempty` tag logic.
// OmitEmpty behaves in the same way as the interpretation of the omitempty tag in the encoding/json library.
// set on all the fields.
// In the current implementation, the omitempty tag is not implemented in the same way as encoding/json,
// so please specify this option if you expect the same behavior.
func OmitEmpty() EncodeOption {
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to the comment above, ideally we have 3 global options:

OmitEmpty() // Keep old commentary
OmitZero()
EnableStdOmitEmpty() // If true, std omitempty is used for both OmitEmpty option and tag.
Copy link
Owner Author

Choose a reason for hiding this comment

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

I do not want to add a new option like EnableStdOmitEmpty() because it would be difficult to maintain if it needs to be deprecated later. While it is true that the difference in behavior between the OmitEmpty option and the omitempty tag can be confusing, I believe it is acceptable since it is documented in the comments and, in practice, most people are not aware of the behavioral differences.
Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, thanks. I wonder how open you are to reconsider this decision - I still think different tag vs option is much more difficult to maintain and understand in your codebase.

Let's try to unpack a bit more, if you don't mind, but eventually it's your decision:

I assume our intentions here are the same, that go-yaml features and code are functional, yet easy to use and maintain. Those qualities are important if we want to enable wider adoption of this library in the CNCF ecosystem.

I do not want to add a new option like EnableStdOmitEmpty() because it would be difficult to maintain if it needs to be deprecated later.

Let's say we switch one day to "std" (new) omitempty behaviour for tag as well -- I would argue you still would need an option to "Disable" this and switch back to the old behaviour IMO. As a result nothing needs to be deprecated. To make it smooth instead of EnableStdOmitEmpty that might be weird when you make the switch, we could have OmitEmptyLogic(value OmitEmptyLogicEnum) option.

While it is true that the difference in behavior between the OmitEmpty option and the omitempty tag can be confusing, I believe it is acceptable since it is documented in the comments and, in practice, most people are not aware of the behavioral differences.

People are not aware until something breaks in a hidden way. Our product just last month had almost a production issue because of a bad omitempty handling around backward compatibility micro-services rollout. People want useful and consistent defaults and some control to unblock them if needed. Having different omitempty logic for tag vs global option in my opinion violates all of those user needs:

  • usefulness: There is no reason to have different tag vs option logic really, especially from user standpoint. Looks like there are some maintainability reasons, but as I mentioned before, is this really true? We need global option to switch logic back (or forward) anyway, so the same amount of code is needed both ways.
  • tag vs option is not consistent and it can create surprises. Imagine you want to see and play with encoding and you put global omitempty option. Now let's say the result is as you expect so you then decide to put more "permanent" tags and the output is totally different. Is this something user would want to step into?
  • no control to decide themselves e.g. tag having new logic.

As a result, I would argue it's not acceptable, but that's my own experience maintaining ~30k direct importers Go library. 🙈
Copy link
Owner Author

Choose a reason for hiding this comment

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

Hmmm, I don't understand why a method to revert the logic is necessary. This is because the current behavior of the omitempty tag can be expressed as a combination of the OmitZero and OmitEmpty options (omitempty tag == OmitEmpty and OmitZero option). Similarly, when specifying it with a tag, you can simply specify two tags like omitempty,omitzero.

Additionally, I also question how much the OmitEmpty option will actually be used. For new users utilizing this option, I believe there won't be an issue as long as the comments and behavior are consistent. This is because if they expect behavior similar to the omitempty tag, they can simply specify the OmitZero option at the same time.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, I don't understand why a method to revert the logic is necessary. This is because the current behavior of the omitempty tag can be expressed as a combination of the OmitZero and OmitEmpty options (omitempty tag == OmitEmpty and OmitZero option). Similarly, when specifying it with a tag, you can simply specify two tags like omitempty,omitzero.

Yes, so the revert is necessary because you are planning to make the tag logic switch in v1 (which I don't recommend). Despite loud release note lines etc, users for sure will be surprised and have production incidents/broken yaml processing because of that breaking change. "Revert" (so option to switch back the logic for omitempty) allows to at least quickly use the latest go-yaml again and not get blocked. You might be surprised how "just" adding omitzero is impossible or takes months-years in many cases (e.g. encoding third party structs). I am also not 100% sure you can assume "current" omitempty logic == union of "std" omitempty and omitzero logic -- there are so many nuances. 🙈

Additionally, I also question how much the OmitEmpty option will actually be used. For new users utilizing this option, I believe there won't be an issue as long as the comments and behavior are consistent. This is because if they expect behavior similar to the omitempty tag, they can simply specify the OmitZero option at the same time.

Yea, global option is new so it can do whatever you are right, but since the above (IMO) logic flag is required anyway, it's a straightforward decision to not make anyones life harder and treat OmitEmpty option and tag the same thing. Imagine the next contributor looking into code and realizing tag and option have different meaning despite the exact same known name.

To me this decision to be a bit slower, have extra switch and consistent omitempty is exponentially cheaper and easier to read use and maintain than having different tag vs option and suddenly at one point the same tag and option and no control over that by a user. Users hate to do more work without notice and current go mod + dependabot tools will simply upgrade go-yaml for them with the omitempty breaking change in a hidden way.

Anyway, sorry for being a pain here, I just really would love to make sure we can use this library, and "our" importers (e.g. Kubernetes) are extremely strict of the stability of (even indirect) dependencies. See our previous discussions 🙈
Copy link
Contributor

@bwplotka bwplotka May 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Also, one important factor why global option of OmitEmptyLogic(enum) might be preferable -- it might be an amazing feature to have, not a "tech debt"

The prev. YAML lib had this omit empty behaviour without a change for a reason - likely because it was wanted by community. So why not allowing users to choose, since the cost of is trivially small (one option, small function to maintain and bit more docs)
return func(e *Encoder) error {
e.omitEmpty = true
return nil
}
}

// OmitZero forces the encoder to assume an `omitzero` struct tag is
// set on all the fields. See `Marshal` commentary for the `omitzero` tag logic.
func OmitZero() EncodeOption {
return func(e *Encoder) error {
e.omitZero = true
return nil
}
}

// CommentPosition type of the position for comment.
type CommentPosition int

Expand Down
3 changes: 3 additions & 0 deletions struct.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ type StructField struct {
IsAutoAnchor bool
IsAutoAlias bool
IsOmitEmpty bool
IsOmitZero bool
IsFlow bool
IsInline bool
}
Expand Down Expand Up @@ -53,6 +54,8 @@ func structField(field reflect.StructField) *StructField {
switch {
case opt == "omitempty":
sf.IsOmitEmpty = true
case opt == "omitzero":
sf.IsOmitZero = true
case opt == "flow":
sf.IsFlow = true
case opt == "inline":
Expand Down
4 changes: 4 additions & 0 deletions yaml.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,10 @@ func (s MapSlice) ToMap() map[interface{}]interface{} {
// encoding/json 'omitempty' definition. It combines some elements
// of 'omitempty' and 'omitzero'. See https://github.com/goccy/go-yaml/issues/695.
//
// omitzero The omitzero tag behaves in the same way as the interpretation of the omitzero tag in the encoding/json library.
// 1) If the field type has an "IsZero() bool" method, that will be used to determine whether the value is zero.
// 2) Otherwise, the value is zero if it is the zero value for its type.
//
// flow Marshal using a flow style (useful for structs,
// sequences and maps).
//
Expand Down