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

[FEATURE] Use omitempty by default #216

Open
1 task done
david-its opened this issue Aug 16, 2024 · 4 comments
Open
1 task done

[FEATURE] Use omitempty by default #216

david-its opened this issue Aug 16, 2024 · 4 comments

Comments

@david-its
Copy link

Is there an existing feature request for this?

  • I have searched the existing feature requests

Is your feature request related to a problem? Please describe.

We heavily use schema to encode request parameters in URL and to avoid adding "omitempty" in all tags would like to have an ability to make it default behavior by calling Encoder.SetDefaultOmitEmpty(true).
If no objections we can submit a patch for that.

Describe the solution that you would like.

A new method Encoder.SetDefaultOmitEmpty(true) which makes omitempty to be a default behavior.

Describe alternatives you have considered.

No response

Anything else?

No response

@zak905
Copy link
Contributor

zak905 commented Aug 25, 2024

Hi @david-its, I was able to reproduce the issue, off course this would happen only when encoding. The following can be used as workaround:

	var probe *bool
	encoder.RegisterEncoder(probe, func(v reflect.Value) string {
		if v.IsNil() {
			return "false"
		}

		return fmt.Sprintf("%v", v.Elem().Bool())
	})

are you willing to provide an implementation to the solution ?

@david-its
Copy link
Author

david-its commented Aug 25, 2024

Such workaround is not correct unfortunately. When we use *bool we support tristate options (undefined, true, false). With this workaround you get only 2 states (true/false) which is not the same.

I'm not sure which solution is better:

  • to fix decoder which should treat "null" as nil pointer.
  • or to have proposed Encoder.SetDefaultOmitEmpty(true) which would simply remove such flags on encode and thus avoid decoding it.

2nd solution is very simple and I can create a PR.

@zak905
Copy link
Contributor

zak905 commented Aug 25, 2024

awesome thanks. I am also leaning towards option 2, because using a flag, we can keep backward compatibility, just in case someone is misusing the old behavior.

Yes, indeed, I overlooked the case where the value is nil, in this case an empty string should be returned. a key will be present in the values without a value, but this is ok I guess when dealing with form submissions or query parameters

var probe *bool
encoder.RegisterEncoder(probe, func(v reflect.Value) string {
		if v.IsNil() {
			return ""
		}

		return fmt.Sprintf("%v", v.Elem().Bool())
	})

david-its pushed a commit to david-its/schema that referenced this issue Aug 25, 2024
By default encoder encodes all nil ptr fields as "null" and decoder is
not able to decode them. Encoding such fields as empty ("") also doesn't
work properly as decoder always initializes ptr fields to zero values
even empty value provided. The only option is to add omitempty tags.

encoder.SetOmitEmptyDefault() allows to make omitempty to be a default
behaviour.

This closes issue gorilla#216
@david-its
Copy link
Author

awesome thanks. I am also leaning towards option 2, because using a flag, we can keep backward compatibility, just in case someone is misusing the old behavior.

Yes, indeed, I overlooked the case where the value is nil, in this case an empty string should be returned. a key will be present in the values without a value, but this is ok I guess when dealing with form submissions or query parameters

var probe *bool
encoder.RegisterEncoder(probe, func(v reflect.Value) string {
		if v.IsNil() {
			return ""
		}

		return fmt.Sprintf("%v", v.Elem().Bool())
	})

@zak905 this doesn't work as well, cause decoder automatically allocates zero values to fields even if there is no value provided. If I remember correctly, it happens at lines 306-312 in decode():

        if t.Kind() == reflect.Ptr {
                t = t.Elem()
                if v.IsNil() {
                        v.Set(reflect.New(t))
                }
                v = v.Elem()
        }

as a result you never have nil ptr in the field even though you decode field=.

I have submitted PR for default omit empty.

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

No branches or pull requests

2 participants