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

Incorrectly decode to embedded field #31

Open
powerman opened this issue Apr 6, 2018 · 4 comments
Open

Incorrectly decode to embedded field #31

powerman opened this issue Apr 6, 2018 · 4 comments
Assignees

Comments

@powerman
Copy link

powerman commented Apr 6, 2018

If same field exists both in parent and embedded struct and url.Values has value only for parent then decoder incorrect set embedded field to same value.

type Embed struct {
	A string
}
var data struct {
	A string
	Embed
}
form.NewDecoder().Decode(&data, url.Values{
	"A": {"one"},
})
fmt.Println(data.A, data.Embed.A)
// Output: one one

If we will add "Embed.A":{"two"} or keep only this one and remove "A": {"one"} then it'll work as expected.

@deankarn
Copy link
Contributor

deankarn commented Apr 6, 2018

This was one that I struggled with a bit and was done on purpose see:

#19
https://github.com/go-playground/form/releases/tag/v3.0.0

in hindsight perhaps I should have made it a configurable option...that can still be done though.

@deankarn deankarn self-assigned this Apr 6, 2018
@powerman
Copy link
Author

powerman commented Apr 6, 2018

if a struct and embedded struct have the same field, this will decode the value into both fields as there is no way to tell which you want

Why is that? It's very easy to tell - just use same rules as Go itself. If there are two different fields in a struct you're decoding to and just one input value then why do you think anyone may like to put this value into both fields? Go doesn't do this, and we shouldn't too.

Of course, there is another one, perfectly legal, use case - when field is just one, but it has two (or more) different names because of embedding struct. In this case decoder should let us to use any of these names (because Go let us work in same way) - and this is already supported.

@deankarn
Copy link
Contributor

deankarn commented Apr 6, 2018

I agree, for the most part, The trouble ATM is this library does not do a lookahead to know if there is another field with the same name to know that there are two fields which can also be quite an undertaking when multiple embeds exist I will have to look into 2 options:

  1. look at extending the cache logic, which is difficult as many values can be uninitialized and need to be initialized before traversing, not insurmountable, but also not fun.

  2. add a setting to each map value indicating it's already been set, which seems like the easiest and least performance impacting solution.

I'll think about how I can integrate # 2 which also gives me some ideas for solving/implementing some other enhancements.

@powerman
Copy link
Author

powerman commented Apr 6, 2018

know if there is another field with the same name

What's for? Why you can't just decode each value as is and let Go handle embedded fields for you?

In any case, just FYI, I'm now working on implementing #28 as a wrapper for this package (this doesn't mean I give up on idea to turn it into PR to this package if you'll decide you want it), and while it's still WIP and not stabilized yet I've already implemented user's data structure introspection which may be useful for you if you really need to know embedded field name aliases for something. (This code will be on github with MIT license as soon as it'll be finished - in a few days, I hope.) For example, given this data structure:

type Embed struct {
    B string
}
var data struct {
	A string
	Embed
}

it'll return something like this:

type constraint struct {
	alias    string // shortest of all aliases
	// there are other fields here too, unrelated to current subject
}
// keys in result are all possible legal url.Values keys which can be decoded
result := map[string]constraint{
	"A": {alias: "A"},
	"B": {alias: "B"},
	"Embed.B": {alias: "B"},
}

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

No branches or pull requests

2 participants