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

Deliberately set empty field #17

Closed
emmaly opened this issue Jan 6, 2014 · 13 comments
Closed

Deliberately set empty field #17

emmaly opened this issue Jan 6, 2014 · 13 comments

Comments

@emmaly
Copy link

emmaly commented Jan 6, 2014

I need a way to explicitly set a field value to empty and not have it be ignored by gorilla/schema when processed. It is important to not simply wipe out all missing fields, but only make empty the fields that were provided and somehow marked as intentionally empty.

Use case: struct that is pre-populated from a database entry that is then passed into schema.Decoder to apply matched fields from a web form.

Possibly if the fields exist in the incoming web form and are empty, they can be wiped out while leaving alone the fields in the struct that weren't referenced in the web form. Such a thing would need testing that I haven't myself done.

I've created a very hacky patch[1] to allow deliberately unsetting a field value when empty, instead of just ignoring it. In an application I'm building, I pull in a record from the database and then use schema to merge changes. When the data coming in from a web form is empty, I intend for the value to be wiped out, not just ignored. But because of how structs work, it's really not easy to tell the difference between absent field versus a deliberately empty value. In this very hacky patch, if it hits a string value of "[[[ERASE]]]" it will set the field value to "". It's not good enough and is bound to have loads of gotchas. Maybe there an alternative way to do this that I've missed; if so, sorry for the wishlist request.

[1] scjalliance@fdf73b8 (don't actually use this commit)

@kisielk
Copy link
Contributor

kisielk commented Jan 6, 2014

Any reason you can't simply set the field to "" before passing the struct to the decoder?

@emmaly
Copy link
Author

emmaly commented Jan 9, 2014

I may be misunderstanding, but on lines 139-141 in decoder.go, it expressly ignores the value when it is set to "". This is not the same as unsetting the value. In my code, I'm taking a pre-populated struct and having the decoder overwrite the struct. If any of the incoming data is set to "", it'll just skip it instead of setting the written-to struct as empty string, which leaves the original value in place. Let me know if what you said is not what I seemed to have interpreted.

@kisielk
Copy link
Contributor

kisielk commented Jan 9, 2014

Okay, let's see if I understand correctly:

You have a struct that is prefilled with data. If the form value for a field is "", you want it to blank out the corresponding field in the struct. The current behaviour just ignores the form value and leaves the field in the struct intact.

Is that the gist of it?

@emmaly
Copy link
Author

emmaly commented Jan 9, 2014

That is correct.

@kisielk
Copy link
Contributor

kisielk commented Jan 9, 2014

Okay, I get it then :) So I think the best way to do this is to add an option to the decoder that will clear the fields. We can't change the default behaviour because it may break existing usage.

@emmaly
Copy link
Author

emmaly commented Jan 9, 2014

In my use case, it's important that any omitted fields (from a form post, for example) are just ignored, yet any actually empty but submitted fields are cleared. Do you believe that such a thing can be done without the hackery I've applied in my not-to-be-used commit I've linked above? I honestly haven't thoroughly evaluated the schema code, but it seems that the absence of a field is able to be known and that it could work out. I'll see about making a workable solution and we can discuss if it is a reasonable solution. I just want to be sure there wasn't already a best practice that doesn't suck (like pre-pre-processing my inputs, for example). Thanks.

@kisielk
Copy link
Contributor

kisielk commented Jan 9, 2014

It should work out that way since we iterate over the keys in the map you receive from the form. If the key is not there we will never try to change the field.

@kisielk
Copy link
Contributor

kisielk commented Jan 9, 2014

This is actually partially addressed in #12 , but the solution there is not quite complete (eg: it changes the default behaviour, and no tests yet), which is why I haven't merged it.

@emmaly
Copy link
Author

emmaly commented Jan 9, 2014

Oh, hah. Well I didn't even see that one apparently. That'd be probably what I want on my end. What would be helpful for moving that forward? Anything I can do?

@emmaly
Copy link
Author

emmaly commented Jan 9, 2014

It looks untested and probably incomplete. I merged his changes into my HEAD^ and I'll see about adding the test cases I care about. My time today is rather tight, so I hope I actually complete it. I'm going to close this issue and let RangelReale's issue #12 carry this forward as it seems to have compatible goals. Thanks.

@emmaly emmaly closed this as completed Jan 9, 2014
@kisielk
Copy link
Contributor

kisielk commented Jan 9, 2014

Thanks for the feedback though, now I have a pretty good idea of how that PR should work. I'd like to add another function to the decoder like dec.ZeroEmpty(). When called it will set a flag on the decoder. If the decoder encounters an empty string, it will check the value of the flag. If the flag is set, then it will set the field to the zero value as it does in that PR, otherwise it will ignore it.

@emmaly
Copy link
Author

emmaly commented Jan 9, 2014

Sounds good to me. Would it be helpful for me to write the tests that I'm concerned about, or shall I wait until you've made proposed changes?

@kisielk
Copy link
Contributor

kisielk commented Jan 9, 2014

Tests would definitely help, thanks.

kisielk added a commit that referenced this issue Jan 10, 2014
Fixes #17, partially fixes #12.
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