szaszm commented on code in PR #1335: URL: https://github.com/apache/nifi-minifi-cpp/pull/1335#discussion_r891114384
########## libminifi/src/CronDrivenSchedulingAgent.cpp: ########## @@ -20,66 +20,53 @@ #include "CronDrivenSchedulingAgent.h" #include <chrono> #include <memory> -#include <thread> -#include <iostream> #include "core/Processor.h" #include "core/ProcessContext.h" #include "core/ProcessSessionFactory.h" -#include "core/Property.h" using namespace std::literals::chrono_literals; +using std::chrono::ceil; +using std::chrono::seconds; +using std::chrono::milliseconds; +using std::chrono::time_point_cast; +using std::chrono::system_clock; Review Comment: Since it's only used in `run`, consider moving it inside the function body, to minimize their scope. ########## libminifi/src/utils/Cron.cpp: ########## @@ -0,0 +1,510 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "utils/Cron.h" +#include <charconv> +#include "utils/TimeUtil.h" +#include "utils/StringUtils.h" +#include "date/date.h" + +using namespace std::literals::chrono_literals; + +using std::chrono::seconds; +using std::chrono::minutes; +using std::chrono::hours; +using std::chrono::days; + +// TODO(C++20): move to std::chrono when calendar is fully supported +using date::local_seconds; +using date::day; +using date::weekday; +using date::month; +using date::year; +using date::year_month_day; +using date::last; +using date::local_days; +using date::from_stream; +using date::make_time; +using date::Friday; +using date::Saturday; +using date::Sunday; + +namespace org::apache::nifi::minifi::utils { +namespace { + +template<class T> +std::optional<T> fromChars(const std::string& input) { + T t{}; + const auto last_char = &*std::cend(input); + const auto result = std::from_chars(&*std::cbegin(input), last_char, t); Review Comment: This is UB. Use `input.data()` and `input.data() + input.size()` instead. ########## libminifi/src/utils/Cron.cpp: ########## @@ -0,0 +1,510 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "utils/Cron.h" +#include <charconv> +#include "utils/TimeUtil.h" +#include "utils/StringUtils.h" +#include "date/date.h" + +using namespace std::literals::chrono_literals; + +using std::chrono::seconds; +using std::chrono::minutes; +using std::chrono::hours; +using std::chrono::days; + +// TODO(C++20): move to std::chrono when calendar is fully supported Review Comment: TODO comments should be used with a username. I would even avoid this kind of TODO comment, since the whole date library is to be replaced with the standardized version as soon as it's available in all target environments. ########## libminifi/src/utils/Cron.cpp: ########## @@ -0,0 +1,510 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "utils/Cron.h" +#include <charconv> +#include "utils/TimeUtil.h" +#include "utils/StringUtils.h" +#include "date/date.h" + +using namespace std::literals::chrono_literals; + +using std::chrono::seconds; +using std::chrono::minutes; +using std::chrono::hours; +using std::chrono::days; + +// TODO(C++20): move to std::chrono when calendar is fully supported +using date::local_seconds; +using date::day; +using date::weekday; +using date::month; +using date::year; +using date::year_month_day; +using date::last; +using date::local_days; +using date::from_stream; +using date::make_time; +using date::Friday; +using date::Saturday; +using date::Sunday; + +namespace org::apache::nifi::minifi::utils { +namespace { + +template<class T> +std::optional<T> fromChars(const std::string& input) { + T t{}; + const auto last_char = &*std::cend(input); + const auto result = std::from_chars(&*std::cbegin(input), last_char, t); + if (result.ptr != last_char) + return std::nullopt; + return t; +} + +bool operator<=(const weekday& lhs, const weekday& rhs) { + return lhs.c_encoding() <= rhs.c_encoding(); +} + +template <typename FieldType> +FieldType parse(const std::string&); + +template <> +seconds parse<seconds>(const std::string& second_str) { + if (auto sec_int = fromChars<uint64_t>(second_str); sec_int && *sec_int <= 59) + return seconds(*sec_int); + throw BadCronExpression("Invalid second " + second_str); +} + +template <> +minutes parse<minutes>(const std::string& minute_str) { + if (auto min_int = fromChars<uint64_t>(minute_str); min_int && *min_int <= 59) + return minutes(*min_int); + throw BadCronExpression("Invalid minute " + minute_str); +} + +template <> +hours parse<hours>(const std::string& hour_str) { + if (auto hour_int = fromChars<uint64_t>(hour_str); hour_int && *hour_int <= 23) + return hours(*hour_int); + throw BadCronExpression("Invalid hour " + hour_str); +} + +template <> +days parse<days>(const std::string& days_str) { + if (auto days_int = fromChars<uint64_t>(days_str)) + return days(*days_int); + throw BadCronExpression("Invalid days " + days_str); +} + +template <> +day parse<day>(const std::string& day_str) { + if (auto day_int = fromChars<uint64_t>(day_str); day_int && day_int >= 1 && day_int <= 31) + return day(*day_int); + throw BadCronExpression("Invalid day " + day_str); +} + +template <> +month parse<month>(const std::string& month_str) { +// https://github.com/HowardHinnant/date/issues/550 +// TODO(gcc11): Due to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78714 +// the month parsing with '%b' is case sensitive in gcc11 +// This has been fixed in gcc12 Review Comment: This is not a TODO, just a workaround comment. Please remove the TODO part. ########## libminifi/src/utils/Cron.cpp: ########## @@ -0,0 +1,510 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "utils/Cron.h" +#include <charconv> +#include "utils/TimeUtil.h" +#include "utils/StringUtils.h" +#include "date/date.h" + +using namespace std::literals::chrono_literals; + +using std::chrono::seconds; +using std::chrono::minutes; +using std::chrono::hours; +using std::chrono::days; + +// TODO(C++20): move to std::chrono when calendar is fully supported +using date::local_seconds; +using date::day; +using date::weekday; +using date::month; +using date::year; +using date::year_month_day; +using date::last; +using date::local_days; +using date::from_stream; +using date::make_time; +using date::Friday; +using date::Saturday; +using date::Sunday; + +namespace org::apache::nifi::minifi::utils { +namespace { + +template<class T> +std::optional<T> fromChars(const std::string& input) { + T t{}; + const auto last_char = &*std::cend(input); + const auto result = std::from_chars(&*std::cbegin(input), last_char, t); + if (result.ptr != last_char) + return std::nullopt; + return t; +} + +bool operator<=(const weekday& lhs, const weekday& rhs) { + return lhs.c_encoding() <= rhs.c_encoding(); +} + +template <typename FieldType> +FieldType parse(const std::string&); + +template <> +seconds parse<seconds>(const std::string& second_str) { + if (auto sec_int = fromChars<uint64_t>(second_str); sec_int && *sec_int <= 59) + return seconds(*sec_int); + throw BadCronExpression("Invalid second " + second_str); +} + +template <> +minutes parse<minutes>(const std::string& minute_str) { + if (auto min_int = fromChars<uint64_t>(minute_str); min_int && *min_int <= 59) + return minutes(*min_int); + throw BadCronExpression("Invalid minute " + minute_str); +} + +template <> +hours parse<hours>(const std::string& hour_str) { + if (auto hour_int = fromChars<uint64_t>(hour_str); hour_int && *hour_int <= 23) + return hours(*hour_int); + throw BadCronExpression("Invalid hour " + hour_str); +} + +template <> +days parse<days>(const std::string& days_str) { + if (auto days_int = fromChars<uint64_t>(days_str)) + return days(*days_int); + throw BadCronExpression("Invalid days " + days_str); +} + +template <> +day parse<day>(const std::string& day_str) { + if (auto day_int = fromChars<uint64_t>(day_str); day_int && day_int >= 1 && day_int <= 31) + return day(*day_int); + throw BadCronExpression("Invalid day " + day_str); +} + +template <> +month parse<month>(const std::string& month_str) { +// https://github.com/HowardHinnant/date/issues/550 +// TODO(gcc11): Due to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78714 +// the month parsing with '%b' is case sensitive in gcc11 +// This has been fixed in gcc12 +#if defined(__GNUC__) && __GNUC__ < 12 + auto patched_month_str = StringUtils::toLower(month_str); + if (!patched_month_str.empty()) + patched_month_str[0] = std::toupper(patched_month_str[0]); + std::stringstream stream(patched_month_str); +#else + std::stringstream stream(month_str); +#endif + + stream.imbue(std::locale("en_US.UTF-8")); + month parsed_month{}; + if (month_str.size() > 2) { + from_stream(stream, "%b", parsed_month); + if (!stream.fail() && parsed_month.ok() && stream.peek() == EOF) + return parsed_month; + } else { + from_stream(stream, "%m", parsed_month); + if (!stream.fail() && parsed_month.ok() && stream.peek() == EOF) + return parsed_month; + } + + throw BadCronExpression("Invalid month " + month_str); +} + +template <> +weekday parse<weekday>(const std::string& weekday_str) { +// https://github.com/HowardHinnant/date/issues/550 +// TODO(gcc11): Due to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78714 +// the weekday parsing with '%a' is case sensitive in gcc11 +// This has been fixed in gcc12 +#if defined(__GNUC__) && __GNUC__ < 12 + auto patched_weekday_str = StringUtils::toLower(weekday_str); + if (!patched_weekday_str.empty()) + patched_weekday_str[0] = std::toupper(patched_weekday_str[0]); + std::stringstream stream(patched_weekday_str); +#else + std::stringstream stream(weekday_str); +#endif + stream.imbue(std::locale("en_US.UTF-8")); + + if (weekday_str.size() > 2) { + weekday parsed_weekday{}; + from_stream(stream, "%a", parsed_weekday); + if (!stream.fail() && parsed_weekday.ok() && stream.peek() == EOF) + return parsed_weekday; + } else { + unsigned weekday_num; + stream >> weekday_num; + if (!stream.fail() && weekday_num < 7 && stream.peek() == EOF) + return weekday(weekday_num-1); + } + throw BadCronExpression("Invalid weekday: " + weekday_str); +} + +template <> +year parse<year>(const std::string& year_str) { + if (auto year_int = fromChars<uint64_t>(year_str); year_int && *year_int >= 1970 && *year_int <= 2999) + return year(*year_int); + throw BadCronExpression("Invalid year: " + year_str); +} + +template <typename FieldType> +FieldType getFieldType(local_seconds time_point); + +template <> +year getFieldType(local_seconds time_point) { + year_month_day year_month_day(floor<days>(time_point)); + return year_month_day.year(); +} + +template <> +month getFieldType(local_seconds time_point) { + year_month_day year_month_day(floor<days>(time_point)); + return year_month_day.month(); +} + +template <> +day getFieldType(local_seconds time_point) { + year_month_day year_month_day(floor<days>(time_point)); + return year_month_day.day(); +} + +template <> +hours getFieldType(local_seconds time_point) { + auto dp = floor<days>(time_point); + auto time = make_time(time_point-dp); + return time.hours(); +} + +template <> +minutes getFieldType(local_seconds time_point) { + auto dp = floor<days>(time_point); + auto time = make_time(time_point-dp); + return time.minutes(); +} + +template <> +seconds getFieldType(local_seconds time_point) { + auto dp = floor<days>(time_point); + auto time = make_time(time_point-dp); + return time.seconds(); +} + +template <> +weekday getFieldType(local_seconds time_point) { + auto dp = floor<days>(time_point); + return weekday(dp); +} + +bool isWeekday(year_month_day date) { + weekday date_weekday = weekday(local_days(date)); + return date_weekday != Saturday && date_weekday != Sunday; +} + +template <typename FieldType> +class SingleValueField : public CronField { + public: + explicit SingleValueField(FieldType value) : value_(value) {} + + [[nodiscard]] bool matches(local_seconds time_point) const override { + return value_ == getFieldType<FieldType>(time_point); + } + + private: + FieldType value_; +}; + +class NotCheckedField : public CronField { + public: + NotCheckedField() = default; + + [[nodiscard]] bool matches(local_seconds) const override { return true; } +}; + +class AllValuesField : public CronField { + public: + AllValuesField() = default; + + [[nodiscard]] bool matches(local_seconds) const override { return true; } +}; + +template <typename FieldType> +class RangeField : public CronField { + public: + explicit RangeField(FieldType lower_bound, FieldType upper_bound) + : lower_bound_(std::move(lower_bound)), + upper_bound_(std::move(upper_bound)) { + if (!(lower_bound_ <= upper_bound_)) + throw std::out_of_range("lower bound must be smaller or equal to upper bound"); + } + + [[nodiscard]] bool matches(local_seconds value) const override { + return lower_bound_ <= getFieldType<FieldType>(value) && getFieldType<FieldType>(value) <= upper_bound_; + } + + private: + FieldType lower_bound_; + FieldType upper_bound_; +}; + +template <typename FieldType> +class ListField : public CronField { + public: + explicit ListField(std::vector<FieldType> valid_values) : valid_values_(std::move(valid_values)) {} + + [[nodiscard]] bool matches(local_seconds value) const override { + return std::find(valid_values_.begin(), valid_values_.end(), getFieldType<FieldType>(value)) != valid_values_.end(); + } + + private: + std::vector<FieldType> valid_values_; +}; + +template <typename FieldType> +class IncrementField : public CronField { + public: + IncrementField(FieldType start, int increment) : start_(start), increment_(increment) {} + + [[nodiscard]] bool matches(local_seconds value) const override { + return (getFieldType<FieldType>(value) - start_).count() % increment_ == 0; + } + + private: + FieldType start_; + int increment_; +}; + +class LastNthDayInMonthField : public CronField { + public: + explicit LastNthDayInMonthField(days offset) : offset_(offset) {} + + [[nodiscard]] bool matches(local_seconds tp) const override { + year_month_day date(floor<days>(tp)); + auto last_day = date.year()/date.month()/last; + auto target_date = local_days(last_day)-offset_; + return local_days(date) == target_date; + } + + private: + days offset_; +}; + +class NthWeekdayField : public CronField { + public: + NthWeekdayField(weekday wday, uint8_t n) : weekday_(wday), n_(n) {} + + [[nodiscard]] bool matches(local_seconds tp) const override { + year_month_day date(floor<days>(tp)); + auto target_date = date.year()/date.month()/(weekday_[n_]); + return local_days(date) == local_days(target_date); + } + + private: + weekday weekday_; + uint8_t n_; +}; + +class LastWeekDayField : public CronField { + public: + LastWeekDayField() = default; + + [[nodiscard]] bool matches(local_seconds value) const override { + year_month_day date(floor<days>(value)); + year_month_day last_day_of_the_month_date = year_month_day(local_days(date.year()/date.month()/last)); + if (isWeekday(last_day_of_the_month_date)) + return date == last_day_of_the_month_date; + year_month_day last_friday_of_the_month_date = year_month_day(local_days(date.year()/date.month()/Friday[last])); + return date == last_friday_of_the_month_date; + } +}; + +class LastSpecificDayOfTheWeekOfTheMonth : public CronField { + public: + explicit LastSpecificDayOfTheWeekOfTheMonth(weekday wday) : weekday_(wday) {} + + [[nodiscard]] bool matches(local_seconds value) const override { + year_month_day date(floor<days>(value)); + year_month_day last_weekday_of_month_date = year_month_day(local_days(date.year()/date.month()/weekday_[last])); + return date == last_weekday_of_month_date; + } + private: + weekday weekday_; +}; + +class ClosestWeekdayToTheNthDayOfTheMonth : public CronField { + public: + explicit ClosestWeekdayToTheNthDayOfTheMonth(day day_number) : day_number_(day_number) {} + + [[nodiscard]] bool matches(local_seconds value) const override { + year_month_day date(floor<days>(value)); + for (auto diff : {0, -1, 1, -2, 2}) { + auto target_date = date.year() / date.month() / (day_number_ + days(diff)); + if (target_date.ok() && isWeekday(target_date)) + return target_date == date; + } + + return false; + } + + private: + day day_number_; +}; + +template <typename FieldType> +std::unique_ptr<CronField> parseCronField(const std::string& field_str) { + try { + if (field_str == "*") { + return std::make_unique<AllValuesField>(); + } + + if (field_str == "?") { + return std::make_unique<NotCheckedField>(); + } + + if (field_str == "L") { + if (std::is_same<day, FieldType>()) + return std::make_unique<LastNthDayInMonthField>(days(0)); + if (std::is_same<weekday, FieldType>()) + return std::make_unique<SingleValueField<weekday>>(Saturday); + throw BadCronExpression("L can only be used in the Day of month/Day of week fields"); + } + + if (field_str == "LW") { + if (!std::is_same<day, FieldType>()) Review Comment: Consider using `if constexpr` for these checks. It's not necessary, since all branches can be compiled in all cases, but makes it clearer that this is happening in compile-time. ########## libminifi/test/unit/CronTests.cpp: ########## @@ -0,0 +1,686 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#include <string> + +#include "../Catch.h" +#include "utils/Cron.h" +#include "date/date.h" +#include "date/tz.h" + +using std::chrono::system_clock; +using std::chrono::seconds; +using org::apache::nifi::minifi::utils::Cron; + + +void checkNext(const std::string& expr, const date::zoned_time<seconds>& from, const date::zoned_time<seconds>& next) { + auto cron_expression = Cron(expr); + auto next_trigger = cron_expression.calculateNextTrigger(from.get_local_time()); + CHECK(next_trigger == next.get_local_time()); +} + + +TEST_CASE("Cron expression ctor tests", "[cron]") { + REQUIRE_THROWS(Cron("1600 ms")); + REQUIRE_THROWS(Cron("foo")); + REQUIRE_THROWS(Cron("61 0 0 * * *")); + REQUIRE_THROWS(Cron("0 61 0 * * *")); + REQUIRE_THROWS(Cron("0 0 24 * * *")); + REQUIRE_THROWS(Cron("0 0 0 32 * *")); + + REQUIRE_THROWS(Cron("1banana * * * * * *")); + REQUIRE_THROWS(Cron("* 1banana * * * * *")); + REQUIRE_THROWS(Cron("* * 1banana * * * *")); + REQUIRE_THROWS(Cron("* * * 1banana * * *")); + REQUIRE_THROWS(Cron("* * * * 1banana * *")); + REQUIRE_THROWS(Cron("* * * * DECbanana * *")); + REQUIRE_THROWS(Cron("* * * * * WEDbanana *")); + + REQUIRE_THROWS(Cron("* * * * * * 1banana")); + REQUIRE_THROWS(Cron("* * * * * * 2000banana")); + + REQUIRE_THROWS(Cron("1G * * * * * *")); + REQUIRE_THROWS(Cron("* 1G * * * * *")); + REQUIRE_THROWS(Cron("* * 1G * * * *")); + REQUIRE_THROWS(Cron("* * * 1G * * *")); + REQUIRE_THROWS(Cron("* * * * 1G * *")); + REQUIRE_THROWS(Cron("* * * * * 1G *")); + REQUIRE_THROWS(Cron("* * * * * * 1G")); + + // Number of fields must be 6 or 7 + REQUIRE_THROWS(Cron("* * * * *")); + REQUIRE_NOTHROW(Cron("* * * * * *")); + REQUIRE_NOTHROW(Cron("* * * * * * *")); + REQUIRE_THROWS(Cron("* * * * * * * *")); + + // LW can only be used in 4th field + REQUIRE_THROWS(Cron("LW * * * * * *")); + REQUIRE_THROWS(Cron("* LW * * * * *")); + REQUIRE_THROWS(Cron("* * LW * * * *")); + REQUIRE_NOTHROW(Cron("* * * LW * * *")); + REQUIRE_THROWS(Cron("* * * * LW * *")); + REQUIRE_THROWS(Cron("* * * * * LW *")); + REQUIRE_THROWS(Cron("* * * * * * LW")); + + // n#m can only be used in 6th field + REQUIRE_THROWS(Cron("2#1 * * * * * *")); + REQUIRE_THROWS(Cron("* 2#1 * * * * *")); + REQUIRE_THROWS(Cron("* * 2#1 * * * *")); + REQUIRE_THROWS(Cron("* * * 2#1 * * *")); + REQUIRE_THROWS(Cron("* * * * 2#1 * *")); + REQUIRE_NOTHROW(Cron("* * * * * 2#1 *")); + REQUIRE_THROWS(Cron("* * * * * * 2#1")); + + // L can only be used in 4th, 5th, 6th fields + REQUIRE_THROWS(Cron("L * * * * * *")); + REQUIRE_THROWS(Cron("* L * * * * *")); + REQUIRE_THROWS(Cron("* * L * * * *")); + REQUIRE_NOTHROW(Cron("* * * L * * *")); + REQUIRE_THROWS(Cron("* * * * L * *")); + REQUIRE_NOTHROW(Cron("* * * * * L *")); + REQUIRE_THROWS(Cron("* * * * * * L")); Review Comment: The comment says 4,5,6, but the code says 4,6. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
