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

Fix merging of interface types with concrete values #106

Merged
merged 1 commit into from
May 2, 2019
Merged

Fix merging of interface types with concrete values #106

merged 1 commit into from
May 2, 2019

Conversation

admtnnr
Copy link

@admtnnr admtnnr commented Mar 7, 2019

No description provided.

@vdemeester
Copy link
Collaborator

@admtnnr Thanks for the PR. Can you describe a bit more why is this needed (which use cases, …) ?

@admtnnr
Copy link
Author

admtnnr commented Mar 17, 2019

@vdemeester sure, here's the case I ran into:
Say we have an interface:

type Doer interface {
  Do() error
}

And we have two implementations of that interface:

type NoopDoer string
func (d NoopDoer) Do() error { return nil }

type RealDoer struct { ... }
func (d RealDoer) Do() error {
  ...
  return nil
}

And we have our config struct like so:

type config struct {
   Doer Doer 
}

And we set up a "default" config that we will merge in that uses the NoopDoer we defined above:

var defaultConfig = config{
  Doer: NoopDoer("this does nothing"),
}

What I've encountered is that the call to mergo.Merge fails if you have a setup where you pass in a config that also has a Doer defined that should take precedence, but is a different concrete type.
So something like this will return an error and panic where it shouldn't since the config should accept any implementation of a Doer:

var myConfig = config{
  Doer: RealDoer{},
}
if err := mergo.Merge(&myConfig, defaultConfig); err != nil {
   panic("THIS WILL GET RUN")
}

So what actually seems to happen is that mergo doesn't check that the types are actually assignable to the field in the struct, but rather just checks that the source and destination types are the same.

It skips through this case since the underlying types aren't the same: https://github.com/imdario/mergo/pull/106/files#diff-a103b5fee5fb31abebf12ee067c01da8R190 and instead hits this line: https://github.com/imdario/mergo/pull/106/files#diff-a103b5fee5fb31abebf12ee067c01da8R195

The test I added will fail if the check for AssignableTo is removed even though both types implement http.Handler.

Hopefully that helps explain it a little better. It's a weird case, I'll give you that. :)

@vdemeester
Copy link
Collaborator

So what actually seems to happen is that mergo doesn't check that the types are actually assignable to the field in the struct, but rather just checks that the source and destination types are the same.

@admtnnr indeed 😅

@vdemeester
Copy link
Collaborator

@admtnnr btw can you rebase against master to fix the CI ? 👼

@admtnnr admtnnr closed this Mar 30, 2019
@admtnnr admtnnr reopened this Mar 31, 2019
@admtnnr
Copy link
Author

admtnnr commented Mar 31, 2019

@vdemeester Rebased.

@admtnnr
Copy link
Author

admtnnr commented Apr 15, 2019

@vdemeester let me know if there's anything else you need from me to get this merged. :)

Copy link
Collaborator

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :tiger:

@vdemeester vdemeester merged commit 45df20b into darccio:master May 2, 2019
thaJeztah added a commit to thaJeztah/docker that referenced this pull request Oct 6, 2019
full diff: darccio/mergo@v0.3.7...v0.3.8

includes:

- darccio/mergo#112 Add strict override
    - fixes darccio/mergo#111 WithOverride should be able to check types
- darccio/mergo#106 Fix merging of interface types with concrete values
- darccio/mergo#120 should not overwrite pointers directly, instead check embedded values
    - fixes darccio/mergo#114 Embedded struct of pointer types will overwrite the whole destination struct
- darccio/mergo#125 added WithOverrideEmptySlice config flag

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
docker-jenkins pushed a commit to docker/docker-ce that referenced this pull request Oct 7, 2019
full diff: darccio/mergo@v0.3.7...v0.3.8

includes:

- darccio/mergo#112 Add strict override
    - fixes darccio/mergo#111 WithOverride should be able to check types
- darccio/mergo#106 Fix merging of interface types with concrete values
- darccio/mergo#120 should not overwrite pointers directly, instead check embedded values
    - fixes darccio/mergo#114 Embedded struct of pointer types will overwrite the whole destination struct
- darccio/mergo#125 added WithOverrideEmptySlice config flag

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Upstream-commit: 9bd1b1a8eca97a2403bc7a2ed9a52427d8c27078
Component: engine
burnMyDread pushed a commit to burnMyDread/moby that referenced this pull request Oct 21, 2019
full diff: darccio/mergo@v0.3.7...v0.3.8

includes:

- darccio/mergo#112 Add strict override
    - fixes darccio/mergo#111 WithOverride should be able to check types
- darccio/mergo#106 Fix merging of interface types with concrete values
- darccio/mergo#120 should not overwrite pointers directly, instead check embedded values
    - fixes darccio/mergo#114 Embedded struct of pointer types will overwrite the whole destination struct
- darccio/mergo#125 added WithOverrideEmptySlice config flag

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: zach <Zachary.Joyner@linux.com>
thaJeztah added a commit to thaJeztah/cli that referenced this pull request Oct 23, 2019
full diff: darccio/mergo@v0.3.7...v0.3.8

includes:

- darccio/mergo#112 Add strict override
    - fixes darccio/mergo#111 WithOverride should be able to check types
- darccio/mergo#106 Fix merging of interface types with concrete values
- darccio/mergo#120 should not overwrite pointers directly, instead check embedded values
    - fixes darccio/mergo#114 Embedded struct of pointer types will overwrite the whole destination struct
- darccio/mergo#125 added WithOverrideEmptySlice config flag

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah added a commit to thaJeztah/cli that referenced this pull request Oct 23, 2019
full diff: darccio/mergo@v0.3.7...v0.3.8

includes:

- darccio/mergo#112 Add strict override
    - fixes darccio/mergo#111 WithOverride should be able to check types
- darccio/mergo#106 Fix merging of interface types with concrete values
- darccio/mergo#120 should not overwrite pointers directly, instead check embedded values
    - fixes darccio/mergo#114 Embedded struct of pointer types will overwrite the whole destination struct
- darccio/mergo#125 added WithOverrideEmptySlice config flag

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
zappy-shu pushed a commit to zappy-shu/cli that referenced this pull request Jan 21, 2020
full diff: darccio/mergo@v0.3.7...v0.3.8

includes:

- darccio/mergo#112 Add strict override
    - fixes darccio/mergo#111 WithOverride should be able to check types
- darccio/mergo#106 Fix merging of interface types with concrete values
- darccio/mergo#120 should not overwrite pointers directly, instead check embedded values
    - fixes darccio/mergo#114 Embedded struct of pointer types will overwrite the whole destination struct
- darccio/mergo#125 added WithOverrideEmptySlice config flag

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
docker-jenkins pushed a commit to docker/docker-ce that referenced this pull request Jan 24, 2020
full diff: darccio/mergo@v0.3.7...v0.3.8

includes:

- darccio/mergo#112 Add strict override
    - fixes darccio/mergo#111 WithOverride should be able to check types
- darccio/mergo#106 Fix merging of interface types with concrete values
- darccio/mergo#120 should not overwrite pointers directly, instead check embedded values
    - fixes darccio/mergo#114 Embedded struct of pointer types will overwrite the whole destination struct
- darccio/mergo#125 added WithOverrideEmptySlice config flag

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Upstream-commit: 6cf7970cd397a77155aec077bd27755bc033b6f4
Component: cli
eiffel-fl pushed a commit to eiffel-fl/cli that referenced this pull request Jul 28, 2020
full diff: darccio/mergo@v0.3.7...v0.3.8

includes:

- darccio/mergo#112 Add strict override
    - fixes darccio/mergo#111 WithOverride should be able to check types
- darccio/mergo#106 Fix merging of interface types with concrete values
- darccio/mergo#120 should not overwrite pointers directly, instead check embedded values
    - fixes darccio/mergo#114 Embedded struct of pointer types will overwrite the whole destination struct
- darccio/mergo#125 added WithOverrideEmptySlice config flag

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
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

Successfully merging this pull request may close these issues.

None yet

2 participants