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

Pointers decoding inconsistency (and discussion) #312

Open
mullerch opened this issue Oct 27, 2022 · 1 comment
Open

Pointers decoding inconsistency (and discussion) #312

mullerch opened this issue Oct 27, 2022 · 1 comment

Comments

@mullerch
Copy link

mullerch commented Oct 27, 2022

When decoding a struct containing a string pointer and a struct pointer into a map, the resulting map has a pointer to the string but not a pointer to the struct.

func TestDecode_structToMap_PointersConsistency(t *testing.T) {
	type SourceChild struct {
		String string `mapstructure:"string"`
	}

	type SourceParent struct {
		ChildPtr  *SourceChild `mapstructure:"child-ptr"`
		StringPtr *string      `mapstructure:"string-ptr"`
	}

	var target map[string]interface{}

	var value = "goodbye"

	source := SourceParent{
		ChildPtr: &SourceChild{
			String: "hello",
		},
		StringPtr: &value,
	}

	if err := Decode(source, &target); err != nil {
		t.Fatalf("got error: %s", err)
	}

	expected := map[string]interface{}{
		"child-ptr": map[string]interface{}{
			"string": "hello",
		},
		"string-ptr": "goodbye",
	}

	expectedPointers := map[string]interface{}{
		"child-ptr": &map[string]interface{}{
			"string": "hello",
		},
		"string-ptr": stringPtr("goodbye"),
	}

	if !reflect.DeepEqual(target, expected) && !reflect.DeepEqual(target, expectedPointers) {
		t.Fatalf("bad: \nexpected        : %#v\nexpectedPointers: %#v\nresult          : %#v", expected, expectedPointers, target)
	}
}

Produces:

=== RUN   TestDecode_structToMap_PointersConsistency
    mapstructure_test.go:2941: bad: 
        expected        : map[string]interface {}{"child-ptr":map[string]interface {}{"string":"hello"}, "string-ptr":"goodbye"}
        expectedPointers: map[string]interface {}{"child-ptr":(*map[string]interface {})(0xc0000a8058), "string-ptr":(*string)(0xc00008eec0)}
        result          : map[string]interface {}{"child-ptr":map[string]interface {}{"string":"hello"}, "string-ptr":(*string)(0xc00008ede0)}
--- FAIL: TestDecode_structToMap_PointersConsistency (0.00s)

mapstructure version bf980b3
go version go1.18.7 linux/amd64

I can't see a reason why structures and base types should behave differently. I would expect to either keep the pointers or remove them. Can you please explain to me if this is a design choice or not? My guess is that struct pointers where not supported before PR #271 which made it dereference struct pointers systematically.

Discussion

Should all pointers be represented in the target map or none?

From the perspective of my use case, the pointer representation in the map is not necessary.

Here are the benefits of the pointers usage in this context:

  • Data representation : allows the representation of "no information". E.g. a nul pointer is different from an empty string.
    -> nul pointer can be represented as an unset key
  • Memory and speed optimization : redundent data structures can be referenced and large data structure can be referenced so that they are not copied on return
    -> this can be usefull when the decoded structure contains big data that should not be copied

For these reasons, I would suggest to:

  • Handle pointers of any type in the same way (e.g. struct pointers and string pointers)
  • Create a global option tho choose whether to keep or not the pointer representation in the target map (in the other direction, it's the type of the struct data that defines if it's a pointer or not). Defaults to "remove pointers". Name suggestion : "PreserveReferences"(true/false)
  • For retro-compatibility, add an option to keep the pointer representation on non-struct types. Eventually set this by default until next major. Name suggestion : "LegacyReferencesHandling" (true/false)
  • Add a property tag option to keep pointer representation and not copy data but only the pointer value (used for optimiazation in case of a large data structure, for example a string, that should be referenced in the map instead of copied). Name suggestion: "nocopy"

As an alternative, the options could be merge in a "ReferencesHandling" option with values legacy/preserve/discard.

Please let me know what you think and if there are any thechnical constraints that would not allow this. I would be happy to contribute.

@wuxq
Copy link

wuxq commented Jul 12, 2023

I think this is very useful. I'm Looking forward to it.

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