Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up DID Resolution section; enable testability via proof by construction #601

Merged
merged 34 commits into from
Feb 11, 2021

Conversation

msporny
Copy link
Member

@msporny msporny commented Feb 6, 2021

This PR is intended to address issue #549 by adding to the abstract functions discussed in the DID Resolution and DID URL Dereferencing sections and ensuring that all statements are testable using a "proof by construction". That is, all statements that return abstract data models are testable due to additional normative statements that effectively state: "anything that returns an abstract data model MUST be serializable in one of the representations". Previously, that statement didn't apply to didDocument, the metadata structure, some of the dereferencing values, etc. This PR fixes that.

The PR also adds at-risk markers to some of the newer features requested by individuals (but not necessarily broadly requested by the WG -- such as nextUpdate, nextVersionId, equivalentId, and canonicalId. There are also a few bugs fixed in this version (such as stating that a contentType must be specified for the resolve call -- that returns an abstract didDocument... which is clearly wrong).

There are a LOT of reformatting changes in this PR, it might be best to look at the rendered preview for this PR.


Preview | Diff

index.html Outdated
resolveRepresentation ( did, did-resolution-input-metadata ) <br>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;-&gt; ( did-resolution-metadata, did-document-stream, did-document-metadata )
</code></p>
resolveRepresentation(did, options) →
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we mean DID or DID URL?

Would prefer to see a note that clarifies the type... and explicitly states if path, query and fragment are allowed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolveRepresentation() takes a DID, not DID URL

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both the resolve* functions take a DID, the dereference function takes a DID URL. This is one of the most important differences between these functions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above ... did is correct here.

index.html Outdated
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;-&gt; ( did-resolution-metadata, did-document, did-document-metadata )
</code></p>
<pre title="Abstract functions for DID Resolution">
resolve(did, options) →
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we mean DID or DID URL?

Would prefer to see a note that clarifies the type... and explicitly states if path, query and fragment are allowed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it's possible to hyperlink to definitions here, that might help avoid any confusion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolve() takes a DID, not DID URL

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both the resolve* functions take a DID, the dereference function takes a DID URL. This is one of the most important differences between these functions.

Copy link
Member Author

@msporny msporny Feb 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, did isn't going to change in this PR (and possibly never going to change).

index.html Outdated
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;-&gt; ( did-url-dereferencing-metadata, content-stream, content-metadata )
</code></p>
<pre title="Abstract functions for DID Dereferencing">
dereference(didUrl, options) →
Copy link
Contributor

@OR13 OR13 Feb 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm, this answers my question about the other functions... but it feels odd that this abstract function is defined so far away from the other ones... wonder if we can define them all together in one place, and then address them one at a time like this PR is doing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're all in the same overall section, but resolution and dereferencing are two very distinct operations and so they're defined separately.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed with @jricher -- we're going to keep these two sections separate since they do two very different things.

With my Editor hat off -- I continue to assert that we don't need to define this behavior in a normative way -- the whole dereferencing section is weird -- it's the purview of software libraries that we don't need standards for. We don't standardize the software library interface that an XML parser should use to extract a subsection of an XML document (same for HTML documents). I don't know why we continue to insist that we do this for DID Documents.

</p>

<dl>
<dt>
did-url
didUrl
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here would be a good place to warn about query and path and their relatively poor utilization... or add some detail on why we kept them in the spec.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want such a warning, I think it should go into the DID URL Syntax section, not here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@OR13, I'm taking that as non-blocking feedback that is editorial and that can be added at a future point in time (during CR).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed.

index.html Outdated
<code>contentStream</code> SHOULD be a <a>resource</a> such as a <a href="">DID
Document</a>, <a href="#verification-methods">Verification Method</a>, or <a
href="#services">service</a> that is serializable in one of the conformant
<a>representations</a> obtained through the resolution process. If the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, is this true of dereferencing an image with location transparency?

For example:

did:example:123?service=images&relativeRef=/favicon.png

I expect the content type to be image/png not application/did+json....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence should say MAY, it was incorrectly changed to SHOULD in cbbca3b3.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@msporny ^ has this comment been addressed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is a MAY now.

If the dereferencing is unsuccessful, this output MUST be an empty <a
href="metadata-structure">metadata structure</a>.
metadata about the <code>contentStream</code>. If the <code>contentStream</code>
is a <a>DID document</a>, this MUST be a <a>didDocumentMetadata</a> structure as
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

implies that contentStream can be something other than a registered did document representation mime type... see https://github.com/w3c/did-core/pull/601/files#r571649879

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the dereferencing function can return something other than the did document itself.

index.html Outdated
The MIME type that the caller prefers for <code>contentStream</code>. The MIME
type MUST be expressed as an <a data-lt="ascii string">ASCII string</a>.
The <a>DID URL dereferencing</a> implementation SHOULD use this value to
determine the <a>representation</a> contained in the returned value if such a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we may mean contentType of the resource... and not want to use representation here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed a while ago.

Copy link
Contributor

@OR13 OR13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this feels like its heading in the correct direction.

Blocking feedback:

I am confused over the relationship between accept, contentType and representation and resource. I think we mean for DID URLs to be capable of dereferencing to resources other than DID Documents, yet we are not clearly saying that here... but we sorta allude to it.

I think the solution is to define the abstract functions all together:

resolve, resolveRepresentation and dereference

And describe the domain of accept and contentType for each.

I am asserting these abstract functions have different domains / ranges... for example:

  1. resolve does not produce a contentType...
  2. resolveRepresentation produces contentType of DID Document Representation...
  3. dereference produces contentType of IANA Registered MIME type (which includes did documents and images)...

Nonblocking feedback:

I think it would help to hyperlink the terms to their definition, and be more excplit about where path, query and fragment are allowed, required, or ignored.

@OR13
Copy link
Contributor

OR13 commented Feb 7, 2021

We may find it helpful to review / agree to #600 before attempting further changes here.

@peacekeeper
Copy link
Contributor

There are also a few bugs fixed in this version (such as stating that a contentType must be specified for the resolve call

@msporny Where does the current text state that?

index.html Outdated
Comment on lines 3768 to 3769
<a href="#did-resolution-metadata"></a>. If the resolution is successful this
structure MUST contain a <code>contentType</code> property containing the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only if resolveRepresentation() was called, not if resolve() was called. Why this change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this is a bug, will fix.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in af3a4fd.

index.html Outdated
Comment on lines 3804 to 3805
<code>didDocument</code> property. This metadata
typically does not change between invocations of the <code>resolveRepresentation</code>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change? didDocumentMetadata is returned both for the didDocument and for the didDocumentStream, returned by resolve() and resolveRepresentation() respectively.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bug, will fix.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5d96fb8.

index.html Outdated
these functions in any way. <a>DID resolver</a> implementations might map the
<code>resolve</code> and <code>resolveRepresentation</code> functions to a
these functions in any way. <a>DID resolver</a> implementations might map
this function to a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll put it back, this is a hold over from when I condensed down into a single function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8d9fe3b.

</p>

<dl>
<dt>
did-url
didUrl
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want such a warning, I think it should go into the DID URL Syntax section, not here.

index.html Outdated
<code>contentStream</code> SHOULD be a <a>resource</a> such as a <a href="">DID
Document</a>, <a href="#verification-methods">Verification Method</a>, or <a
href="#services">service</a> that is serializable in one of the conformant
<a>representations</a> obtained through the resolution process. If the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence should say MAY, it was incorrectly changed to SHOULD in cbbca3b3.

@peacekeeper
Copy link
Contributor

@msporny thanks for this new PR, I agree with a lot of changes, but am also confused by some others. Maybe it would be simpler to break this up into multiple PRs so they are easier to review?

  • I agree with the clarification that says the didDocument (abstract data model) returned by resolve() can be transformed into a conforming DID document (representation). This solves the testability problem, right?
  • I agree with the change from hyphens (did-document) to camelCase (didDocument).
  • I agree with the at-risk markers
  • I probably agree with renaming input metadata to options (but this could be done in a separate PR)
  • I don't understand which bugs you are referring to, and I don't understand several smaller changes in the PR (see inline comments).

index.html Outdated
resolveRepresentation ( did, did-resolution-input-metadata ) <br>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;-&gt; ( did-resolution-metadata, did-document-stream, did-document-metadata )
</code></p>
resolveRepresentation(did, options) →
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above ... did is correct here.

index.html Outdated
Comment on lines 3768 to 3769
<a href="#did-resolution-metadata"></a>. If the resolution is successful this
structure MUST contain a <code>contentType</code> property containing the
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this is a bug, will fix.

index.html Outdated
Comment on lines 3804 to 3805
<code>didDocument</code> property. This metadata
typically does not change between invocations of the <code>resolveRepresentation</code>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bug, will fix.

index.html Outdated
these functions in any way. <a>DID resolver</a> implementations might map the
<code>resolve</code> and <code>resolveRepresentation</code> functions to a
these functions in any way. <a>DID resolver</a> implementations might map
this function to a
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll put it back, this is a hold over from when I condensed down into a single function.

index.html Outdated
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;-&gt; ( did-url-dereferencing-metadata, content-stream, content-metadata )
</code></p>
<pre title="Abstract functions for DID Dereferencing">
dereference(didUrl, options) →
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed with @jricher -- we're going to keep these two sections separate since they do two very different things.

With my Editor hat off -- I continue to assert that we don't need to define this behavior in a normative way -- the whole dereferencing section is weird -- it's the purview of software libraries that we don't need standards for. We don't standardize the software library interface that an XML parser should use to extract a subsection of an XML document (same for HTML documents). I don't know why we continue to insist that we do this for DID Documents.

</p>

<dl>
<dt>
did-url
didUrl
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@OR13, I'm taking that as non-blocking feedback that is editorial and that can be added at a future point in time (during CR).

@w3c w3c deleted a comment from jricher Feb 8, 2021
Copy link
Member

@TallTed TallTed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after changes suggested above

If the dereferencing is unsuccessful, this output MUST be an empty <a
href="metadata-structure">metadata structure</a>.
metadata about the <code>contentStream</code>. If the <code>contentStream</code>
is a <a>DID document</a>, this MUST be a <a>didDocumentMetadata</a> structure as
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the dereferencing function can return something other than the did document itself.

Comment on lines +4292 to +4275
definitions use strings for values. The entire metadata structure MUST be
serializable in a conforming <a>representation</a>.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this additional requirement added? The implementation might do any number of things to serialize the metadata separately from the document itself. As long as it gets in and out of the map, that's fine. By requiring the metadata to re-use the type and data structure system it inherits this property anyway.

Suggested change
definitions use strings for values. The entire metadata structure MUST be
serializable in a conforming <a>representation</a>.
definitions use strings for values.

Copy link
Member Author

@msporny msporny Feb 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the implementation might do any number of things -- but we can't test "any number of things" -- we need to test a "specific thing". The text was added to provide something specific we can test for... it doesn't say you have to serialize it... it says it "must be serializable" -- that is, you must be able to do it (and the test suite will test that by requesting a serialized form). Implementations aren't required to serialize, just ensure that it is possible to do so.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no such thing as "a conformant representation" for metadata. The representations are defined in terms of the did documents. This requirement does not make sense.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we wanted to be able to return metadata as cbor, json, yaml, etc... are we saying we want to be able to return a json did document with cbor meta data?

My understanding was that the entire response values would be in the requested representation... so if you asked for json, you would not get back cbor... but perhaps you see this as way to return response data in http codes / headers... which will never be JSON / CBOR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metadata is not ever defined in terms of JSON, CBOR, or anything. It's a data structure (a map).

The ability to call the resolve function for a specific platform is going to require some way to get the map values in and out. We specify this in terms of the data structure -- map -- and not in terms of a serialization for that data structure. The method of passing that data structure in and out is the binding of the abstract function to a specific implementation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounds like what you're writing here is a requirement for hooking functions to the test harness, and not requirements for the function itself. This is a dangerous line to cross.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no such thing as "a conformant representation" for metadata.
Metadata is not ever defined in terms of JSON, CBOR, or anything. It's a data structure (a map).

Here's the problem that we continue to have with the Resolution section:

The original authors intend an unspecified subset of it to be a purely abstract thing while simultaneously wanting there to be normative language related to the abstract concepts. While it is true that we can write normative statements about abstract concepts, it has been asserted that doing so is less useful than being able to have testable statements. This point has been made repeatedly throughout the past decade or more at W3C, across multiple Working Groups, with repeated consensus positions reaffirming that groups shouldn't write normative language around abstract concepts that are not realizable in concrete tests.

In other words, we cannot test abstract things in the test suite if they are never realized as something concrete. It is problematic to have normative language on abstract concepts because it's very easy for people to make subjective assertions about their conformance to those statements. When you don't make things testable, you harm interoperability.

We have three options that were explored:

  1. Migrate the entire Resolution section into a NOTE.
  2. Remove normative language that is associated with abstract concepts/statements.
  3. Make the normative language around abstract concepts/statements testable by providing a path to a concrete expression of those abstract concepts/statements.

The third item seemed to be the one that had the best chances of achieving consensus.

In order for that to happen, we need a proposal that enables us to get the metadata into a concrete form so the test suite can test it. One approach is to say that "the metadata structure MUST be serializable in a conforming representation". If we look at the JSON production rules, it doesn't say anything about production ONLY applying to DID Documents... it just states that "if you have data type X, then you can serialize by doing Y." ... and if we can do that with the metadata structure, then we have a path to testability.

I'm also open to any other paths to testability.

At this point, @jricher, we need a counter-proposal on how we get the metadata structure to testability, or we need you to clarify if you are objecting or if you can live with the language.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not going to formally object to anything in here, but I am on record in saying that this is a bad idea that makes for a weak specification.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In lieu of a counter-proposal that can achieve stronger consensus, I'm hearing that you won't object to keeping the language as it stands.

msporny and others added 21 commits February 10, 2021 16:55
Co-authored-by: Markus Sabadello <[email protected]>
Co-authored-by: Amy Guy <[email protected]>
Co-authored-by: Markus Sabadello <[email protected]>
Co-authored-by: Justin Richer <[email protected]>
@msporny msporny force-pushed the msporny-issue-549-construction-testability branch from ee76e43 to a58ae7f Compare February 10, 2021 21:57
@jricher
Copy link
Contributor

jricher commented Feb 10, 2021

@msporny My concerns have been noted and discussed, and I disagree with the conclusions and solutions to those concerns that are part of this PR. That said my opinion is just one of many in the working group, and so I am not attempting to block this pull request with just my own stance, regardless of how and why I disagree.

@msporny
Copy link
Member Author

msporny commented Feb 10, 2021

@msporny My concerns have been noted and discussed, and I disagree with the conclusions and solutions to those concerns that are part of this PR. That said my opinion is just one of many in the working group, and so I am not attempting to block this pull request with just my own stance, regardless of how and why I disagree.

Acknowledged, thank you.

I am going to pull this PR in with the following caveat -- we still have a few weeks left to improve this section if anyone has a better idea on how to strike the abstract/testable balance. There are also legitimate concerns about the Representation sections and how they apply to some of the language in this PR that needs to be addressed; are they about serializing DID Documents only, or any data expressed using the ADM? We're almost certainly headed for a 2nd PR... I've never seen a specification with 130+ machine-testable normative statements get it right the first time -- but we're going to try. This PR addresses all the outstanding testability concerns with the Resolution section, but in a way that causes almost all of us heartburn... so if anyone has any better ideas in the meantime, please feel free to raise a PR -- the worst that can happen is that the group decides to not take it up before CR.

@msporny
Copy link
Member Author

msporny commented Feb 11, 2021

Normative, multiple reviews via #587, #591, #592, and #601, changes requested and made, remaining objections are non-blocking, merging with caveat in #601 (comment).

@msporny msporny merged commit df95e21 into main Feb 11, 2021
@msporny msporny deleted the msporny-issue-549-construction-testability branch February 21, 2021 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants