Project

General

Profile

Actions

Feature #1872

closed

Validator: error code

Added by Alex Afanasyev over 9 years ago. Updated almost 7 years ago.

Status:
Closed
Priority:
Normal
Category:
Security
Target version:
Start date:
Due date:
% Done:

0%

Estimated time:

Description

OnDataValidationFailed callback currently has only std::string parameter that explains the error. We should define a set of "standard" errors and add error code, as further processing can differ in different cases.

Examples:

  • failure because violates trust model
  • failure because key cannot be fetched ...

Related issues 2 (1 open1 closed)

Related to ndn-cxx - Feature #3223: Validator should give some details in error messagesClosed

Actions
Blocks ndn-cxx - Feature #3412: SegmentFetcher: onError signal should report more specific failure informationNew

Actions
Actions #1

Updated by Yingdi Yu over 9 years ago

  • Status changed from New to Code review
  • % Done changed from 0 to 100
Actions #2

Updated by Alex Afanasyev over 8 years ago

  • Target version set to v0.5
Actions #3

Updated by Vince Lehman about 8 years ago

  • Blocks Feature #3412: SegmentFetcher: onError signal should report more specific failure information added
Actions #4

Updated by Junxiao Shi about 8 years ago

  • Tracker changed from Task to Feature
  • Subject changed from Add error code to OnDataValidationFailed callback to Validator: error code
  • Status changed from Code review to New
  • Start date deleted (08/16/2014)
  • % Done changed from 100 to 0

As discussed in 20160114 conference call, http://gerrit.named-data.net/1398 is to be abandoned, and Yingdi will restart the feature in feature-keychain branch.

Actions #5

Updated by Junxiao Shi almost 8 years ago

Actions #6

Updated by Junxiao Shi almost 8 years ago

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.

Actions #7

Updated by Junxiao Shi over 7 years ago

  • Related to Feature #3223: Validator should give some details in error messages added
Actions #8

Updated by Alex Afanasyev about 7 years ago

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?

Actions #9

Updated by Alex Afanasyev about 7 years ago

  • Status changed from New to Code review
  • Assignee changed from Yingdi Yu to Alex Afanasyev
Actions #10

Updated by Alex Afanasyev about 7 years ago

  • Target version changed from v0.5 to v0.6
Actions #11

Updated by Davide Pesavento about 7 years ago

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"});
Actions #12

Updated by Junxiao Shi about 7 years ago

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.

Actions #13

Updated by Alex Afanasyev about 7 years ago

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.

Actions #14

Updated by Alex Afanasyev about 7 years ago

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?
Actions #15

Updated by Junxiao Shi about 7 years ago

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;
};
Actions #16

Updated by Davide Pesavento about 7 years ago

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.

Actions #17

Updated by Alex Afanasyev about 7 years ago

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

Actions #18

Updated by Alex Afanasyev about 7 years ago

  • Blocks deleted (Task #3098: Merge KeyChain branch)
Actions #19

Updated by Junxiao Shi almost 7 years ago

  • Status changed from Code review to Closed

There is no open Change on Gerrit for this issue. Further ValidationPolicy work is being performed in #3920.

Actions

Also available in: Atom PDF