Enforce non-empty subject for VCDM v2

[sbihel]

As described in the specs https://www.w3.org/TR/vc-data-model-2.0/#credential-subject

[timothee-haudebourg]

There is a difference between "A verifiable credential must have a credentialSubject property" and "the credentialSubject property must not be empty". So do we really need to ensure it is non-empty? Or just require it is present?

[timothee-haudebourg]

I think some function names may be problematic like into, but most importantly the Clone can be removed (almost?) everywhere.

[timothee-haudebourg]

We don't want the Clone bound here. As you can see it propagates everywhere.

[sbihel]

Yeah, but I'm not sure where to put it?

[timothee-haudebourg]

Why do you need it exactly? I couldn't see it

[sbihel]

52ee9d9 I moved the bound to serde bounds which was necessary otherwise the derive of serde traits was failing

[timothee-haudebourg]

Can you try to implement Serialize by hand? This is really weird, there is no reason for serde to require Clone here 🤔

[sbihel]

Implementing Serialize manually help, but it didn't for Deserialize

[timothee-haudebourg]

That will clash with the Into::into function, where the intent may be to convert a NonEmptyVec into a Vec. So I would suggest finding another name for this, like map (although this is usually for functions accepting a mapping function as parameter), or cast.

[sbihel]

Yes. Considering Vec doesn't itself implement such functions, I've decided to remove them in favour of implementing IntoIter so a consumer can easily do the same themselves

[timothee-haudebourg]

Can you also add impl<'a, T> IntoIterator for &'a mut NonEmptyVec<T> {}?

[sbihel]

There is a difference between "A verifiable credential must have a credentialSubject property" and "the credentialSubject property must not be empty". So do we really need to ensure it is non-empty? Or just require it is present?

You're right, but I think simply requiring it to be present doesn't make much sense because:

• if it's an empty object, then that's not allowed (which we already check with NonEmptyObject)
• if it's an empty array, then I don't think it's in the spirit of the specs because it's only using arrays when there more than one item, and "A verifiable credential contains claims about one or more subjects." confirms this IMO

But I'll defer to your judgement

[timothee-haudebourg]

I don't think it's in the spirit of the specs

I would tend to agree? I think? It's really hard to tell. If we stick to the JSON-LD interpretation, even if the value is a single object and not an array, then JSON-LD will expand it into an array. But then if this array is empty the RDF deserialization algorithm won't generate a triple with the https://www.w3.org/2018/credentials#credentialSubject property, which may be what they really mean by "must have a credentialSubject property".

Just to understand what we're loosing here: I can see some situations where the app adds the credential subjects to an existing credential like this:

fn populate_credential(cred: &mut JsonCredential) {
  cred.credential_subjects.push(stuff)
}

This pattern would not be possible if we forbid empty credential subjects at the type-system level. And also its easier to implement, just by changing

#[serde(
    with = "value_or_array",
    default,
    skip_serializing_if = "Vec::is_empty"
)]
pub credential_subjects: Vec<S>,

into

#[serde(with = "value_or_array")]
pub credential_subjects: Vec<S>,

With that in mind, I let you choose if you want to proceed, I convinced myself that I don't have a strong opinion here. Just remove the Clone bound if you go ahead 😄

[sbihel]

I've decided to move forward with this because I think it would be too easy to make a mistake otherwise (and the API already requires a subject at initialisation time)