The Redfish Node class should be removed

The redfish Node class at one point had grand ambitions of being a central spot for putting Redfish specific routing code, but as the future came and went, most of the abstractions it provided ended up getting rolled into the core, and today, all it is is a thin wrapper around app.routeDynamic().

The key reason the Node class existed originally was because the internal route handler couldn't declare duplicate routes with different verbs, nor could it handle permissions. Both of these issues have been solved for some time now.

Because of the inheritance-based API the Node class provides, almost every route handler has boilerplate like.

if (params.size() != 1)
{
    messages::internalError(asyncResp->res);
    return;
}

At the top of every handler, which is problematic, and removes a lot of the compile-time routing magic that BMCWEB_ROUTE can do. It also requires construction and copy of a new string for every invocation.

The core of this enhancement will mostly be splitting up all the various doGet, doPatch, doPut, and other methods into their own BMCWEB_ROUTE declarations,

For example, this code from certificate service

class HTTPSCertificate : public Node
{
  public:
    HTTPSCertificate(App& app) :
        Node(app,
             "/redfish/v1/Managers/bmc/NetworkProtocol/HTTPS/Certificates/"
             "<str>/",
             std::string())
    {
        entityPrivileges = {
            {boost::beast::http::verb::get, {{"Login"}}},
            {boost::beast::http::verb::head, {{"Login"}}},
            {boost::beast::http::verb::patch, {{"ConfigureComponents"}}},
            {boost::beast::http::verb::put, {{"ConfigureComponents"}}},
            {boost::beast::http::verb::delete_, {{"ConfigureComponents"}}},
            {boost::beast::http::verb::post, {{"ConfigureComponents"}}}};
    }

    void doGet(crow::Response& res, const crow::Request& req,
               const std::vector<std::string>& params) override
    {
        if (params.size() != 1)
        {
            messages::internalError(asyncResp->res);
            return;
        }
       const std::string& id = params[0];
      // existing implementation
};

Would turn into

BMCWEB_ROUTE("/redfish/v1/Managers/bmc/NetworkProtocol/HTTPS/Certificates/<str>/")
.privileges({"ConfigureComponents"})
.methods(boost::beast::http::verb::get)(
    [](const crow::Request& req, crow::Response& res, const std::string& id) {
        // existing implementation
    });

Just looking the two blocks of code is motivation enough to do this, the second has less boilerplate, and many more coding errors are caught at compile time instead of runtime.

While the Node class is mostly dead, there is one exception that got added recently in the form of the isAllowedWithoutConfigureSelf, which got added to the node class. So far as I'm able to discern, it's a static method that has no interaction with the node class whatsoever, and can easily be moved to a non-member method without any changes.

openbmc / bmcweb

The Redfish Node class should be removed #181