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

Have Update() and Remove() return ChangeInfo #294

Open
wants to merge 3 commits into
base: development
Choose a base branch
from

Conversation

kahuang
Copy link

@kahuang kahuang commented Oct 30, 2018

As these are updating documents, it is important that we return what was actually updated

@kahuang kahuang force-pushed the changeinfo branch 2 times, most recently from 8a36955 to 6ae1285 Compare October 30, 2018 23:38
@maitesin
Copy link

maitesin commented Nov 1, 2018

Hi @kahuang,

Thanks for your contribution. Can you explain why you want to return the updated document? If there was no error the operation succeed and the changes have been applied. Therefore, the document contained in the database is the same as the one you used to make the change.

@kahuang
Copy link
Author

kahuang commented Nov 1, 2018

Thanks for your contribution. Can you explain why you want to return the updated document? If there was no error the operation succeed and the changes have been applied. Therefore, the document contained in the database is the same as the one you used to make the change.

The operation can succeed, but nothing may have been changed/modified. This is why mongo will return changeinfo so that the caller can determine if in fact something has been modified or not, rather than conflating a successful call with a successful modification.

See https://docs.mongodb.com/manual/reference/method/db.collection.update/#writeresults-update as a reference for updates (a similar construct applies for removes).

@eminano
Copy link

eminano commented Nov 6, 2018

Hi @kahuang,

Thanks for your PR! There are quite a few build errors due to your changes, could you please have a look at those?

Also, please rebase your changes to target development branch instead of master (applies to all your PRs) - as per contributing guidelines.

Thanks :)
Esther

session.go Outdated
@@ -3024,17 +3024,20 @@ func (c *Collection) Update(selector interface{}, update interface{}) error {
}
lerr, err := c.writeOp(&op, true)
if err == nil && lerr != nil && !lerr.UpdatedExisting {
return ErrNotFound
return &ChangeInfo{}, ErrNotFound
Copy link

Choose a reason for hiding this comment

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

You can return the info from the named return, and it'll be a nil ChangeInfo in this case instead of an empty one (which makes more sense in case of error). It's also consistent with what's done for the Upsert.

session.go Show resolved Hide resolved
session.go Outdated
}
return err
if err == nil && lerr != nil {
info = &ChangeInfo{Updated: lerr.modified, Matched: lerr.N}
Copy link

Choose a reason for hiding this comment

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

For the remove I think you need to set the Removed field instead.
https://docs.mongodb.com/manual/reference/method/db.collection.remove/#writeresult

@domodwyer
Copy link

Hey @kahuang

Thanks for the PR (super useful addition) however this would be a (very) breaking change for end users, which we try to avoid for compatibility with all the existing projects relying on mgo that haven't necessarily vendored - hopefully this situation will improve as dependency tools become the norm for Go.

We would be really interested in having the ability to access the ChangeInfo in mgo though, perhaps you could add a type similar to the following:

type WithChangeInfo struct{
    // whatever is needed here
}

// With a constructor method on the Collection
func (c *Collection) WithChangeInfo() *WithChangeInfo {
    // etc
}

// And WithChangeInfo implements the methods you want to include the *ChangeInfo on
func (c *WithChangeInfo) Update(selector interface{}, update interface{}) (info *ChangeInfo, err error) {
    // Do an update here
}

That way existing users are unaffected but can opt in to your ChangeInfo additions pretty easily:

info, err := coll.WithChangeInfo().Update(selectors, updated)

Dom

@kahuang
Copy link
Author

kahuang commented Nov 9, 2018

These are great points. This diff was originally for a private fork before we had introduced any clients, so there was no risk of backward incompatibility issues

What I've done here instead of breaking the API is to introduce two new methods, UpdateWithChangeInfo and RemoveWithChangeInfo

The reasoning here is that if we create these methods on the same struct, then we can reuse them for Update/Remove and just throw away the changeinfo and should make this easier to maintain moving forward.

I've also added some additional unit tests to check that we're setting ChangeInfo correctly

@kahuang kahuang force-pushed the changeinfo branch 2 times, most recently from 8c32159 to 775b654 Compare November 10, 2018 00:27
@eminano
Copy link

eminano commented Nov 20, 2018

Hi @kahuang,

Thanks for your changes, it's really appreciated!

I see that you're returning the ChangeInfo for Update and Remove but there are other methods (Insert, InsertId, UpdateId, RemoveId) that still don't have it. While I am aware the code is quite messy, with only half of the methods returning ChangeInfo, I think your effort to correct that should go all the way so we can have a uniform collection interface where all methods can return the ChangeInfo (as per spec).

I believe that's why @domodwyer suggested the separate implementation, to try and keep all of the new ChangeInfo methods contained.

I hope it's not too much to ask, but it would mean having a complete solution to the problem instead of a partial one.

Thanks in advance,
Esther

@kahuang
Copy link
Author

kahuang commented Nov 26, 2018

IMO, the methods are just wrong if they aren't returning a ChangeInfo (or something that contains that info) and we shouldn't allow that as an option in the API

After looking more closely at the code, it seems that we can have the best of both worlds if we return a LastError always, which contains the info we care about

(Pasting for context):

type LastError struct {
	Err             string
	Code, N, Waited int
	FSyncFiles      int `bson:"fsyncFiles"`
	WTimeout        bool
	UpdatedExisting bool        `bson:"updatedExisting"`
	UpsertedId      interface{} `bson:"upserted"`

	modified int
	ecases   []BulkErrorCase
}

I think the only modification we'd have to make is to allow modified and ecases to be public instead of private. Then the API spec remains unchanged, but we can get all the info we need from the LastError.

This approach allows us to maintain backwards compatibility of the API, add no new duplicate methods which improves maintainability, while also getting the information we need about the status of the writes. Are there any objections to this approach?

@domodwyer
Copy link

IMO, the methods are just wrong if they aren't returning a ChangeInfo (or something that contains that info) and we shouldn't allow that as an option in the API

That's fine in principle, but there are thousands of users who would probably value a non-breaking API change over returning a ChangeInfo they may or may not then use.

This is a community fork of the original mgo - we're stuck with some design decisions, we try and keep everyone happy as best we can by making non-breaking, additive changes.

After looking more closely at the code, it seems that we can have the best of both worlds if we return a LastError always

...snip...

This approach allows us to maintain backwards compatibility of the API, add no new duplicate methods which improves maintainability, while also getting the information we need about the status of the writes. Are there any objections to this approach?

I may have missed something, but surely always returning an error would be a significantly breaking change?

if err := collection.Update(selectors, docs); err != nil {
    log.Fatal(err)
}

Wouldn't the above explode if the Update() then began returning a LastError for every call?

@kahuang
Copy link
Author

kahuang commented Nov 26, 2018

IMO, the methods are just wrong if they aren't returning a ChangeInfo (or something that contains that info) and we shouldn't allow that as an option in the API

That's fine in principle, but there are thousands of users who would probably value a non-breaking API change over returning a ChangeInfo they may or may not then use.

This is a community fork of the original mgo - we're stuck with some design decisions, we try and keep everyone happy as best we can by making non-breaking, additive changes.

To expand on my comment, the reason I consider it wrong is that returning no ChangeInfo implies that the changes always succeed, unless there is some kind of error. @maitesin's clarifying question earlier in this thread is a prime example of that confusion.

If non-breaking API changes is a requirement, then I'll move forward with the WithChangeInfo() route that you suggested.

After looking more closely at the code, it seems that we can have the best of both worlds if we return a LastError always
...snip...
This approach allows us to maintain backwards compatibility of the API, add no new duplicate methods which improves maintainability, while also getting the information we need about the status of the writes. Are there any objections to this approach?

I may have missed something, but surely always returning an error would be a significantly breaking change?

if err := collection.Update(selectors, docs); err != nil {
    log.Fatal(err)
}

Wouldn't the above explode if the Update() then began returning a LastError for every call?

Ah, you're right. Forgive me, Go is not second nature for me yet.

@eminano
Copy link

eminano commented Jan 16, 2019

Hi @kahuang ,

How are you getting along with the changes we requested on the last review? If you don't have the time to implement it at the moment, we can always close this, and you can reopen it whenever it's more convenient.

Esther

@arichiardi
Copy link

Stumbled across the need for this one...thanks for the work on it.

This is great because you don't have to use two different db calls for fetching the information that actually changed after the update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants