SERVER-19272 ActionLogType should do its validation and handle setting balancing details

4 files changed, 155 insertions, 276 deletions

diff --git a/src/mongo/s/balance.cpp b/src/mongo/s/balance.cpp
index e172b588afc..4086a13cb76 100644
--- a/src/mongo/s/balance.cpp
+++ b/src/mongo/s/balance.cpp
@@ -208,46 +208,6 @@ void Balancer::_ping(bool waiting) {
         NULL);
 }
 
-/*
-* Builds the details object for the actionlog.
-* Current formats for detail are:
-* Success: {
-*           "candidateChunks" : ,
-*           "chunksMoved" : ,
-*           "executionTimeMillis" : ,
-*           "errorOccured" : false
-*          }
-* Failure: {
-*           "executionTimeMillis" : ,
-*           "errmsg" : ,
-*           "errorOccured" : true
-*          }
-* @param didError, did this round end in an error?
-* @param executionTime, the time this round took to run
-* @param candidateChunks, the number of chunks identified to be moved
-* @param chunksMoved, the number of chunks moved
-* @param errmsg, the error message for this round
-*/
-
-static BSONObj _buildDetails(bool didError,
-                             int executionTime,
-                             int candidateChunks,
-                             int chunksMoved,
-                             const std::string& errmsg) {
-    BSONObjBuilder builder;
-    builder.append("executionTimeMillis", executionTime);
-    builder.append("errorOccured", didError);
-
-    if (didError) {
-        builder.append("errmsg", errmsg);
-    } else {
-        builder.append("candidateChunks", candidateChunks);
-        builder.append("chunksMoved", chunksMoved);
-    }
-
-    return builder.obj();
-}
-
 bool Balancer::_checkOIDs() {
     vector<ShardId> all;
     grid.shardRegistry()->getAllShardIds(&all);
@@ -597,11 +557,10 @@ void Balancer::run() {
                         _moveChunks(candidateChunks, writeConcern.get(), waitForDelete);
                 }
 
-                actionLog.setDetails(_buildDetails(false,
-                                                   balanceRoundTimer.millis(),
-                                                   static_cast<int>(candidateChunks.size()),
-                                                   _balancedLastTime,
-                                                   ""));
+                actionLog.setDetails(boost::none,
+                                     balanceRoundTimer.millis(),
+                                     static_cast<int>(candidateChunks.size()),
+                                     _balancedLastTime);
                 actionLog.setTime(jsTime());
 
                 grid.catalogManager()->logAction(actionLog);
@@ -620,7 +579,7 @@ void Balancer::run() {
             LOG(1) << "*** End of balancing round";
 
             // This round failed, tell the world!
-            actionLog.setDetails(_buildDetails(true, balanceRoundTimer.millis(), 0, 0, e.what()));
+            actionLog.setDetails(string(e.what()), balanceRoundTimer.millis(), 0, 0);
             actionLog.setTime(jsTime());
 
             grid.catalogManager()->logAction(actionLog);
diff --git a/src/mongo/s/catalog/replset/catalog_manager_replica_set_log_action_test.cpp b/src/mongo/s/catalog/replset/catalog_manager_replica_set_log_action_test.cpp
index 77e3e0ada10..87ed6feed18 100644
--- a/src/mongo/s/catalog/replset/catalog_manager_replica_set_log_action_test.cpp
+++ b/src/mongo/s/catalog/replset/catalog_manager_replica_set_log_action_test.cpp
@@ -78,8 +78,9 @@ public:
             ASSERT_EQUALS(1U, inserts.size());
             BSONObj insert = inserts.front();
 
-            ActionLogType actualActionLog;
-            ASSERT_TRUE(actualActionLog.parseBSON(insert, &errmsg));
+            auto actualActionLogRes = ActionLogType::fromBSON(insert);
+            ASSERT_OK(actualActionLogRes.getStatus());
+            const ActionLogType& actualActionLog = actualActionLogRes.getValue();
 
             ASSERT_EQUALS(expectedActionLog.toBSON(), actualActionLog.toBSON());
 
@@ -98,6 +99,7 @@ TEST_F(LogActionTest, LogActionNoRetryAfterSuccessfulCreate) {
     expectedActionLog.setServer("server1");
     expectedActionLog.setTime(network()->now());
     expectedActionLog.setWhat("moved a chunk");
+    expectedActionLog.setDetails(boost::none, 0, 1, 1);
 
     auto future =
         launchAsync([this, &expectedActionLog] { catalogManager()->logAction(expectedActionLog); });
@@ -125,6 +127,7 @@ TEST_F(LogActionTest, LogActionNoRetryCreateIfAlreadyExists) {
     expectedActionLog.setServer("server1");
     expectedActionLog.setTime(network()->now());
     expectedActionLog.setWhat("moved a chunk");
+    expectedActionLog.setDetails(boost::none, 0, 1, 1);
 
     auto future =
         launchAsync([this, &expectedActionLog] { catalogManager()->logAction(expectedActionLog); });
@@ -155,6 +158,7 @@ TEST_F(LogActionTest, LogActionCreateFailure) {
     expectedActionLog.setServer("server1");
     expectedActionLog.setTime(network()->now());
     expectedActionLog.setWhat("moved a chunk");
+    expectedActionLog.setDetails(boost::none, 0, 1, 1);
 
     auto future =
         launchAsync([this, &expectedActionLog] { catalogManager()->logAction(expectedActionLog); });
diff --git a/src/mongo/s/catalog/type_actionlog.cpp b/src/mongo/s/catalog/type_actionlog.cpp
index 1cefd2774dc..b4f6db43fc1 100644
--- a/src/mongo/s/catalog/type_actionlog.cpp
+++ b/src/mongo/s/catalog/type_actionlog.cpp
@@ -25,17 +25,20 @@
  *    delete this exception statement from all source files in the program,
  *    then also delete it in the license file.
  */
+
+#include "mongo/platform/basic.h"
+
 #include "mongo/s/catalog/type_actionlog.h"
 
-#include "mongo/db/field_parser.h"
+#include "mongo/base/status_with.h"
+#include "mongo/bson/bsonobj.h"
+#include "mongo/bson/bsonobjbuilder.h"
+#include "mongo/bson/util/bson_extract.h"
+#include "mongo/util/assert_util.h"
 #include "mongo/util/mongoutils/str.h"
 
 namespace mongo {
 
-using std::string;
-
-using mongo::str::stream;
-
 const std::string ActionLogType::ConfigNS = "config.actionlog";
 
 const BSONField<std::string> ActionLogType::server("server");
@@ -43,118 +46,114 @@ const BSONField<std::string> ActionLogType::what("what");
 const BSONField<Date_t> ActionLogType::time("time");
 const BSONField<BSONObj> ActionLogType::details("details");
 
-ActionLogType::ActionLogType() {
-    clear();
-}
+StatusWith<ActionLogType> ActionLogType::fromBSON(const BSONObj& source) {
+    ActionLogType actionLog;
+
+    {
+        std::string actionLogServer;
+        Status status = bsonExtractStringField(source, server.name(), &actionLogServer);
+        if (!status.isOK())
+            return status;
+        actionLog._server = actionLogServer;
+    }
+
+    {
+        BSONElement actionLogTimeElem;
+        Status status = bsonExtractTypedField(source, time.name(), Date, &actionLogTimeElem);
+        if (!status.isOK())
+            return status;
+        actionLog._time = actionLogTimeElem.date();
+    }
 
-ActionLogType::~ActionLogType() {}
+    {
+        std::string actionLogWhat;
+        Status status = bsonExtractStringField(source, what.name(), &actionLogWhat);
+        if (!status.isOK())
+            return status;
+        actionLog._what = actionLogWhat;
+    }
 
-bool ActionLogType::isValid(std::string* errMsg) const {
-    std::string dummy;
-    if (errMsg == NULL) {
-        errMsg = &dummy;
+    {
+        BSONElement actionLogDetailsElem;
+        Status status =
+            bsonExtractTypedField(source, details.name(), Object, &actionLogDetailsElem);
+        if (!status.isOK())
+            return status;
+        actionLog._details = actionLogDetailsElem.Obj().getOwned();
     }
 
-    // All the mandatory fields must be present.
-    if (!_isServerSet) {
-        *errMsg = stream() << "missing " << server.name() << " field";
-        return false;
+    return actionLog;
+}
+
+Status ActionLogType::validate() const {
+    if (!_server.is_initialized() || _server->empty()) {
+        return {ErrorCodes::NoSuchKey, str::stream() << "missing " << server.name() << " field"};
     }
-    if (!_isTimeSet) {
-        *errMsg = stream() << "missing " << time.name() << " field";
-        return false;
+
+    if (!_what.is_initialized() || _what->empty()) {
+        return {ErrorCodes::NoSuchKey, str::stream() << "missing " << what.name() << " field"};
     }
-    if (!_isWhatSet) {
-        *errMsg = stream() << "missing " << what.name() << " field";
-        return false;
+
+    if (!_time.is_initialized()) {
+        return {ErrorCodes::NoSuchKey, str::stream() << "missing " << time.name() << " field"};
     }
-    if (!_isDetailsSet) {
-        *errMsg = stream() << "missing " << details.name() << " field";
-        return false;
+
+    if (!_details.is_initialized() || _details->isEmpty()) {
+        return {ErrorCodes::NoSuchKey, str::stream() << "missing " << details.name() << " field"};
     }
 
-    return true;
+    return Status::OK();
 }
 
 BSONObj ActionLogType::toBSON() const {
     BSONObjBuilder builder;
 
-    if (_isServerSet)
-        builder.append(server(), _server);
-    if (_isTimeSet)
-        builder.append(time(), _time);
-    if (_isWhatSet)
-        builder.append(what(), _what);
-    if (_isDetailsSet)
-        builder.append(details(), _details);
+    if (_server)
+        builder.append(server.name(), getServer());
+    if (_what)
+        builder.append(what.name(), getWhat());
+    if (_time)
+        builder.append(time.name(), getTime());
+    if (_details)
+        builder.append(details.name(), _details.get());
 
     return builder.obj();
 }
 
-bool ActionLogType::parseBSON(const BSONObj& source, string* errMsg) {
-    clear();
-
-    std::string dummy;
-    if (!errMsg)
-        errMsg = &dummy;
-
-    FieldParser::FieldState fieldState;
-
-    fieldState = FieldParser::extract(source, server, &_server, errMsg);
-    if (fieldState == FieldParser::FIELD_INVALID)
-        return false;
-    _isServerSet = fieldState == FieldParser::FIELD_SET;
-
-    fieldState = FieldParser::extract(source, time, &_time, errMsg);
-    if (fieldState == FieldParser::FIELD_INVALID)
-        return false;
-    _isTimeSet = fieldState == FieldParser::FIELD_SET;
-
-    fieldState = FieldParser::extract(source, what, &_what, errMsg);
-    if (fieldState == FieldParser::FIELD_INVALID)
-        return false;
-    _isWhatSet = fieldState == FieldParser::FIELD_SET;
-
-    fieldState = FieldParser::extract(source, details, &_details, errMsg);
-    if (fieldState == FieldParser::FIELD_INVALID)
-        return false;
-    _isDetailsSet = fieldState == FieldParser::FIELD_SET;
-
-    return true;
+std::string ActionLogType::toString() const {
+    return toBSON().toString();
 }
 
-void ActionLogType::clear() {
-    _server.clear();
-    _isServerSet = false;
-
-    _what.clear();
-    _isWhatSet = false;
-
-    _time = Date_t();
-    _isTimeSet = false;
-
-    _details = BSONObj();
-    _isDetailsSet = false;
+void ActionLogType::setServer(const std::string& server) {
+    invariant(!server.empty());
+    _server = server;
 }
 
-void ActionLogType::cloneTo(ActionLogType* other) const {
-    other->clear();
-
-    other->_server = _server;
-    other->_isServerSet = _isServerSet;
-
-    other->_what = _what;
-    other->_isWhatSet = _isWhatSet;
-
-    other->_time = _time;
-    other->_isTimeSet = _isTimeSet;
+void ActionLogType::setWhat(const std::string& what) {
+    invariant(!what.empty());
+    _what = what;
+}
 
-    other->_details = _details;
-    other->_isDetailsSet = _isDetailsSet;
+void ActionLogType::setTime(const Date_t& time) {
+    _time = time;
 }
 
-std::string ActionLogType::toString() const {
-    return toBSON().toString();
+void ActionLogType::setDetails(const boost::optional<std::string>& errMsg,
+                               int executionTime,
+                               int candidateChunks,
+                               int chunksMoved) {
+    BSONObjBuilder builder;
+    builder.append("executionTimeMillis", executionTime);
+    builder.append("errorOccured", errMsg.is_initialized());
+
+    if (errMsg) {
+        builder.append("errmsg", errMsg.get());
+    } else {
+        builder.append("candidateChunks", candidateChunks);
+        builder.append("chunksMoved", chunksMoved);
+    }
+
+    _details = builder.obj();
 }
 
 }  // namespace mongo
diff --git a/src/mongo/s/catalog/type_actionlog.h b/src/mongo/s/catalog/type_actionlog.h
index b7ff387a823..d13f21466da 100644
--- a/src/mongo/s/catalog/type_actionlog.h
+++ b/src/mongo/s/catalog/type_actionlog.h
@@ -28,11 +28,11 @@
 
 #pragma once
 
+#include <boost/optional.hpp>
 #include <string>
 
-#include "mongo/base/disallow_copying.h"
-#include "mongo/base/string_data.h"
 #include "mongo/db/jsobj.h"
+#include "mongo/util/time_support.h"
 
 namespace mongo {
 
@@ -40,31 +40,9 @@ namespace mongo {
  * This class represents the layout and contents of documents contained in the
  * config.actionlog collection. All manipulation of documents coming from that
  * collection should be done with this class.
- *
- * Usage Example:
- *
- *     // Contact the config. 'conn' has been obtained before.
- *     DBClientBase* conn;
- *     BSONObj query = QUERY(ActionLogType::exampleField("exampleFieldName"));
- *     exampleDoc = conn->findOne(ActionLogType::ConfigNS, query);
- *
- *     // Process the response.
- *     ActionLogType exampleType;
- *     std::string errMsg;
- *     if (!exampleType.parseBSON(exampleDoc, &errMsg) || !exampleType.isValid(&errMsg)) {
- *         // Can't use 'exampleType'. Take action.
- *     }
- *     // use 'exampleType'
- *
  */
 class ActionLogType {
-    MONGO_DISALLOW_COPYING(ActionLogType);
-
 public:
-    //
-    // schema declarations
-    //
-
     // Name of the actionlog collection in the config server.
     static const std::string ConfigNS;
 
@@ -81,139 +59,78 @@ public:
     static const BSONField<long long> executionTimeMicros;
     static const BSONField<std::string> errmsg;
 
-
-    //
-    // actionlog type methods
-    //
-
-    ActionLogType();
-    ~ActionLogType();
-
-    /**
-     * Returns true if all the mandatory fields are present and have valid
-     * representations. Otherwise returns false and fills in the optional 'errMsg' string.
-     */
-    bool isValid(std::string* errMsg) const;
-
-    /**
-     * Returns the BSON representation of the entry.
-     */
-    BSONObj toBSON() const;
-
-    void buildDetails();
-
     /**
-     * Clears and populates the internal state using the 'source' BSON object if the
-     * latter contains valid values. Otherwise sets errMsg and returns false.
+     * Constructs a new ActionLogType object from BSON.
+     * Also does validation of the contents.
      */
-    bool parseBSON(const BSONObj& source, std::string* errMsg);
+    static StatusWith<ActionLogType> fromBSON(const BSONObj& source);
 
     /**
-     * Clears the internal state.
+     * Returns OK if all fiels have been set. Otherwise, returns NoSuchKey
+     * and information about the first field that is missing.
      */
-    void clear();
+    Status validate() const;
 
     /**
-     * Copies all the fields present in 'this' to 'other'.
+     * Returns the BSON representation of the entry.
      */
-    void cloneTo(ActionLogType* other) const;
+    BSONObj toBSON() const;
 
     /**
      * Returns a std::string representation of the current internal state.
      */
     std::string toString() const;
 
-    //
-    // individual field accessors
-    //
-
-    void setServer(StringData server) {
-        _server = server.toString();
-        _isServerSet = true;
-    }
-
-    void unsetServer() {
-        _isServerSet = false;
-    }
-
-    bool isServerSet() const {
-        return _isServerSet;
-    }
-
-    // Calling get*() methods when the member is not set results in undefined behavior
     const std::string& getServer() const {
-        dassert(_isServerSet);
-        return _what;
-    }
-
-    void setWhat(StringData what) {
-        _what = what.toString();
-        _isWhatSet = true;
-    }
-
-    void unsetWhat() {
-        _isWhatSet = false;
-    }
-
-    bool isWhatSet() const {
-        return _isWhatSet;
+        return _server.get();
     }
+    void setServer(const std::string& server);
 
-    // Calling get*() methods when the member is not set results in undefined behavior
     const std::string& getWhat() const {
-        dassert(_isWhatSet);
-        return _what;
-    }
-
-    void setTime(const Date_t time) {
-        _time = time;
-        _isTimeSet = true;
-    }
-
-    void unsetTime() {
-        _isTimeSet = false;
-    }
-
-    bool isTimeSet() const {
-        return _isTimeSet;
-    }
-
-    // Calling get*() methods when the member is not set results in undefined behavior
-    const Date_t getTime() const {
-        dassert(_isTimeSet);
-        return _time;
-    }
-
-    void setDetails(const BSONObj& details) {
-        _details = details.getOwned();
-        _isDetailsSet = true;
-    }
-
-    void unsetDetails() {
-        _isDetailsSet = false;
-    }
-
-    bool isDetailsSet() const {
-        return _isDetailsSet;
-    }
-
-    // Calling get*() methods when the member is not set results in undefined behavior
-    const BSONObj getDetails() const {
-        dassert(_isDetailsSet);
-        return _details;
-    }
+        return _what.get();
+    }
+    void setWhat(const std::string& what);
+
+    const Date_t& getTime() const {
+        return _time.get();
+    }
+    void setTime(const Date_t& time);
+
+    /*
+     * Builds the details object for the actionlog.
+     * Current formats for detail are:
+     * Success: {
+     *           "candidateChunks" : ,
+     *           "chunksMoved" : ,
+     *           "executionTimeMillis" : ,
+     *           "errorOccured" : false
+     *          }
+     * Failure: {
+     *           "executionTimeMillis" : ,
+     *           "errmsg" : ,
+     *           "errorOccured" : true
+     *          }
+     * @param errMsg: set if a balancer round resulted in an error
+     * @param executionTime: the time this round took to run
+     * @param candidateChunks: the number of chunks identified to be moved
+     * @param chunksMoved: the number of chunks moved
+     */
+    void setDetails(const boost::optional<std::string>& errMsg,
+                    int executionTime,
+                    int candidateChunks,
+                    int chunksMoved);
 
 private:
     // Convention: (M)andatory, (O)ptional, (S)pecial rule.
-    std::string _server;  // (M)  hostname of server that we are making the change on.
-                          // Does not include port.
-    bool _isServerSet;
-    std::string _what;  // (M)  what the action being performed was.
-    bool _isWhatSet;
-    Date_t _time;  // (M)  time this change was made
-    bool _isTimeSet;
-    BSONObj _details;  // (M)  A BSONObj containing extra information about some operations
-    bool _isDetailsSet;
+
+    // (M) hostname of server that we are making the change on. Does not include the port.
+    boost::optional<std::string> _server;
+    // (M) what the action being performed is.
+    boost::optional<std::string> _what;
+    // (M) time this change was made.
+    boost::optional<Date_t> _time;
+    // (M) A BSON document containing extra information about some operations
+    boost::optional<BSONObj> _details;
 };
 
 }  // namespace mongo