[WIP] Introduce a polymorphic Error class

[MartinBriza]

📒 Description

There is a lot of text so please don't get scared. I just want to know your opinion, especially on the topic if this is overengineered or not from your point of view.

This PR will introduce an error-focused suite of classes to handle errors in a more manageable manner.
All of the new Error API is implemented in the src/util/error.h file and it's rather short.
For now, the API design is as follows:

• ErrorBase is an abstract class implementing a basic API to work with errors. It provides the following methods:
  • std::string Class() returns the name of the error class (like DatabaseError or NoError for example)
  • int Type() is the type of the error - this can (doesn't have to) be provided by the class implementing this API. It is intended to be used for different types of errors, like with the database, I have currently implemented types for example for:
    • PROGRAMMING_ERROR (when a null argument is passed),
    • SESSION_ERROR (when a SQL query fails),
    • INCONSISTENT_DATA (when we're trying to update a nonexistent value for example)
    • ... and more - as I mentioned previously, these will be dependent on the particular <something>Error class implementation and will be different for a future TimeEntryError
  • bool IsError() to determine if it's actually an error (only NoError will return false in here)
  • std::string LogMessage() will return a message to be printed to the log (and probably sent to Bugsnag as well)
  • std::string UserMessage() will return a message to be displayed to the user (which will very often be an empty string)
• Error is a wrapper class that is never used on its own. It acts as a return value to all methods that were returning error before. I needed to implement it like this because abstract (polymorphic) classes in C++ have to be passed around as pointers if we want to benefit from the polymorphism (assigning a DatabaseError value to a ErrorBase variable "slices" it, removing the DatabaseError part of the value).
  • template<class T> Error(T &&e) is the most important method (and a constructor) of this class because it allows implicit conversion of all ErrorBase-based classes to (for example) Error<DataBaseError> which can be passed around safely (and since it contains a smart pointer, the memory will be freed automatically when it goes out of scope)
  • Then there are some helper methods to allow accessing the internal ErrorBase instance and working with it in an easy manner.

Now, that I've described the internals (which seem really convoluted but don't worry, they allow for something that I consider very nice further down the road), I can put an example of the usage below:

I can now declare a class called DatabaseError and that will contain an enum with a few error types:

class DatabaseError : public ErrorBase {
public:
    enum Type {
        UNKNOWN_ERROR,
        SESSION_ERROR,
        INCONSISTENT_DATA,
        PROGRAMMING_ERROR
    };
    inline static const std::map<int, std::string> UserMessages {
        { UNKNOWN_ERROR, "Unexpected database error" }, // safeguard
        { SESSION_ERROR, {} }, // No string because it doesn't have to be reported to the user
        { INCONSISTENT_DATA, "Data storage failed, check your time entry list for errors" }, //  Will get shown to the user
        { PROGRAMMING_ERROR, {} } // No user displayed error but a bugsnag report is necessary because we messed up somewhere
    };
    // regular constructor to be used in the DB code
    DatabaseError(enum Type type, const std::string &logMessage)  : ErrorBase(), log_message_(logMessage), type_(type) {}
    DatabaseError(DatabaseError &&o) = default;
    DatabaseError(const DatabaseError &o) = default;

    // just to report which Error type this is
    virtual std::string Class() const override {  return "DatabaseError"; }
    // returns what Type (see the enum above) this is
    virtual int Type() const override { return type_; }
    // This just returns the log message that was used when creating the error
    virtual std::string LogMessage() const override { return log_message_; }
    // And this will look into the map above to find the message to show to the user
    virtual std::string UserMessage() const override {
        auto it = UserMessages.find(Type());
        if (it != UserMessages.end()) {
            return it->second;
        }
        return UserMessages.at(UNKNOWN_ERROR);
    }
private:
    std::string log_message_;
    enum Type type_;
};

Now, all this is just an example, the UserMessage handling doesn't really have to be an enum with a map, we can of course implement a class that will handle this as a string that will get constructed somewhere else - OR we don't have to show anything to the user at all.

With this class implemented, we can then rewrite the Database class and change the error usages to DatabaseError:

BEFORE:

error Database::setJournalMode(const std::string &mode) {
    if (mode.empty()) {
        return error("Cannot set journal mode without a mode");
    }


    try {
        Poco::Mutex::ScopedLock lock(session_m_);
        poco_check_ptr(session_);

        *session_ <<
                  "PRAGMA journal_mode=" << mode,
                  now;
    } catch(const Poco::Exception& exc) {
        return exc.displayText();
    } catch(const std::exception& ex) {
        return ex.what();
    } catch(const std::string & ex) {
        return ex;
    }
    return last_error("setJournalMode");
}

AFTER:

Error Database::setJournalMode(const std::string &mode) {
    if (mode.empty()) {
        return DatabaseError { DatabaseError::PROGRAMMING_ERROR, "Cannot set journal mode without a mode" };
    }

    try {
        Poco::Mutex::ScopedLock lock(session_m_);
        poco_check_ptr(session_);

        *session_ <<
                  "PRAGMA journal_mode=" << mode,
                  now;
    } catch(const Poco::Exception& exc) {
        return DatabaseError { DatabaseError::SESSION_ERROR, exc.displayText() };
    } catch(const std::exception& ex) {
        return DatabaseError { DatabaseError::SESSION_ERROR, ex.what() };
    } catch(const std::string & ex) {
        return DatabaseError { DatabaseError::SESSION_ERROR, ex };
    }
    return last_error("setJournalMode");
}

Notice how the error messages are now classified according to where they are coming from.

🕶️ Types of changes

• New feature (non-breaking change which adds functionality)

🤯 List of changes

• Most previously shown confusing error messages are now more user friendly

👫 Relationships

Closes #4515

🔎 Review hints

There will have to be some discussion on which errors need to be reported to which channels (shown to the user, written to log, sent to bugsnag).

[AndrewVebster]

Great 👍 I like that now errors have a type and also that we can distinguish between user-faced errors and ones that should not be shown. This is an awesome improvement.

Because I'm noob in C++ cannot comment on the implementation details. Wanted to ask only about this:
if (err->IsError()) { ...
This is a little bit confusing to me that Error could be not an error. Is it possible to return null instead of NoError?

[MartinBriza]

Yeah, it is possible. In a similar vein, I was actually thinking about implementing a operator bool() for implicit conversion so instead of err->isError() you would write just if (err) (or if (!err)?)
This may get confusing with doing things like handling return calls directly because then it'd be tempting to write this: if (LoadSomeDataOrSomething()) { ... with the function returning Error - then it's a bit confusing. Maybe casting a real error to false and NoError to true would be the way?

[MartinBriza]

Well, overall, after thinking about this today, I don't think an implicit conversion to true/false or nullptr is not necessarily what we want because it could lead to confusing constructs as mentioned above (in both cases when an actual error is treated as true and when it's treated as false).

I agree though that using if (err.IsError()) is a bit clumsy. At this point, we can do if (err == NoError {}) which is even worse. However, after we're rid of the original std::string based error "class", we can reuse the noError constant for this purpose and the errors could look the same as before: if (err == noError) which is probably the least confusing way.

HOWEVER, one can do a lot with the C++ syntax so if you feel like there's a cleaner way to check errors, please just suggest how the construct should look like according to you and I can try implementing that.