C++ Structural Requests System with Payload Management

Question

First things first, I'd thank Mr. G. Sliepen and Mr. Davislor regarding their help in the previous questions (C++ System data transfer design) (C++ System data transfer design - Following 1), given the resultant code can be found here*.

* The code includes some added functionalities beyond the point :)

Now I'm taking the review request one step further, as I've mentioned the "simplified SystemInterface" within the last question, I'm here to focus on it with any potential enhancements and optimizations.

General notes:

• The System Data transfer is over-simplified within this code.
• An introduced "Guarders" to guard against inaccepted payloads and allowed for correction in some cases.
• A partial of SystemInterface is herein provided.
• Comes with a "simplified ServiceManager", which manages Services (or Entities), giving each one a unique number which is utilized to identify it across other entities (as Logger/SystemInterface/..).
• SystemInterface is provided here as one instance lies in the global scope, in my code it's a composed object within each entity, which receives a pid at construction.
• The SystemInterface uses a kind of structural recursion to find the intended request.

Generally _execReq is called when a data is received by any communication mean (as MQTT/HTTP/Serial/...), and it's callable from other entities ofcourse.

One of my concerns is the runtime of _execReq call, and whether it's already optimized or there's a room for optimizing.

#include <algorithm>
#include <cassert>
#include <concepts>
#include <cstdint>
#include <functional>
#include <iostream>
#include <iterator>
#include <ranges>
#include <span>
#include <string>
#include <string_view>
#include <unordered_map>
#include <utility>
#include <variant>
#include <vector>


//
//  Tools
//

#define CHOP_FRONT(vs) (std::vector<std::string>(++vs.begin(),vs.end()))

std::string join(const std::vector<std::string>& vs,const char* delim) {
    std::string rv="";
    if(vs.size()){
        std::string sd(delim);
        for(auto const& v:vs) rv+=v+sd;
        for(int i=0;i<sd.size();i++) rv.pop_back();     
    }
    return rv;
}

std::vector<std::string> split(const std::string& s, const char* delimiter){
    std::vector<std::string> vt;
    std::string delim(delimiter);
    auto len=delim.size();
    auto start = 0U;
    auto end = s.find(delim);
    while (end != std::string::npos){
        vt.push_back(s.substr(start, end - start));
        start = end + len;
        end = s.find(delim, start);
    }
    std::string tec=s.substr(start, end);
    if(tec.size()) vt.push_back(tec);       
    return vt;
}

enum class RequestErrors : uint8_t
{
    OK,
    NOT_FOUND,
    NOT_STRING,
    TOO_MANY_INPUTS,
    TOO_FEW_INPUTS,
    NOT_NUMERIC,

};


//
//  Data types
//
using BasicReferenceData = std::variant<std::string_view, std::span<std::uint8_t>>;
using BasicConcreteData = std::variant<std::string, std::vector<uint8_t>>;
std::span<std::uint8_t> t;
using ConcreteDataContainer = std::vector<BasicConcreteData>;
using ReferenceDataContainer = std::vector<BasicReferenceData>;

/* 
    Over simplified ReferenceData and ConcreteData, for the full implemented: https://godbolt.org/z/EofKMzGE9
 */
using ReferenceData = ReferenceDataContainer;
using ConcreteData = ConcreteDataContainer;


using ReturnData = ConcreteData;
using RequestReturn = std::pair<RequestErrors, ReturnData>;
using CallbackSignature = std::function<RequestReturn(const std::string &context, const ReferenceData &)>;


///
///     Guarders
///
using PayloadGuardFN = std::function<RequestReturn(const ReferenceData&)>;

/*      Guarders Helper functions    */

bool binaryData(const BasicReferenceData& data) {
    return std::holds_alternative<std::span<std::uint8_t>>(data);
}
bool printable(const BasicReferenceData& in) {
    auto span = std::get_if<std::span<std::uint8_t>>(&in);
    return !span || std::all_of(span->begin(), span->end()-1, isprint) && (isprint(span->back()) || span->back() == '\0'); // Allows the presence of zero terminator 
    // !span means it's a string_view
}
void to_string(BasicReferenceData& data) { // Might remove the check.
    if (std::holds_alternative<std::span<std::uint8_t>>(data)) {
        auto span = std::get<std::span<std::uint8_t>>(data);
        data = BasicReferenceData{std::string_view(reinterpret_cast<char*>(span.data()), span.size())};
    }
}
void to_string(ReferenceDataContainer::iterator it) {
    auto span = std::get<std::span<std::uint8_t>>(*it);
    *it = BasicReferenceData{std::string_view(reinterpret_cast<char*>(span.data()), span.size())};
}
bool is_number(std::string_view s) {
    bool numeric = false;
    if (s.size()) {
        bool first_char_valid = numeric = (isdigit(s.front()) || (s.front() == '-' && s.size() > 1));
        if (first_char_valid) {
            bool found_dot = false;
            for (auto it = s.begin(); it != s.end(); it++) {
                if (isdigit(*it)) continue;
                else if (*it=='.' && !found_dot) {
                    found_dot = true;
                    continue;
                }
                numeric = false;
                break;
            }
        }
    }
    return numeric;
}

class Guarder {
    public:
    virtual RequestReturn operator()(const ReferenceData& in) = 0;
};

/*        Guarder classes       */
class CountGuarder : public Guarder { // Or a template ??
    size_t count;
public:
            RequestReturn operator()(const ReferenceData& in) override{ return std::make_pair((in.size() > count ? RequestErrors::TOO_MANY_INPUTS : in.size() < count ?  RequestErrors::TOO_FEW_INPUTS : RequestErrors::OK), ReturnData{});}
    CountGuarder(size_t count) : count(count) {}
                            
};

class StringsGuarder : public Guarder {
    public:
            RequestReturn operator()(const ReferenceData& in) override {
                std::cout << "String Guarder\t";
                auto mut_in = const_cast<ReferenceData*>(&in);
                // auto view = in | std::views::filter(binaryData);
                if (std::all_of(in.begin(), in.end(), printable)) {
                    for (auto it = mut_in->begin(); it != mut_in->end(); it++) {
                        if (binaryData(*it)) {
                            to_string(it);
                        }
                    }
                    // std::ranges::for_each(mut_in | std::views::filter(binaryData), to_string);
                    std::cout << "OK\n";
                    return std::make_pair(RequestErrors::OK, ConcreteData{});
                } else {
                    std::cout << "NOT_STRING\n";
                    return std::make_pair(RequestErrors::NOT_STRING, ConcreteData{});
                }
            }
};

// Should follow a StringGuarder within the pipeline and remove the StringGuarder inside?
class NumericGuarder : public Guarder {
    public:
            RequestReturn operator()(const ReferenceData& in) override {
                std::cout << "Numeric Guarder\t";
                StringsGuarder stringify;
                auto result = stringify(in);
                if (result.first != RequestErrors::OK) {
                    return result;
                }
                for (auto& i : in) {
                    auto* sv = std::get_if<std::string_view>(&i);
                    if (!sv || !is_number(*sv)){
                        std::cout << "Not Numeric!\n";
                        return std::make_pair(RequestErrors::NOT_NUMERIC, ConcreteDataContainer{std::string(*sv)});
                    }
                }
                std::cout << "OK\n";

                return std::make_pair(RequestErrors::OK, ConcreteData{});
            }
};


// Can build custom Guarders.

RequestReturn toStringCorrector(const ReferenceData& pload) {
    StringsGuarder stringify;
    return stringify(pload);
}

//
//  SystemInterface
//

struct request {
    std::uint32_t           owner;
    std::uint32_t           levID;
    CallbackSignature       cbf;
    std::vector<Guarder*>   guarders;
};

using   REQMAP          = std::unordered_multimap<std::string,request>;
using   REQMAP_I        = REQMAP::iterator;

enum REQ_ID {
    REQ_ROOT,
    REQ_LOGGER, // A predefined ID for some instances, as the Logger.
    REQ_MAX
};


class SystemInterface
{
    REQMAP          requestsMap;

    REQMAP_I __exactMatch(const std::string& cmd,uint32_t owner){
        auto any=requestsMap.equal_range(cmd);
        for(auto i=any.first;i!=any.second;i++) if(i->second.owner==owner) return i;
        return requestsMap.end();
    }
    
    void __flatten(std::function<void(std::string)> fn){ // For generating "help" list
        REQMAP_I ptr;
        for(ptr=requestsMap.begin();ptr!=requestsMap.end(); ptr++){
            if(!(ptr->second.owner)){
                if(ptr->second.levID) _flattenCmds(fn,ptr->first,ptr->first,ptr->second.levID);    
                else fn(ptr->first);
            } 
        }
    }

    RequestReturn _dispatch(std::vector<std::string> vs, ReferenceDataContainer ploads, uint32_t owner=0){
        std::cout << "_dispatch: " << join(vs,"/") << "\n";
        if(vs.size()){
            REQMAP_I i;
            std::string cmd=vs[0];
            i=__exactMatch(cmd,owner);
            if(i!=requestsMap.end()){
                if(i->second.cbf) {
                    return [=]()->RequestReturn{ 
                        for(auto& guard : i->second.guarders) {
                            auto ret = (*guard)(ploads);
                            if (ret.first!=RequestErrors::OK) {
                                return ret;
                            }
                        }
                        return i->second.cbf(join(CHOP_FRONT(vs),"/"), ploads); 
                    }();
                }
                else return _dispatch(CHOP_FRONT(vs), ploads, i->second.levID);
            } else return std::make_pair(RequestErrors::NOT_FOUND, ConcreteDataContainer{});
        } else return std::make_pair(RequestErrors::NOT_FOUND, ConcreteDataContainer{});
    }
    
public:
    void addRequest(uint32_t owner, uint32_t levId, const std::string &name, CallbackSignature cbf, std::initializer_list<Guarder*> guarders={})
    {
        if (__exactMatch(name, owner) == requestsMap.end()){
            // std::cout << "Inserting " << name << "\n";
            requestsMap.insert(std::make_pair(name,request{owner,levId, cbf, guarders}));}
        else 
            std::cout << "Already assigned path!\n";
    }

    
    RequestReturn _execReq(std::string topic, ReferenceDataContainer ploads){
        std::vector<std::string> cmd=split(topic,"/");
        return _dispatch(cmd, ploads); // optimise?
    }
    
    void _flattenCmds(std::function<void(std::string)> fn,std::string cmd,std::string prefix,uint32_t lev){
        REQMAP_I i=requestsMap.find(cmd);
        for(i=requestsMap.begin();i!=requestsMap.end();i++){
            if(i->second.owner==lev){
                std::string trim = prefix+"/"+i->first;
                if(i->second.levID) _flattenCmds(fn,i->first,trim,i->second.levID);
                else fn(trim);
            }
        }
    }

    void help() {
        std::vector<std::string> unsorted={};
        __flatten([&unsorted](std::string s){ unsorted.push_back(s); });
        sort(unsorted.begin(),unsorted.end());
        std::cout << "HELP: \n";
        for (auto& path : unsorted) {
            std::cout << "\t" << path << "\n";
        }
    }

};

SystemInterface interface;


//
// Guarder objects
//

CountGuarder c1Guard(1);
NumericGuarder nGuard;
StringsGuarder sGuard;

//
// ServiceManager
//

class ServiceManager {
    protected:
    std::uint32_t pid;
    std::string name;
    static std::uint32_t services; 
    ServiceManager(std::string name) : name(name), pid(++services+REQ_MAX) {
        // std::cout << "name=" << name << " pid=" << pid << "\n";
        } 
};
std::uint32_t ServiceManager::services=0;

class Service1 : public ServiceManager {
    public:
    Service1() : ServiceManager("service1") {
        
        // Installs a level of "svc1"
        interface.addRequest(REQ_ROOT, pid, "svc1", nullptr, {}); // LevelID
        // Can add sub level ID on top of (pid) further ...

        // Adds a request path "svc1/test1"
        interface.addRequest(pid, 0, "test1",[this](const std::string& ctx, const ReferenceData& pload){
            std::cout << "Received on test1\n";
            return std::make_pair(RequestErrors::OK, ConcreteDataContainer{std::string{"Hello from test1"}});
        }, {});

        // Adds a request path "svc1/onestring" 
        interface.addRequest(pid, 0, "onestring",[this](const std::string& ctx, const ReferenceData& pload){
            std::cout << "Received on onestring\n";
            return std::make_pair(RequestErrors::OK, ConcreteDataContainer{std::string{"Hello from onestring"}});
        }, {&c1Guard, &sGuard});
    }
};

class Logger : public ServiceManager {
    // dummy logger
    int level;
    public:
    Logger() : ServiceManager("logger") {
        interface.addRequest(REQ_ROOT, REQ_LOGGER, "logger", nullptr, {}); // LevelID

        interface.addRequest(REQ_LOGGER, 0, "level", [this](const std::string& ctx, const ReferenceData& pload){
            std::cout << "Received on level ctx=" << ctx << "\n";
            if (ctx == "set") {
                if ((pload.size() == 1)) {
                    if (auto plevel = std::get_if<std::string_view>(&pload[0])) { // MUST be true with the sGuard.
                        level = std::atoi(std::string(*plevel).c_str());
                    }
                } else {
                    return std::make_pair(RequestErrors::TOO_FEW_INPUTS, ConcreteDataContainer{});
                }
            } else if (ctx != "get") {
                return std::make_pair(RequestErrors::NOT_FOUND, ConcreteDataContainer{});
            }
            return std::make_pair(RequestErrors::OK, ConcreteDataContainer{std::to_string(level)});
        }, {&sGuard, &nGuard});
    }
};

Service1 svc1;
Logger logger;

And here's a compiler explorer link with a simple example.

Lastly, I'd like to attribute Philbowles (Passed) who is the original author of SystemInterface's internal methods which inspired me.

With Thanks,
Hamza Hajeir

Answer 1

Avoid macros whenever possible

Macros are notoriously difficult to write correctly, and will easily fail with different use cases. For example, the following will fail to compile:

CHOP_FRONT(std::vector<std::string>{"Aap", "Noot", "Mies"});

If you had written a regular function, as already suggested by Davislor, you would have avoided this issue.

Naming

I strongly recommend you use verbs for functions, and nouns for types and variables. For example, isBinaryData() and isPrintable().

Be consistent: why use snake_case in to_string(), but camelCase in toStringCorrector()? Why does struct request not start with a capital when other types do? Why are some type aliases in ALL_CAPS?

Never use double underscores: these are always reserved. I also recommend not using leading underscores at all. Either use trailing underscores (1 at most), or use some other prefix, like m_ for class members.

What is the significance of a leading underscore anyway? I see you have some public member functions also starting with an underscore, so it definitely doesn't mean "private", otherwise you could have made those functions explicitly private.

Incorrect error handling

I see a lot of information messages, warnings and error messages all being written to std::cout. However, all this should go to std::log and std::cerr as appropriate.

Furthermore, if you encounter an error, you must do something about it. Just printing something like "Already assigned path!\n" is not enough; the program will happily continue running, the user looking at the output thinks "What path? Is this good or bad? I'll just ignore it." Consider throwing an exception; either this will cause the program to abort, or the caller can catch it if it has some way to recover from this error. If you really can't use exceptions, use some other way to panic or to return the error to the caller somehow.

This is unnecessarily complex

There are several issues with this design that make it unnecessarily complex. Let's start with the levels and owners. Why have these at all? Why split the command path into its components, and then have a std::unordered_multimap sorted by components, when you can just have a map ordered by the full request path? I think you can greatly simplify things by writing:

class SystemInterface
{
    std::unordered_map<std::string request, CallbackSignature> requestsMap;
    …
    void addRequest(const std::string& path, CallbackSignature cbf) {
        …
    }
};

Another issue are the guarders. It might seem like this is convenient, so the callback functions don't have to validate the payload. But you could easily rewrite the guarders so the callback functions can call them, such that the code is just as short as with your solution. Consider:

interface.addRequest("logger/level", [this](auto& ctx, auto& pload) {
    …
    if (auto plevel = GetNumber(pload[0])) {
        level = *plevel;
    }
    …
});

Where you created helper functions to deal with the data encapsulated in BasicReferenceData:

std::optional<int> GetNumber(const BasicReferenceData& data) {
    if (/* checks to see it data actually contains a valid number */) {
        return std::atoi(std::get<std::string_view>(data));
    } else {
        return std::nullopt;
    }
}

With C++23 you will get std::expected<>, so you can also have it return a more specific error code. Your RequestReturn type is a bit similar in that it holds some desired data and a possible error code, but it's hardcoded to return ConcreteData, which still requires the caller to further unpack that, and it doesn't have any of the handy features of std::optional and std::expected, like conversion to bool returning if it has an expected value or not, as well as monadic operations since C++23. If you cannot use that yet but still want to return something like RequestReturn, consider making that a proper class that emulates std::expected

Anyway, with these changes, SystemInterface is greatly simplified: it mostly is a std::unordered_map with some access control. help() no longer needs to flatten anything, as the paths are already stored flattened.