Closed johnniedoe closed 8 years ago
It's not clear to me what this change does. It seems like it just changes the name of the field, which seems to be a change that does nothing.
The change causes the list of struct fields to be keyed off of the encName rather than the fieldName. As a result, a shadowed struct field can still be encoded so long as the tag-specified alias is not shadowed. In my tests, this fixes the first variable shadowing issue mentioned in #159.
If you'd like, I can add a test to the unit tests demonstrating it.
But if I look at https://github.com/ugorji/go/pull/160/commits/b53f7c41dc5efa3112caedaecc67e2b2f72d4624 , it just looks like you changed the word "fieldName" to "encName".
I can't see how that change actually does anything. It seems more like a refactoring.
Please take a look at the actual change again.
The rename of "fieldName" to "encName" within the sfiIdx struct was made for consistency/clarity, but shouldn't have any other runtime effects.
The change that really makes a difference is on 1011. The function is forcing uniqueness of fields by comparing them against a list of already "found" ones. Before the change, it was using the "fieldName" field of "structFieldInfo" as the key for comparison. Post-change, it is using the "encName" field. These are two different fields in a pre-existing struct.
To see the difference that it makes, try running this sample program against both versions:
import (
"bytes"
"encoding/json"
"fmt"
"github.com/ugorji/go/codec"
)
type Composite struct {
X string
Embedded
}
type Embedded struct {
X string `json:"x"`
}
func main() {
d := Composite{X:"a",Embedded:Embedded{X:"b"}}
msg, _ := json.Marshal(d)
fmt.Println(" json: ", string(msg))
buf := &bytes.Buffer{}
codec.NewEncoder(buf, &codec.JsonHandle{}).Encode(d)
fmt.Println("codec: ", buf.String())
}
The existing code should produce:
json: {"X":"a","x":"b"}
codec: {"X":"a"}
while the updated code should produce:
json: {"X":"a","x":"b"}
codec: {"X":"a","x":"b"}
I did verify that the updated version passes the existing unit tests. However, there may be case(s) that I'm not thinking of that aren't in the unit tests...
Looks good. I have validated it.
I can't merge it, because there are a few other merges that don't apply.
If you can make this a standalone merge, I will merge it. Else, I will just include the fix in my next updates (but you lose credit for the change). Let me know your preference.
@johnniedoe ^^ (please read and respond to prior message)
Updated full fix for #159 in change c0d481cf0ab1f0d68f8257fd3ffa267506dfc04b . This pull request will no longer work.
Sorry for the slow reply. I'd been away on vacation.
Yours looks like the more comprehensive solution. Mine only handled one of the issues mentioned in #159, while your appears to handle both. Nice.
Thanks again for the fix.
I think that this would resolve the first use case mentioned in #159
The second use case seems a bit more involved.