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

Unable to set Public flag to false #463

Closed
mfzl opened this issue May 20, 2017 · 6 comments
Closed

Unable to set Public flag to false #463

mfzl opened this issue May 20, 2017 · 6 comments

Comments

@mfzl
Copy link
Contributor

mfzl commented May 20, 2017

If a client is created with Public flag set to true, client managers are not able to update the flag to false. This is because of how mergo works.

package main

import (
	"fmt"

	"github.com/imdario/mergo"
)

type Client struct {
	Public bool
}

func main() {

	// Client with updated information,
	// wanting to set Public flag to false
	c := &Client{
		Public: false,
	}

	// Client in database with Public flag set to true
	o := &Client{
		Public: true,
	}

	// "Mergo is intended to assign only zero value fields on destination with source value."
	// mergo.Merge(dst, src)
	mergo.Merge(c, o)
	// Because c (the destination) contains false it is considered a zero value
	// and gets replaced by the value of o (the source)

	fmt.Printf("c %+v\n", c)
	fmt.Printf("o %+v\n", o)
}
@mfzl
Copy link
Contributor Author

mfzl commented May 20, 2017

I can't think of a simple way to fix without resorting to *bool. Another way would be to inspect the JSON payload with any json query package and check if value was sent by the user. But then we would need to pass around the fact that Public flag was set or not.

@aeneasr
Copy link
Member

aeneasr commented May 23, 2017

Hm yeah, actually I think that the PUT command should have the full payload and not mergo at all. What do you think?

@mfzl
Copy link
Contributor Author

mfzl commented May 24, 2017

SGTM, that is more inline with the semantics of PUT. One downside would be backwards compatibility, I can't think of anything else.

@aeneasr
Copy link
Member

aeneasr commented May 24, 2017

I think that's ok, we'll add it to the release notes

@aeneasr
Copy link
Member

aeneasr commented Jun 2, 2017

@faxal would you mind creating a PR for that? Should be straight forward :)

@mfzl
Copy link
Contributor Author

mfzl commented Jun 2, 2017

Done #478

@aeneasr aeneasr closed this as completed Jun 4, 2017
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