Support MongoDB session-wide write concern #3646
Conversation
| } | ||
|
|
||
| var concern writeConcern | ||
| err = json.Unmarshal([]byte(input), &concern) |
There was a problem hiding this comment.
This might be better to do in the Initialize function and cache the writeConcern object in the struct. Will save on marshaling it on every call.
There was a problem hiding this comment.
This only gets through if Connection() creates a new session, otherwise it will return pretty early up top. Since other session settings were set here, I thought calling SetSafe here as well.
There was a problem hiding this comment.
It's also probably better to return any unmarshaling errors during Init too.
There was a problem hiding this comment.
I'd say decode and unmarshal in Init, store the mgo.Safe pointer in the struct and call SetSafe in Connection()
plugins/database/mongodb/util.go
Outdated
| @@ -17,6 +17,16 @@ type mongoDBStatement struct { | |||
| Roles mongodbRoles `json:"roles"` | |||
| } | |||
|
|
|||
| // writeConcern is the mgo.Safe struct with JSON tags | |||
| // More info: https://godoc.org/gopkg.in/mgo.v2#Safe | |||
| type writeConcern struct { | |||
There was a problem hiding this comment.
Do we need this struct or can we just use the mgo.Safe version?
There was a problem hiding this comment.
The mgo.Safe struct doesn't have json tags for the fields :(
There was a problem hiding this comment.
Actually, we might not need the JSON tags if there's no snake-casing on the fields so this could probably go away.
There was a problem hiding this comment.
Okay, in that case i'd say it's fine to leave this struct as is
There was a problem hiding this comment.
We agreed offline that this is okay to remove since we are following mongoDB's naming convention (e.g. wtimeout is not snake-cased). In this case, we can use mgo.Safe's struct directly.
| // Only set c.safe if anything got marshaled into concern. We don't want to | ||
| // pass an empty, non-nil mgo.Safe object into mgo.SetSafe in Connection(). | ||
| if (mgo.Safe{} != *concern) { | ||
| c.safe = concern |
There was a problem hiding this comment.
Perhaps this should be an error case? If they set the WriteConcern JSON we should expect to have successfully marshaled something
|
Thanks for taking the time to review! |
* oss/master: changelog++ Support MongoDB session-wide write concern (#3646) Clarify api_addr related errors on VaultPluginTLSProvider (#3620) allowed/disallowed_policies as TypeCommaStringSlice (#3641) Update example payload and response for pem_keys field which needs \n after header and before footer in order to be accepted as a valid RSA or ECDSA public key (#3632) Docs: Update /sys/policies/ re: beta refs to address #3624 (#3629) Update secrets page Remove beta notice Expanding on the quick start guide with how to set up an intermediate authority (#3622) Docs: mlock() notes, fixes #3605 (#3614) Fix spelling (#3609) Add command to example to register plugin (#3601) update relatedtools, add Goldfish UI. (#3597) Fix docs for Transit API (#3588) Update cassandra docs with consistency value. Remove Trailing White space in Kubernetes Doc (#3360) Missing command for vault PUT operation (#3355) Update some rekey docs
Closes #3595
Add support for MongoDB's write concern for the mongodb database backend.
One thing to note is that this uses mgo's session-wide write concern, so if that's set when configuring the backend, the write concern applies to all statements.