[GODRIVER-1621] options.Update().SetUpsert(true) being reset after each collection.updateOne Created: 20/May/20  Updated: 27/Oct/23  Resolved: 22/May/20

Status: Closed
Project: Go Driver
Component/s: CRUD
Affects Version/s: 1.1.4
Fix Version/s: None

Type: Bug Priority: Major - P3
Reporter: Andrew Hodel Assignee: Unassigned
Resolution: Works as Designed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified
Environment:

Linux 4.9.0-11-amd64



 Description   

If I setup an opts variable, like this:

 

```

opts := options.Update().SetUpsert(true)

```

 

Then I run collection.updateOne using that opts variable, then try to run another collection.UpdateOne using that opts variable it gives an error which does not describe that the problem is that the upsert option is not set to true and there is no existing document that matches the filter,.

 

If I just reset the opts variable after each collection.updateOne, like this:

 

```

opts := options.Update().SetUpsert(true)

```

 

Then it works as intended.  The error is ambiguous and I do not understand the reason that it would reset the options each time when the options are stored in a variable.



 Comments   
Comment by Divjot Arora (Inactive) [ 22/May/20 ]

Variadic is just a way for saying "give us any number of options instances".

Yes, the driver absolutely does not change the variable passed in as the options argument. We only take options as pointers rather than a value because some of the options structs are large and copying wouldn't be efficient.

Variable scope within an if-statement, or any other scope-defining block, can be weird. If you use ":=", you actually define a new variable which is only valid for that scope. This variable can have the same name as one from an outer scope, which can cause confusing issues. This is called shadowing. See https://yourbasic.org/golang/gotcha-shadowing-variables/ for an example.

Comment by Andrew Hodel [ 22/May/20 ]

So are you saying that variadic is just an incorrect way of describing arguments as default values should work just fine or is that some problem elsewhere?

More importantly are you just saying that the driver does not change the variable passed as the option argument and that Go still argues about pointers, variables and references in regards to the scope or access to change?

Or are you just saying variable scope within an if statement is wierd? LOL

Thank You,
Andrew Hodel

Comment by Divjot Arora (Inactive) [ 22/May/20 ]

The language doesn't have default function parameter values, so if the UpdateOne function didn't take a variadic number of parameters, you'd always have to specify nil even if you didn't want to give any options.

Going back to the original question in the description though, I don't think the discussion about variadics is relevant. Your original bug report was that you were constructing an UpdateOptions as

options.Update().SetUpsert(true)

and passing that to a Collection.UpdateOne call. After the call, the upsert value was not set to true anymore (I'm not sure if this means it was set to false or nil).

You later said that the issue was no longer happening, indicating that it was probably something else. I also provided a code sample demonstrating that the UpdateOne call does not change the options values you pass in. It constructs new ones, but does not change yours.

Given all of this, I think we can close out this issue as "works as designed". Do you agree?

Comment by Andrew Hodel [ 22/May/20 ]

Can you explain how Go does not have default options and what that has to do with a function argument?

My understanding if this was really an issue was that this driver, not Go had changed a variable.

Are you trying to say that Go cannot change a variable unless it is passed a pointer? That does not seem to be true with a variable .

Thank You,
Andrew Hodel

Comment by Divjot Arora (Inactive) [ 22/May/20 ]

> Why would we need to repeat options being changed by another call to the same function?

You shouldn't need to do. The CRUD function calls do not change the options structs you pass into them. The code sample I linked in my previous comment shows that a call to UpdateOne does not modify any of the values on the UpdateOptions passed to it.

> why would you need to merge update options when they are an argument to a function?

You might've noticed that the signatures for our CRUD functions are like

UpdateOne(filter interface{}, update interface{}, opts ...*options.UpdateOptions)

The opts parameter is variadic. This was done because Go doesn't have default options and we didn't want to require users to pass in an explicit{{nil}} if they didn't want to specify options. The downside is that this API technically allows you to pass in more than one instance of UpdateOptions. To get around this, we wrote a{{MergeUpdateOptions}} functions that can take any number of UpdateOptions and combine them into a new instance. This is done by creating a new instance and then setting its fields based on the ones passed to the functions by the user.

– Divjot

Comment by Andrew Hodel [ 22/May/20 ]

Why would we need to repeat options being changed by another call to the same function?

Or better, why would you need to merge update options when they are an argument to a function?

Are we going back through the argument of a function argument? If so how are we not trillionaires lol?

Thank You,
Andrew Hodel

Comment by Divjot Arora (Inactive) [ 22/May/20 ]

Hi andrewhodel@gmail.com,

I'd closed as "works as designed" because of your comment that it seemed to be working. This is our usual way of closing tickets where there doesn't seem to be a bug.

The flow for the UpdateOne/UpdateMany methods is that they call options.MergeUpdateOptions (https://github.com/mongodb/mongo-go-driver/blob/master/mongo/options/updateoptions.go#L75), which combines the variadic options.UpdateOptions instances passed to the method into a single instance. As you can see, the MergeUpdateOptions method doesn't alter any of its arguments and instead creates a new UpdateOptions instances to represent the merged options.

I've also written up a small gist to verify the behavior at https://github.com/mongodb/mongo-go-driver/blob/master/mongo/options/updateoptions.go#L75. After an UpdateOne/UpdateMany call, the Upsert vale in the options struct remains true in my test.

If you have any other questions about the method flow, feel free to comment here and I can re-open the ticket if necessary.

– Divjot

Comment by Andrew Hodel [ 22/May/20 ]

I don’t know about works as designed because you never commented on the flow and I never read the source code.

Thank You,
Andrew Hodel

Comment by Andrew Hodel [ 21/May/20 ]

Sure

On Thu, May 21, 2020 at 8:49 AM Divjot Arora (Jira) <jira@mongodb.org>

Comment by Divjot Arora (Inactive) [ 21/May/20 ]

Hi andrewhodel@gmail.com,

Can this ticket be closed? If not, can you provide a minimal code sample that reliably reproduces the issue?

– Divjot

Comment by Andrew Hodel [ 20/May/20 ]

Well, now that seems to be working.  Must have been another issue somewhere.

Comment by Andrew Hodel [ 20/May/20 ]

Bah, the reset is obviously without := and with = only.

Generated at Thu Feb 08 08:36:47 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.