NFD » ndn-cxx

This issue blocks feature-keychain branch merge because it's going to introduce backwards-incompatible API changes. Merging them together can break applications one less time.

As part of https://gerrit.named-data.net/#/c/3531/, I have added additional parameters to callbacks, which look like

typedef function<void(const Data& data,
                      ValidationError errorCode, const std::string& errorMsg)> DataValidationFailureCallback;

typedef function<void(const Interest& interest,
                      ValidationError errorCode, const std::string& errorMsg)> InterestValidationFailureCallback;

Which in the code will be used as

callback(ValidationError::INVALID_SIGNATURE, "some explanation");

Junxiao suggested to change this to a class. I was trying to implement it, but couldn't figure out how to use enum class in that implementation that doesn't create crazyness when calling callback. Here is my attempt:

class ValidationError
{
public:
  /**
 * @brief Validation error code
 * @sa specs/validation-error-code.rst
 */
  enum class Code {
    OK                   = 0,
    INVALID_SIGNATURE    = 1,
    NO_SIGNATURE         = 2,
    CANNOT_RETRIEVE_CERT = 3,
    EXPIRED_CERT         = 4,
    LOOP_DETECTED        = 5,
    MALFORMED_CERT       = 6,
    REACHED_STEP_LIMIT   = 7,
    INVALID_KEY_LOCATOR  = 8,

    IMPLEMENTATION_ERROR = 255
  };

public:
  /**
   * @brief Validation error, implicitly convertible from ValidationError::Code
   */
  ValidationError(ValidationError::Code code, const std::string& info="")
    : m_code(code)
    , m_info(info)
  {
  }

  ValidationErrorCode
  getCode() const
  {
    return m_code;
  }

  const std::string&
  getInfo() const
  {
    return m_info;
  }

private:
  ValidationErrorCode m_code;
  std::string m_info;
};

Look kind of OK, until you see what to do when calling the callback

callback(ValidationError(ValidationError::Code::EXPIRED_CERT, "some message"));

Even if I change to just enum, it would look like

callback(ValidationError(ValidationError::EXPIRED_CERT, "some message"));

Any other way to make it nicer?

Can you use aggregate init? (don't provide a constructor for ValidationError, it can be a simple POD) Then the call site should become something like:

callback({ValidationError::Code::EXPIRED_CERT, "something went wrong"});

Answer to note-8:

This idea can be used together with "continuation". The continuation function accepts two parameters code and message, which internally constructs the ValidationError instance and pass to the public API's callback.

I will definitely not going to do what #12 proposes. The whole point for the introduction of such a class is to allow seamless extension of the parameters passed as an error. If all validator will suddenly need to be changed to support the new continuation, then there is no point of jumping hoops and invent data structures.

Davide Pesavento wrote:

Can you use aggregate init? (don't provide a constructor for ValidationError, it can be a simple POD) Then the call site should become something like:

callback({ValidationError::Code::EXPIRED_CERT, "something went wrong"});

Hmm. This could be workable, but I will then change to non-class enum:

callback({ValidatiorError::EXPIRED_CERT, "xxx");

Two questions (I can try myself later):

• will callback({ValidationError::EXPIRED_CERT}) will work too?
• kind of the same question, will adding additional variable to ValidationError cause old code to break?

Reply to note-13:

The continuation can be declared as a class with multiple operator() overloads, each is capable of accepting one set of parameters.

class ValidationError
{
public:
  enum Code {
  };

  ValidationError(Code code, const std::string& info = "");

  ValidationError(Code code, const Name& unavailableCert);
}

template<typename Packet>
class ValidationContinuation
{
public:
  template<typename ...Args>
  void
  operator(const Args&... args) const
  {
    m_callback(m_packet, ValidationError(args...));
  }

private:
  const std::function<void(const Packet& pkt, const ValidationError& err)>& m_callback;
  const Packet& m_packet;
};

Alex Afanasyev wrote:

Two questions (I can try myself later):

• will callback({ValidationError::EXPIRED_CERT}) will work too?

It should. It's basically the same as writing callback({ValidationError::EXPIRED_CERT, {}}).

• kind of the same question, will adding additional variable to ValidationError cause old code to break?

No, as long as new members are added after all existing members and are not reference types. Members that are not explicitly initialized are initialized by empty lists according to list-initialization rules (since C++14, their default initializers are considered first if present, but we can't rely on this behavior yet). This means that newly added members must also be of non-class types, or aggregates, or non-aggregate classes with a default constructor.

I will go with Davide's suggestion of ValidationError as a POD. Seem to be simpler and covers the intended use cases.