SERVER-33173: Make format parameter optional for "$dateToString"

7 files changed, 153 insertions, 39 deletions

diff --git a/src/mongo/db/pipeline/expression.cpp b/src/mongo/db/pipeline/expression.cpp
index f679616e720..f0f6d4cd84f 100644
--- a/src/mongo/db/pipeline/expression.cpp
+++ b/src/mongo/db/pipeline/expression.cpp
@@ -1412,11 +1412,11 @@ Value ExpressionDateFromString::evaluate(const Document& root) const {
             }
 
             return Value(getExpressionContext()->timeZoneDatabase->fromString(
-                dateTimeString, timeZone, formatValue.getStringData()));
+                dateTimeString, timeZone.get(), formatValue.getStringData()));
         }
 
         return Value(
-            getExpressionContext()->timeZoneDatabase->fromString(dateTimeString, timeZone));
+            getExpressionContext()->timeZoneDatabase->fromString(dateTimeString, timeZone.get()));
     } catch (const ExceptionFor<ErrorCodes::ConversionFailure>&) {
         if (_onError) {
             return _onError->evaluate(root);
@@ -1622,20 +1622,11 @@ intrusive_ptr<Expression> ExpressionDateToString::parse(
         }
     }
 
-    uassert(18627, "Missing 'format' parameter to $dateToString", !formatElem.eoo());
     uassert(18628, "Missing 'date' parameter to $dateToString", !dateElem.eoo());
 
-    uassert(18533,
-            "The 'format' parameter to $dateToString must be a string literal",
-            formatElem.type() == BSONType::String);
-
-    const string format = formatElem.str();
-
-    TimeZone::validateToStringFormat(format);
-
     return new ExpressionDateToString(expCtx,
-                                      format,
                                       parseOperand(expCtx, dateElem, vps),
+                                      formatElem ? parseOperand(expCtx, formatElem, vps) : nullptr,
                                       timeZoneElem ? parseOperand(expCtx, timeZoneElem, vps)
                                                    : nullptr,
                                       onNullElem ? parseOperand(expCtx, onNullElem, vps) : nullptr);
@@ -1643,12 +1634,12 @@ intrusive_ptr<Expression> ExpressionDateToString::parse(
 
 ExpressionDateToString::ExpressionDateToString(
     const boost::intrusive_ptr<ExpressionContext>& expCtx,
-    const string& format,
     intrusive_ptr<Expression> date,
+    intrusive_ptr<Expression> format,
     intrusive_ptr<Expression> timeZone,
     intrusive_ptr<Expression> onNull)
     : Expression(expCtx),
-      _format(format),
+      _format(std::move(format)),
       _date(std::move(date)),
       _timeZone(std::move(timeZone)),
       _onNull(std::move(onNull)) {}
@@ -1663,7 +1654,11 @@ intrusive_ptr<Expression> ExpressionDateToString::optimize() {
         _onNull = _onNull->optimize();
     }
 
-    if (ExpressionConstant::allNullOrConstant({_date, _timeZone, _onNull})) {
+    if (_format) {
+        _format = _format->optimize();
+    }
+
+    if (ExpressionConstant::allNullOrConstant({_date, _format, _timeZone, _onNull})) {
         // Everything is a constant, so we can turn into a constant.
         return ExpressionConstant::create(getExpressionContext(), evaluate(Document{}));
     }
@@ -1674,25 +1669,53 @@ intrusive_ptr<Expression> ExpressionDateToString::optimize() {
 Value ExpressionDateToString::serialize(bool explain) const {
     return Value(
         Document{{"$dateToString",
-                  Document{{"format", _format},
-                           {"date", _date->serialize(explain)},
+                  Document{{"date", _date->serialize(explain)},
+                           {"format", _format ? _format->serialize(explain) : Value()},
                            {"timezone", _timeZone ? _timeZone->serialize(explain) : Value()},
                            {"onNull", _onNull ? _onNull->serialize(explain) : Value()}}}});
 }
 
 Value ExpressionDateToString::evaluate(const Document& root) const {
     const Value date = _date->evaluate(root);
+    Value formatValue;
+
+    // Eagerly validate the format parameter, ignoring if nullish since the input date nullish
+    // behavior takes precedence.
+    if (_format) {
+        formatValue = _format->evaluate(root);
+        if (!formatValue.nullish()) {
+            uassert(18533,
+                    str::stream() << "$dateToString requires that 'format' be a string, found: "
+                                  << typeName(formatValue.getType())
+                                  << " with value "
+                                  << formatValue.toString(),
+                    formatValue.getType() == BSONType::String);
+
+            TimeZone::validateToStringFormat(formatValue.getStringData());
+        }
+    }
+
+    // Evaluate the timezone parameter before checking for nullish input, as this will throw an
+    // exception for an invalid timezone string.
+    auto timeZone = makeTimeZone(getExpressionContext()->timeZoneDatabase, root, _timeZone.get());
 
     if (date.nullish()) {
         return _onNull ? _onNull->evaluate(root) : Value(BSONNULL);
     }
 
-    auto timeZone = makeTimeZone(getExpressionContext()->timeZoneDatabase, root, _timeZone.get());
     if (!timeZone) {
         return Value(BSONNULL);
     }
 
-    return Value(timeZone->formatDate(_format, date.coerceToDate()));
+    if (_format) {
+        if (formatValue.nullish()) {
+            return Value(BSONNULL);
+        }
+
+        return Value(timeZone->formatDate(formatValue.getStringData(), date.coerceToDate()));
+    }
+
+    return Value(timeZone->formatDate(Value::kISOFormatString, date.coerceToDate()));
 }
 
 void ExpressionDateToString::_doAddDependencies(DepsTracker* deps) const {
@@ -1704,6 +1727,10 @@ void ExpressionDateToString::_doAddDependencies(DepsTracker* deps) const {
     if (_onNull) {
         _onNull->addDependencies(deps);
     }
+
+    if (_format) {
+        _format->addDependencies(deps);
+    }
 }
 
 /* ----------------------- ExpressionDivide ---------------------------- */
diff --git a/src/mongo/db/pipeline/expression.h b/src/mongo/db/pipeline/expression.h
index b70457cc06f..6f3f7c6126d 100644
--- a/src/mongo/db/pipeline/expression.h
+++ b/src/mongo/db/pipeline/expression.h
@@ -990,12 +990,12 @@ protected:
 
 private:
     ExpressionDateToString(const boost::intrusive_ptr<ExpressionContext>& expCtx,
-                           const std::string& format,                  // The format string.
-                           boost::intrusive_ptr<Expression> date,      // The date to format.
-                           boost::intrusive_ptr<Expression> timeZone,  // The optional timezone.
+                           boost::intrusive_ptr<Expression> format,
+                           boost::intrusive_ptr<Expression> date,
+                           boost::intrusive_ptr<Expression> timeZone,
                            boost::intrusive_ptr<Expression> onNull);
 
-    const std::string _format;
+    boost::intrusive_ptr<Expression> _format;
     boost::intrusive_ptr<Expression> _date;
     boost::intrusive_ptr<Expression> _timeZone;
     boost::intrusive_ptr<Expression> _onNull;
diff --git a/src/mongo/db/pipeline/expression_date_test.cpp b/src/mongo/db/pipeline/expression_date_test.cpp
index fbceb9c5349..fe51f40eb80 100644
--- a/src/mongo/db/pipeline/expression_date_test.cpp
+++ b/src/mongo/db/pipeline/expression_date_test.cpp
@@ -591,8 +591,8 @@ TEST_F(ExpressionDateToStringTest, SerializesToObjectSyntax) {
     auto dateExp = Expression::parseExpression(expCtx, spec, expCtx->variablesParseState);
     auto expectedSerialization =
         Value(Document{{"$dateToString",
-                        Document{{"format", "%Y-%m-%d"_sd},
-                                 {"date", Document{{"$const", Date_t{}}}},
+                        Document{{"date", Document{{"$const", Date_t{}}}},
+                                 {"format", Document{{"$const", "%Y-%m-%d"_sd}}},
                                  {"timezone", Document{{"$const", "Europe/London"_sd}}},
                                  {"onNull", Document{{"$const", "nullDefault"_sd}}}}}});
 
@@ -603,15 +603,21 @@ TEST_F(ExpressionDateToStringTest, SerializesToObjectSyntax) {
 TEST_F(ExpressionDateToStringTest, OptimizesToConstantIfAllInputsAreConstant) {
     auto expCtx = getExpCtx();
 
-    // Test that it becomes a constant if both format and date are constant, and timezone is
+    // Test that it becomes a constant if date is constant, and both format and timezone are
     // missing.
-    auto spec = BSON("$dateToString" << BSON("format"
-                                             << "%Y-%m-%d"
-                                             << "date"
-                                             << Date_t{}));
+    auto spec = BSON("$dateToString" << BSON("date" << Date_t{}));
     auto dateExp = Expression::parseExpression(expCtx, spec, expCtx->variablesParseState);
     ASSERT(dynamic_cast<ExpressionConstant*>(dateExp->optimize().get()));
 
+    // Test that it becomes a constant if both format and date are constant, and timezone is
+    // missing.
+    spec = BSON("$dateToString" << BSON("format"
+                                        << "%Y-%m-%d"
+                                        << "date"
+                                        << Date_t{}));
+    dateExp = Expression::parseExpression(expCtx, spec, expCtx->variablesParseState);
+    ASSERT(dynamic_cast<ExpressionConstant*>(dateExp->optimize().get()));
+
     // Test that it becomes a constant if format, date and timezone are provided, and all are
     // constants.
     spec = BSON("$dateToString" << BSON("format"
@@ -680,6 +686,14 @@ TEST_F(ExpressionDateToStringTest, OptimizesToConstantIfAllInputsAreConstant) {
                                         << "$onNull"));
     dateExp = Expression::parseExpression(expCtx, spec, expCtx->variablesParseState);
     ASSERT_FALSE(dynamic_cast<ExpressionConstant*>(dateExp->optimize().get()));
+
+    // Test that it does *not* become a constant if 'format' does not evaluate to a constant.
+    spec = BSON("$dateToString" << BSON("format"
+                                        << "$format"
+                                        << "date"
+                                        << Date_t{}));
+    dateExp = Expression::parseExpression(expCtx, spec, expCtx->variablesParseState);
+    ASSERT_FALSE(dynamic_cast<ExpressionConstant*>(dateExp->optimize().get()));
 }
 
 TEST_F(ExpressionDateToStringTest, ReturnsOnNullValueWhenInputIsNullish) {
@@ -721,6 +735,79 @@ TEST_F(ExpressionDateToStringTest, ReturnsOnNullValueWhenInputIsNullish) {
     dateExp = Expression::parseExpression(expCtx, spec, expCtx->variablesParseState);
     ASSERT_VALUE_EQ(Value(), dateExp->evaluate(Document{}));
 }
+
+TEST_F(ExpressionDateToStringTest, ReturnsNullIfInputDateIsNullish) {
+    auto expCtx = getExpCtx();
+
+    auto spec = fromjson("{$dateToString: {date: '$date'}}");
+    auto dateExp = Expression::parseExpression(expCtx, spec, expCtx->variablesParseState);
+    ASSERT_VALUE_EQ(Value(BSONNULL), dateExp->evaluate(Document{{"date", BSONNULL}}));
+    ASSERT_VALUE_EQ(Value(BSONNULL), dateExp->evaluate(Document{}));
+}
+
+TEST_F(ExpressionDateToStringTest, ReturnsNullIfFormatIsNullish) {
+    auto expCtx = getExpCtx();
+
+    auto spec = fromjson("{$dateToString: {date: '$date', format: '$format'}}");
+    auto dateExp = Expression::parseExpression(expCtx, spec, expCtx->variablesParseState);
+    ASSERT_VALUE_EQ(Value(BSONNULL),
+                    dateExp->evaluate(Document{{"date", Date_t{}}, {"format", BSONNULL}}));
+    ASSERT_VALUE_EQ(Value(BSONNULL), dateExp->evaluate(Document{{"date", Date_t{}}}));
+}
+
+TEST_F(ExpressionDateToStringTest, UsesDefaultFormatIfNoneSpecified) {
+    auto expCtx = getExpCtx();
+
+    auto spec = fromjson("{$dateToString: {date: '$date'}}");
+    auto dateExp = Expression::parseExpression(expCtx, spec, expCtx->variablesParseState);
+    ASSERT_VALUE_EQ(Value("1970-01-01T00:00:00.000Z"_sd),
+                    dateExp->evaluate(Document{{"date", Date_t{}}}));
+}
+
+TEST_F(ExpressionDateToStringTest, FailsForInvalidTimezoneRegardlessOfInputDate) {
+    auto expCtx = getExpCtx();
+
+    auto spec = fromjson("{$dateToString: {date: '$date', timezone: '$tz'}}");
+    auto dateExp = Expression::parseExpression(expCtx, spec, expCtx->variablesParseState);
+    ASSERT_THROWS_CODE(dateExp->evaluate(Document{{"date", BSONNULL}, {"tz", "invalid"_sd}}),
+                       AssertionException,
+                       40485);
+    ASSERT_THROWS_CODE(
+        dateExp->evaluate(Document{{"date", BSONNULL}, {"tz", 5}}), AssertionException, 40517);
+}
+
+TEST_F(ExpressionDateToStringTest, FailsForInvalidFormatStrings) {
+    auto expCtx = getExpCtx();
+
+    auto spec = BSON("$dateToString" << BSON("date" << Date_t{} << "format"
+                                                    << "%n"));
+    auto dateExp = Expression::parseExpression(expCtx, spec, expCtx->variablesParseState);
+    ASSERT_THROWS_CODE(dateExp->evaluate({}), AssertionException, 18536);
+
+    spec = BSON("$dateToString" << BSON("date" << Date_t{} << "format"
+                                               << "%Y%"));
+    dateExp = Expression::parseExpression(expCtx, spec, expCtx->variablesParseState);
+    ASSERT_THROWS_CODE(dateExp->evaluate({}), AssertionException, 18535);
+}
+
+TEST_F(ExpressionDateToStringTest, FailsForInvalidFormatRegardlessOfInputDate) {
+    auto expCtx = getExpCtx();
+
+    auto spec = fromjson("{$dateToString: {date: '$date', format: '$format', onNull: 0}}");
+    auto dateExp = Expression::parseExpression(expCtx, spec, expCtx->variablesParseState);
+    ASSERT_THROWS_CODE(
+        dateExp->evaluate(Document{{"date", BSONNULL}, {"format", 5}}), AssertionException, 18533);
+    ASSERT_THROWS_CODE(dateExp->evaluate(Document{{"date", BSONNULL}, {"format", "%n"_sd}}),
+                       AssertionException,
+                       18536);
+    ASSERT_THROWS_CODE(dateExp->evaluate(Document{{"date", BSONNULL}, {"format", "%"_sd}}),
+                       AssertionException,
+                       18535);
+    ASSERT_THROWS_CODE(dateExp->evaluate(Document{{"date", "Invalid date"_sd}, {"format", 5}}),
+                       AssertionException,
+                       18533);
+}
+
 }  // namespace ExpressionDateToStringTest
 
 namespace ExpressionDateFromStringTest {
diff --git a/src/mongo/db/pipeline/value.cpp b/src/mongo/db/pipeline/value.cpp
index 67f4e4eb08e..5adbfaff580 100644
--- a/src/mongo/db/pipeline/value.cpp
+++ b/src/mongo/db/pipeline/value.cpp
@@ -57,9 +57,7 @@ using std::string;
 using std::stringstream;
 using std::vector;
 
-namespace {
-constexpr StringData kISOFormatString = "%Y-%m-%dT%H:%M:%S.%LZ"_sd;
-}
+constexpr StringData Value::kISOFormatString;
 
 void ValueStorage::verifyRefCountingIfShould() const {
     switch (type) {
@@ -604,7 +602,7 @@ string Value::coerceToString() const {
             return getTimestamp().toStringPretty();
 
         case Date:
-            return TimeZoneDatabase::utcZone().formatDate(kISOFormatString, getDate());
+            return TimeZoneDatabase::utcZone().formatDate(Value::kISOFormatString, getDate());
 
         case EOO:
         case jstNULL:
@@ -1140,7 +1138,7 @@ ostream& operator<<(ostream& out, const Value& val) {
         case Undefined:
             return out << "undefined";
         case Date:
-            return out << TimeZoneDatabase::utcZone().formatDate(kISOFormatString,
+            return out << TimeZoneDatabase::utcZone().formatDate(Value::kISOFormatString,
                                                                  val.coerceToDate());
         case bsonTimestamp:
             return out << val.getTimestamp().toString();
diff --git a/src/mongo/db/pipeline/value.h b/src/mongo/db/pipeline/value.h
index 90ecd5c3ffa..aa7f842d3a3 100644
--- a/src/mongo/db/pipeline/value.h
+++ b/src/mongo/db/pipeline/value.h
@@ -127,6 +127,8 @@ public:
     /// Deep-convert from BSONElement to Value
     explicit Value(const BSONElement& elem);
 
+    static constexpr StringData kISOFormatString = "%Y-%m-%dT%H:%M:%S.%LZ"_sd;
+
     /** Construct a long or integer-valued Value.
      *
      *  Used when preforming arithmetic operations with int where the
diff --git a/src/mongo/db/query/datetime/date_time_support.cpp b/src/mongo/db/query/datetime/date_time_support.cpp
index 31a38dba606..ad66f04b133 100644
--- a/src/mongo/db/query/datetime/date_time_support.cpp
+++ b/src/mongo/db/query/datetime/date_time_support.cpp
@@ -200,7 +200,7 @@ static timelib_tzinfo* timezonedatabase_gettzinfowrapper(char* tz_id,
 }
 
 Date_t TimeZoneDatabase::fromString(StringData dateString,
-                                    boost::optional<TimeZone> tz,
+                                    const TimeZone& tz,
                                     boost::optional<StringData> format) const {
     std::unique_ptr<timelib_error_container, TimeZoneDatabase::TimelibErrorContainerDeleter>
         errors{};
@@ -279,7 +279,7 @@ Date_t TimeZoneDatabase::fromString(StringData dateString,
                       << "\"");
     }
 
-    if (tz && !tz->isUtcZone()) {
+    if (!tz.isUtcZone()) {
         switch (parsedTime->zone_type) {
             case 0:
                 // Do nothing, as this indicates there is no associated time zone information.
@@ -306,7 +306,7 @@ Date_t TimeZoneDatabase::fromString(StringData dateString,
         }
     }
 
-    tz->adjustTimeZone(parsedTime.get());
+    tz.adjustTimeZone(parsedTime.get());
 
     return Date_t::fromMillisSinceEpoch(
         durationCount<Milliseconds>(Seconds(parsedTime->sse) + Microseconds(parsedTime->us)));
diff --git a/src/mongo/db/query/datetime/date_time_support.h b/src/mongo/db/query/datetime/date_time_support.h
index 9a94fd657b2..c2246fd74b6 100644
--- a/src/mongo/db/query/datetime/date_time_support.h
+++ b/src/mongo/db/query/datetime/date_time_support.h
@@ -384,7 +384,7 @@ public:
      *  * The string does not match the 'format' specifier.
      */
     Date_t fromString(StringData dateString,
-                      boost::optional<TimeZone> tz,
+                      const TimeZone& tz,
                       boost::optional<StringData> format = boost::none) const;
 
     /**