[SERVER-34628] Remove appendCommandStatus() in favor of throwing exceptions Created: 23/Apr/18  Updated: 29/Oct/23  Resolved: 09/May/18

Status: Closed
Project: Core Server
Component/s: Internal Code
Affects Version/s: None
Fix Version/s: 4.0.0-rc0

Type: Improvement Priority: Major - P3
Reporter: Siyuan Zhou Assignee: Mathias Stearn
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Related
is related to SERVER-34679 Write concern errors are lost when co... Closed
Backwards Compatibility: Minor Change
Sprint: Platforms 2018-05-07, Platforms 2018-05-21
Participants:

 Description   

Now we have two ways to return an error to clients: appendCommandStatus() and throwing exceptions, e.g. uassertStatusOK(). It'll be great if throwing exceptions is the only way to do so.

SERVER-34256 appends error labels for certain error codes only in transaction. The information whether the user indicates running in a transaction is currently only available in service_entry_point_common.cpp, so appendCommandStatus() doesn't have enough information to append the label. We have to do that afterwards by inspecting the response.



 Comments   
Comment by Mathias Stearn [ 09/May/18 ]

While our tests showed that there was no change in the error replies we look at, it is possible that that this change causes some commands that accidentally left information in the result builder to no longer leak that (presumably incorrect) information any more.

Comment by Githook User [ 08/May/18 ]

Author:

{'email': 'mathias@10gen.com', 'name': 'Mathias Stearn', 'username': 'RedBeard0531'}

Message: SERVER-34628 Really remove appendCommandStatus

All remaining callers are transitioned to some form of usassert. This was done
with an elaborate set of vim macros to make this tractable. Therefore it
should not be considered an example of the best way to write new code, just as
an improvement on what was there before. In particular, I couldn't easily
remove Status's that are named then only used once in uassertStatusOK, nor
could I convert the pattern of checking a StatusWith<T>'s getStatus() then
calling getValue() to just call uassertStatusOK(returnsStatusWith()).
Branch: master
https://github.com/mongodb/mongo/commit/db41862c5380ab33cf28db99726cdac252df0872

Comment by Githook User [ 08/May/18 ]

Author:

{'email': 'mathias@10gen.com', 'name': 'Mathias Stearn', 'username': 'RedBeard0531'}

Message: SERVER-34628 Remove support for Status/StatusWith returns in TypedCommand
Branch: master
https://github.com/mongodb/mongo/commit/2d35461cb54e35afea223714fab1a184a9b381e2

Comment by Githook User [ 08/May/18 ]

Author:

{'email': 'mathias@10gen.com', 'name': 'Mathias Stearn', 'username': 'RedBeard0531'}

Message: SERVER-34628 Prep for removing appendCommandStatus

  • Added appendCommandStatusNoThrow matching the current aCS behavior
  • Make appendCommandStatus call uassertStatusOK then aCS on success
  • Make the few places that need to not throw call aCSNT

A following commit will completely remove appendCommandStatus. It is split out
because that commit is fairly huge.
Branch: master
https://github.com/mongodb/mongo/commit/589af3820b00ed0b7ac26a84cfeed6554ab191f3

Comment by Spencer Brody (Inactive) [ 24/Apr/18 ]

The key thing here is to have a single error-handling path where code that needs to run when the command failed can run, without having to remember to duplicate that code in two places - once in the exception handling block and once by manually inspecting the responseBuilder.

Comment by Kaloian Manassiev [ 24/Apr/18 ]

From what I understand, appendCommandStatus just appends the error to an already populated response buffer, whereas uassert will reset it and only populate it with the assertion error. Thus removing appendCommandStatus removes the ability to do the former, but perhaps with the new means to specify serialization format for an exception's data, if there is a need to attach side information to an exception we can use that mechanism.

CC redbeard0531

Generated at Thu Feb 08 04:37:19 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.