[SERVER-19924] getLastError's n value now indicates the number of inserts completed. Created: 13/Aug/15 Updated: 20/Aug/15 Resolved: 20/Aug/15 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Write Ops |
| Affects Version/s: | 3.1.7 |
| Fix Version/s: | None |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | Craig Wilson | Assignee: | David Storch |
| Resolution: | Done | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||||||||||||||
| Operating System: | ALL | ||||||||||||||||||||||||
| Steps To Reproduce: | In 3.0:
In 3.1:
|
||||||||||||||||||||||||
| Participants: | |||||||||||||||||||||||||
| Description |
|
getLastError used to return 0 for "n" after an insert. With the latest 3.1.7 nightly, it is now returning 1 (or presumably, the number of documents inserted). I certainly think this is the right value for "n" if we're starting from scratch, but at this point we shouldn't change this behavior as users may be relying on this. |
| Comments |
| Comment by Craig Wilson [ 20/Aug/15 ] |
|
Thanks David. I don't think either one holds true. All my tests of current/older .NET driver versions all pass except for where I'm asserting the n value. I think this will probably hold true for other drivers as well. So, let's mark the change as Drivers Changes Needed and leave it in. |
| Comment by David Storch [ 19/Aug/15 ] |
|
craiggwilson, got it. I am inclined to close this as Works as Designed with Drivers Changes Needed. In general I think that we should fix bugs on the server, even if they amount to a behavior change in a publicly documented API. The exception to this would be if it any of the following are true:
Do either of these hold true? Of course the change will be backwards breaking if a user calls getLastError directly, but it sounds like it probably won't change from the application's perspective for getLastError invoked by the driver? Are the required drivers changes going to be difficult? |
| Comment by Craig Wilson [ 19/Aug/15 ] |
|
All I know is that this is a change in behavior to a publicly documented API. I have no idea who might be relying on this, only that the output will be different. 1. It only applies to getLastError, which is all but deprecated already due to the introduction of the write commands. Why change something that we are effectively not using? All in all, like I said in the description, I think this is the correct value for n. If you decide you want it fixed, then make sure to mark this as Drivers Changes Needed so all drivers can evaluate their codebases and make the appropriate changes. |
| Comment by David Storch [ 19/Aug/15 ] |
|
craiggwilson, my reading of the commit that introduced this behavior change is that this was an explicit bug fix packaged with some other getLastError refactoring. In particular, the old implementation recorded the number of inserts here: This sets nObjects to the number of documents inserted, but fails to mark the LastError instance as "valid". The downstream effect is that the number of inserts are misreported in the getLastError command response. What's your argument for why we should revert this bug fix? Who might be depending on this behavior in a way that would break after upgrade to 3.2? |
| Comment by David Storch [ 19/Aug/15 ] |
|
This behavior change was introduced by commit da31be34dc66 under CC schwerin. |