NFD » ndn-cxx

Please use markdown syntax next time. Code blocks should be indented 4 spaces

The proposed refactoring is a little bit weird and I don't see much value in doing it. We could go further and do more logical separation. I'll upload a picture of what I'm thinking

I can recommend finalizing UML design first. Here is the file I got: https://dl.dropboxusercontent.com/u/45347685/uml/pib.vpp (I'm using VisualParadigm software http://www.visual-paradigm.com/)

Did my previous note show up?

the only note from you is note-7

1. why getPibLocator, getTpmLocator, setTpmLocator are the operations of Identities rather than Pib?

2. what does the generate method do in Keys?

3. what does add method actually do in Identities? do we create an identity and then add it? how to create an identity?

4. we can rename doesExist as has

Michael, can you post a JPG of that last diagram? I want to see if Yingdi's questions are answered from #10. I have some other questions (based on Alex's JPG):

1. Shouldn't there be a setDefault() on Keys to set the default Certificate?
2. Do begin() and end() actually have something to do with building the object or do they just mean first()/last()?

And let me respond to your questions:

Q:Shouldn't there be a setDefault() on Keys to set the default Certificate?
A: Fixed, see new chart.

Q: Do begin() and end() actually have something to do with building the object or do they just mean first()/last()?
A: If the question is where you can use the iterators to insert new Identities, no. If the question is where you can use iterators to access an Identity and then "build" it by adding keys and things, the iterator should provide access to the underlying Identity, and I see no reason it could not be modifiable reference (i.e const_iterator, iterator) idiom.

Thanks; I still don't understand begin() and end()... can you explain what they are there for? What happens when you call one of them?

A couple of other questions:

• Does Identity extend Name?

• Can we change 'doesExist' either to 'contains' (the traditional Java collection naming) or 'has' (Yingdi's suggestion)? I think 'contains' describes the method better... personal opinion.

• I have a hunch that getDefault()/setDefault() will cause less confusion as operations you do on the parent object and not on the collection (e.g. on Key instead of Certificates). If developers have to write ...getKey().getCertificates().getDefault() they will assume that the underlying code retrieves all certificates and then finds the default out of the list; this would be confusing because the most likely implementation is to have the default certificate ID as a foreign key on the Key row and retrieve the default key with one database operation. I would pull those up a level (e.g. getKey().getDefault()).

• I am assuming Key and Certificate will have methods similar to the ones the currently have (to retrieve DERs, etc.)?

Does Identity extend Name?

I do not believe it would. It likely will have a Name but not necessarily be a Name.

Can we change 'doesExist' either to 'contains' ...

Sure contains works for me.

I have a hunch that getDefault()/setDefault() ...

I do not have an issues with this, it seems the logic makes sense either way.

I am assuming Key and Certificate will have methods similar to the ones the currently have (to retrieve DERs, etc.)?

I do not believe so, but let me check with Yingdi.

Makes sense; so Identity will have a getName() so that we can inspect it.

Re: Key/Certificate-specific methods, you may want to look at https://github.com/named-data/jndn/blob/master/src/net/named_data/jndn/security/certificate/Certificate.java; Jeff has a lot of code in there and I don't know whether it needs to stay or go in the new API. I'll ping him to weigh in.

My code for Certificate.java follows the ndn-cxx interface closely: https://github.com/named-data/ndn-cxx/blob/3e8b52e5bd948a58d04d67dc9ed49dc7f9383233/src/security/certificate.hpp .

So I guess Andrew is asking: If you are specifying the new API, should it specify the details of the Certificate API such as addSubjectDescription(), etc.?

Andrew Brown wrote:

Thanks; I still don't understand begin() and end()... can you explain what they are there for? What happens when you call one of them?

Michael, I was just parading my C++ iterator ignorance; please disregard.

I do not think I will be touching the existing Certificate type, if I there is some additional functionality that we want to add to Certificate (I do not believe there is) then I would likely make a new issue for that since it is a bit outside the scope of the discuss for these aggregations.

So I think the get(), etc in the Certificates container would likely just return instances of the existing Certificate type.

Just try to catch up the discussion here.

1. we should follow the naming convention of each language. Take doesExist as an example, for c++, find could be better given the class is designed as a container. Moreover, find provides the functionalities of both doesExist and get, so we do not need to define the later two. For java, it can be named as corresponding one of the standard java containers. (@Michael, it seems that there are still some inconsistency in the diagram, some container has the method find while others have doesExist.)

2. for the getDefault/setDefault, Andrew made a good suggestion, I just wonder what is the return type, an iterator of the container? Another question is that if we move these methods out of the container, would it be better to give them a more specific name, such as getDefaultIdentity().getDefaultKey().getDefaultCertificate()?

3. for certificate, we do not create a separate one but just reuse the existing one. However, the existing one also needs certain changes to reflect the new certificate format. For example the addSubjectDescription is no longer needed, as it is removed from the new certificate format.

Michael, sounds good. Yingdi, all three points sound good.

Please address following issues:

1. The set/getDefaultKey should be member methods of Identity.

2. The set/getDefaultCertificate should be member methods of Key.

3. What is Keys:generate()? what is the difference between generate and add?

4. Why there is no remove in Certificates?

I won't review UML because it doesn't contain complete information (eg. parameter types and return types).

Please submit header files to Gerrit, and I can have a look.

Comments on commit:edc0bec1c8aab1f6594672303c3577789257e533

The design strictly enforces the hierarchy of identity-key-certificate, which implies the hierarchical trust model, and contradicts with ndn-cxx's goal of supporting multiple trust models.

The plural typenames Identities Keys Certificates are confusing.
They look similar to corresponding singular typenames, which can cause mistakes.
It's better to use more explicit typenames such as IdentityContainer, etc.

Pib:::getDefaultIdentity and Identity::getDefaultKey return mutable references.
If I copy-assign another Identity/Key to the return value of these functions, is the default Identity/Key changed?
If yes, delete setDefaultX method; otherwise, return constant references.

The category of each iterator should be declared (and asserted with a concept check).

Why is ndn::Certificates::Certificate type nested?

Why is ndn::Certificates::iterator dereferenced to Key?

Junxiao Shi wrote:

The design strictly enforces the hierarchy of identity-key-certificate, which implies the hierarchical trust model, and contradicts with ndn-cxx's goal of supporting multiple trust models.

The trust model is created through the namespace of the Identity, which this does not deal with. This merely provides organization for Identities, the Keys for these Identities, and the Certificates for these Keys. A separate concern.

Junxiao Shi wrote:

The category of each iterator should be declared (and asserted with a concept check).

I very well understand that the final design for the iterator will be far more fleshed out, the version shown here is just a stand-in to allow for review of the design of the container. The category of the iterator will depend on the support of the underlying infrastructure. Clearly it is currently a ForwardIterator, which is a lower bound on the functionality required.

Your remaining comments I believe I implemented.

Ok, after speaking to Yingdi, and determining that the iterator will far more likely than not be a ForwardIterator, I went ahead and defined it was such.
I also made an effort to more correctly style the code as well as resolving some issues noticed by Yingdi and more that I myself had missed.

Just upload a new abstraction and implementation, please check it out: http://gerrit.named-data.net/1862. If we agree with the new change, then we will revise the description of this task.

Design questions/problems in commit:eff2b849f1b6b4ab45ee032c824e2c924e1f417a

Pib constructor¶

Why is Pib constructor protected? How to obtain a Pib instance in a normal program?

Identity constructor¶

Identity does not have a public constructor; Pib::getIdentity returns Identity and can potential throw.

Suppose a program wants to wrap Pib::getIdentity in a try-catch, and also wants to minimize the scope of try block (which is a good practice).

The following snippet is natural, but won't compile due to the lack of Identity public constructor:

bool
f()
{
  Identity identity;
  try {
    identity = pib.getIdentity(...);
  }
  catch (PibImpl::Error&) {
    return false;
  }
  // work with identity
  return true;
}

And the programmer either has to use a pointer:

bool
f()
{
  Identity* identity = nullptr;
  try {
    identity = &pib.getIdentity(...);
  }
  catch (PibImpl::Error&) {
    return false;
  }
  // work with identity
  return true;
}

or expand the scope of try block:

bool
f()
{
  try {
    Identity identity = pib.getIdentity(...);
    // work with identity
    return true;
  }
  catch (PibImpl::Error&) {
    return false;
  }
}

Same problem occurs with Key constructor and Identity::getKey function.

separate container type¶

Why not use Pib as a container of Identitys, Identity as a container of Keys, Key as a container of Certificates?

container size¶

XContainer should have a size_t size() const getter.

Depending on the implementation, it could be more efficient than std::distance(container.begin(), container.end()).

iterator¶

XContainer::iterator should be declared as an alias of XContainer::const_iterator.

It's okay to have iterator as a constant iterator (rather than a mutable iterator). STL std::set<...>::iterator is a constant iterator since C++11.

Pib constructor¶

A: Pib instance should be created by KeyChain and stays in KeyChain. User can obtain a reference to the Pib instance through a KeyChain method.

Identity constructor¶

A: The reason of not exposing the constructor is that we do not want users to be aware of the backend implementation and also do not want users to manipulate backend data set directly through a constructor. But for the problem you mentioned, I think making a public default constructor should be fine. The default constructor will set the backend pointer to nullptr, so it won't be useful until it obtained an actual instance from getIdentity.

separate container type¶

A: I did this according to the original UML diagram. I am okay with merging them together, but the syntax may look weird

identity.begin() // actually return a iterator to key...

container size¶

A: We can add a size method.

iterator¶

A: We can add

typedef const_iterator iterator

Pib constructor¶

KeyChain will be a friend class of Pib. I am not sure if there any sub-class of Pib, that's why I mark its constructor protected. I can mark it as private for now, and if we really need to have a sub-class (though very unlikely), we can mark it as protected.

Identity constructor¶

After adding a default constructor, we also need operator! to test whether the instance is invalid.

Calling any method on an invalid instance should throw an exception (not assertion).

How to use the operator!? Are you saying that every user of the Identity instance must check if it is valid before using it? Would that be too cumbersome?

Pib constructor¶

Add KeyChain as a friend now.

Since Pib could have subclass, it's fine to keep the constructor as protected.

Identity constructor¶

To ensure semantic correctness and prevent misuse, it's important to throw an exception if some method is called on an invalid instance.

The proper type of exception is std::domain_error.

It's useful to provide Identity::oprator! in case some library function wants to ensure an Identity is valid without triggering or catching exceptions.

To test for valid instance, a caller can write if (!!identity) { ... }.

If the caller is sure that an Identity instance is valid, it doesn't need to use the conditional.

Junxiao Shi wrote:

Pib constructor¶

Add KeyChain as a friend now.

Since Pib could have subclass, it's fine to keep the constructor as protected.

I do not think friend relationship can be inherited, so should we allow subclass? If so, then we require every subclass to declare KeyChain as a friend. This seems to be a bad idea.

I also realized that a particular Pib instance is explicitly created by KeyChain. Compared to creating a subclass of PibImpl and pass it to the Pib constructor directly, creating a subclass of Pib seems unnecessary. In this way, we only need to change the Pib constructor to accept one more parameter shared_ptr<PibImpl>, and mark KeyChain as a friend of Pib.

Identity constructor¶

To ensure semantic correctness and prevent misuse, it's important to throw an exception if some method is called on an invalid instance.

The proper type of exception is std::domain_error.

ok

It's useful to provide Identity::oprator! in case some library function wants to ensure an Identity is valid without triggering or catching exceptions.

To test for valid instance, a caller can write if (!!identity) { ... }.

This seems a little bit weird. Is there any problem with operator bool?

Pib constructor¶

friend relation is not inheritable. If KeyChain decides to subclass Pib, MyPib could have a public constructor.

Otherwise, I agree with having Pib constructor private, and accepting PibImpl (smart) pointer as argument.

Identity constructor¶

Typically both operator! and operator bool (implicit conversion) are provided.

PibImpl::Error¶

Many methods can throw PibImpl::Error, where PibImpl is an implementation detail. We should declare Pib::Error and use that in docs, and make PibImpl::Error a subclass of Pib::Error if necessary.

Ok, I will make these changes.

Design problem in commit:3b0f4398b0d3d9eccf775382b285bd98757f836f :

Many PibImpl functions, such as .addIdentity, has noexcept specifier and also returns void.

This may lead difficulty in implementing a PibImpl that relies on an external storage such as a file or a MySQL database.

Suppose said external storage has failed (disk is full or MySQL server is down), it's semantically incorrect to return normally from .addIdentity because the identity isn't added, but the function is forbidden from raising an exception.

The only remaining choice is to invoke std::terminate, but this is not always desirable.

Recommendation:

• Drop noexcept specifier.
• Semantic errors (such as calling .setDefaultKeyOfIdentity with a nonexistent key) throw Pib::Error (not PibImpl::Error).
• Underlying errors (such as inaccessible MySQL server) throw PibImpl::Error, which is not a subclass of Pib::Error.

Then basically every method may throw PibImpl::Error, should we state that for each method, or generally state that in the class description? Also, who should catch the error?

• PibImpl itself does not throw and requires no \throw PibImpl::Error

• PibImpl::Error should say:

  \brief represents a non-semantic error
  
  A subclass of PibImpl may throw a subclass of this type when
  there's a non-semantic error, such as a storage problem.
  

• Pib::Error should say: \brief represents a semantic error

• Every method in Pib/Identity/Key needs \throw PibImpl::Error error in underlying PibImpl if it invokes a PibImpl method

• Pib/Identity/Key does not catch PibImpl::Error

My question is: who is expected to catch PibImpl::Error?

who is expected to catch PibImpl::Error?

Not the library. Caller of the library, ie the app, MAY catch PibImpl::Error.