3
\$\begingroup\$

Well, it's not actually a schema validator but anyway...

Motivation

I'm writing a library which is a UI to work with some electronics - modules. Each real module is represented by a corresponding class in the library. I've decided to add the ability to program a module at once using the config entity. Also it should be possible to read the current configuration from a module. Also I want a user could save (load) config to (from) file on disk, and that data should be human readable and editable. So I've chosen death JSON as a config entity.

So my classes have the following member functions:

void ReadConfig( json& config );
void WriteConfig( const json& config );

As an advantage I have a polymorphic object here : I don't need many different configs for each class.

As a disadvantage well ... the same. Having such a polymorphic object makes it impossible to check actions with wrong configs at compile time, for example, passing the discriminator config to program an ADC.

Solution

I've decided that any configurable module must have a default config (some kind of schema) which values, if any, are nulls. For example:

{
  "name": "V2718",
  "settings": {
    "inputs": [
      {
        "led_polarity": null,
        "polarity": null
      },
      {
        "led_polarity": null,
        "polarity": null
      }
    ],
    ...
}

So the config is correct if and only if it can be obtained from the default one using only replacements of nulls. How do I check this? The answer is JSON Patch.

Code

Here is the code (see the Validate member function). Each module-class inherits from the UConfigurable abstract class. Of course, I could provide code for the Validate function only, but I think the whole header is more consistent.

UConfigurable.h

#ifndef V_PLUS_CONFIGURABLE_H
#define V_PLUS_CONFIGURABLE_H

#include <nlohmann/json.hpp>
#include <iostream>


namespace vmeplus
{
    using json = nlohmann::json;

    // Curiously recurring template pattern
    template <typename T>
    class UConfigurable
    {
        protected :
            static json   fDefaultConfig;

        public :
            UConfigurable() {};
            virtual ~UConfigurable() {};

            virtual void ReadConfig( nlohmann::json &config ) = 0;
            virtual void WriteConfig( const nlohmann::json &config ) = 0;

            static json GetDefaultConfig() { return fDefaultConfig; }
            static bool Validate( const json& source );
    };

    template<typename T>
    bool UConfigurable<T>::Validate( const json& source )
    {
        bool verdict = true;
        json patch = json::diff( source, fDefaultConfig );
        for( auto it = patch.begin(); it != patch.end(); ++it )
        {
            // key "op" MUST be in any patch according to
            // https://datatracker.ietf.org/doc/html/rfc6902
            // and its value MUST be one of the following
            // "add", "remove", "replace", "move", "copy", or "test"
            if( it->at("op") == "replace" )
            {
                // if "op" is "replace" then there MUST be the "value" key
                if( not (it->at("value").is_null()) )
                {
                    verdict = false;
                    break;
                }
            }
            else
            {
                verdict = false;
                break;
            }
        }
        return verdict;
    }

    void WriteConfigToFile( const json& j, const std::string& path );

    json ReadConfigFromFile( const std::string& path );
}
#endif
\$\endgroup\$
5
  • 1
    \$\begingroup\$ Seems like you're sort of trying to re-invent JSON Schema. If you're interested primarily in the end result, maybe it would be easier to use this instead? \$\endgroup\$ Aug 16, 2021 at 20:34
  • \$\begingroup\$ @JerryCoffin, No, I'm not trying to re-invent JSON Schema. I knew about it when I was writing this post. Of course, JSON Schema would be more appropriate here, but then I would have to write a schema instead of a default config =) \$\endgroup\$
    – LRDPRDX
    Aug 17, 2021 at 14:49
  • 1
    \$\begingroup\$ I use ThorsSerializer to convert to/from JSON for storage. But while in the application it is a standard C++ object (thus you don't have to check for the correct type it is or it does not compile). Note: It supports polymorphic objects and strict validation if you need it out of the box. \$\endgroup\$ Aug 17, 2021 at 17:10
  • \$\begingroup\$ @MartinYork, looks cool ! Thank you. \$\endgroup\$
    – LRDPRDX
    Aug 18, 2021 at 7:43
  • \$\begingroup\$ I want to also mention the cereal library. \$\endgroup\$
    – LRDPRDX
    Sep 16, 2021 at 10:57

2 Answers 2

2
\$\begingroup\$
            if ( ⋯ ) {
                if ( ⋯ ) {
                    verdict = false;
                    break;
                }
            }
            else
            {
                verdict = false;
                break;
            }
        }
        return verdict;

Just return false; when you detect a failure case!
Then return true; if you reach the end.

You get:

            if ( ⋯ ) {
                if ( ⋯ ) return false;
            }
            else return false;
        }
        return true;
\$\endgroup\$
6
  • \$\begingroup\$ I prefer only one return in a function. \$\endgroup\$
    – LRDPRDX
    Aug 17, 2021 at 14:52
  • 2
    \$\begingroup\$ @LRDPRDX Thats very C like and not very C++ like. \$\endgroup\$ Aug 17, 2021 at 17:13
  • 3
    \$\begingroup\$ This is a great point. Using one return means you have more complex control flow and state (extra boolean variable). It's mostly a historical antipattern nowadays. See Where did the notion of “one return only” come from?. If the function is too complex, break it out into a subroutine such that it has fewer/one return, then the parent function can operate at a higher level with a single return as well. \$\endgroup\$
    – ggorlen
    Aug 18, 2021 at 4:08
  • 2
    \$\begingroup\$ @LRDPRDX "one return in a function" is an anti-pattern that is contrary to good C++. Instead you should "keep to the left", and avoid extra state variables and "dead flow" to drain out of the function when you're actually done. Look how much smaller it gets in this example: eliminate the break and you have a clean single-statement in a if/else branch so you get rid of the braces too, and you delete 8 lines. \$\endgroup\$
    – JDługosz
    Aug 18, 2021 at 15:12
  • 1
    \$\begingroup\$ @LRDPRDX I don't remember why My guess would be an obsolete coding standard you had to use when you were learning. That's why it's good to keep re-evaluating our patterns; the best practices change over a period of years, based on updates to the language and library as well as discussions within the C++ community. \$\endgroup\$
    – JDługosz
    Aug 19, 2021 at 14:41
2
\$\begingroup\$

Following on from @JDlugosz

I would simplify the code to:

// Why is this a template class!!!!
// I don't see any use of the type "T"?
// Could we move it to a base class that is not templated?
// or is this an artifact of simplifying the code for review?
template<typename T>
bool UConfigurable<T>::Validate( const json& source )
{
    json patch = json::diff( source, fDefaultConfig );

    // Use the (new*) range based for loop
    for(auto const& it: patch)
    {
        // key "op" MUST be in any patch according to
        // https://datatracker.ietf.org/doc/html/rfc6902
        // Therefore "op" must be "replace"
        if( it.at("op") != "replace" ) {
            return false;
        }

        // if "op" is "replace" then there MUST be the "value" key
        if( not (it.at("value").is_null()) ) {
            return false;
        }
    }
    // If it did not fail.
    // We are good and can return true.
    return true;
}
\$\endgroup\$
6
  • \$\begingroup\$ 1) Why CRTP ? At least for static json fDefaultConfig. \$\endgroup\$
    – LRDPRDX
    Aug 19, 2021 at 9:09
  • \$\begingroup\$ 2) I see your control flow is indeed simpler but less readable I think. Because the key "value" is guaranteed to exist only when the "op" is "replace". So I would prefer @JDlugosz's variant. \$\endgroup\$
    – LRDPRDX
    Aug 19, 2021 at 9:13
  • \$\begingroup\$ 1) It has a type json not T. If you have a default value based on T then make getting the default based on a virtual function in the base class. \$\endgroup\$ Aug 19, 2021 at 17:37
  • \$\begingroup\$ 2) Its a subjective call I suppose. I prefer mine as more readable, that's why I wrote it. But no major objection to the original. If I was the senior engineer on a team I would put a note in the code review but not require a change. \$\endgroup\$ Aug 19, 2021 at 17:39
  • \$\begingroup\$ 3) Yea. I messed up the comment. Fixed it. \$\endgroup\$ Aug 19, 2021 at 17:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.