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

Add representation-specific entry map in Production and Consumption. #679

Merged
merged 2 commits into from
Feb 22, 2021

Conversation

msporny
Copy link
Member

@msporny msporny commented Feb 19, 2021

This PR attempts to address @peacekeeper's long standing concern wrt. production and consumption and representation-specific entries by using the proposal for Option 3.3 in issue #664 (comment). This is also an attempt to get PRs #596 and #597 unstuck.

Specifically, it defines the input and return parameters for conforming producers and consumers and splits the representation-specific entries into their own map that can be serialized and returned along with the DID Document data model. It adds a requirement that all representations detect known representation-specific entries, which is required to cleanly create the two classes of information. The change is substantive.


Preview | Diff

@msporny msporny added the substantive This is a substantive change to the specification. label Feb 19, 2021
Comment on lines +2480 to +2482
A <a>conforming consumer</a> MUST detect any representation-specific
entry across all known <a>representations</a> and place the entry into a
representation-specific entries <a data-cite="INFRA#maps">map</a> which is
Copy link
Member Author

@msporny msporny Feb 19, 2021

Choose a reason for hiding this comment

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

I had played around with alternative language here that effectively said: "any representation specific entries that could appear in the representation" -- which basically boils down to just @context for the forseeable future... %YAML and xmlns are highly unlikely to appear in JSON and CBOR.

index.html Outdated Show resolved Hide resolved
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.

I would prefer to see abstract function signatures for consume and produce.

something like:

produce = (didDocument, representation-entries, representation-options) => didDocumentByteStream
consume = (didDocumentByteStream) => didDocument, representation-entries

I don't understand why we define abstract function for resolution, and then not for production and consumption.

representation-entries and representation-options need to be defined next to didDocumnt and didDocumentByteStream

If an issue is added pointing to this comment, I am happy to approve and do the PR myself.

index.html Show resolved Hide resolved
index.html Outdated
A <a>conforming producer</a> MUST indicate which <a>representation</a> has been
used for a <a>DID document</a> via a MIME type as described in <a
href="#did-resolution-metadata"></a>.
A <a>conforming producer</a> MUST return the MIME type <a
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
A <a>conforming producer</a> MUST return the MIME type <a
A <a>conforming producer</a> MUST return the Media Type <a

MIME Types became Media Types with RFC 1590 in 1994. (There are several, if not many, places this change is needed.)

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, I'm going to do this in a big spec-wide PR in one go. Rejecting your change suggestion, even though it's correct, want to bundle all those MIME type -> Media Type changes together /after/ the spec has settled.

Copy link
Member

Choose a reason for hiding this comment

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

Fair ... though I don't know what would be lost by doing some of these changes sooner, and think that having a mix of MIME & Media would be more likely to flag the need to replace the former than having all MIME would. I'll count on you having a list of such "will do these later in batches" changes.

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 d915bea.

index.html Outdated
href="#data-model">data model</a> using only the <a>representation</a>'s data
type processing rules.
A <a>conforming consumer</a> MUST take a <a>representation</a> and
MIME type <a data-cite="INFRA#string">string</a> as input into
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
MIME type <a data-cite="INFRA#string">string</a> as input into
Media Type <a data-cite="INFRA#string">string</a> as input into

or "media type" ... Capitalization may be important for clarity.

Copy link
Member Author

Choose a reason for hiding this comment

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

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 d915bea

index.html Outdated Show resolved Hide resolved
@msporny
Copy link
Member Author

msporny commented Feb 19, 2021

I don't understand why we define abstract function for resolution, and then not for production and consumption.

With my Editor's hat firmly on: We never should have defined abstract functions for resolution using our own bespoke abstract function language... I cannot underscore how terrible the half-measure that is the Resolution section is -- we're bending over backwards and doing really strange stuff there to half-define something. We should have either just kept all of it to a testable algorithm in prose, like https://www.w3.org/TR/json-ld11-api/#expansion-algorithm does... or actually defined a concrete set of functions using Javascript/Typescript/WebIDL. What we have is a weird half-measure and I'm reluctant to double-down on that mistake.

I did start to write what you suggested up initially, and then decided against it because...

representation-options

We have no way to realize this parameter in the way the Resolution section is written (which again, is strange and bespoke and isn't clearly specified for anything that is not INFRA->JSON... like, you know, CBOR). None of the data types in the data model or INFRA support it. Consider the use case of injecting encoded CBOR values into the root CBOR map... integers as keys -- can't do it... no concrete representation in INFRA for CBOR. So the best I could do is use prose to say that you can take other options and those options can be used to affect production. We're not testing MAY statements... that's the only way I could get to making representation-options a possibility.

It's not that it's impossible to write the spec up with bespoke abstract function language... but it makes it worse, IMHO, not better. All that said, I'll draft a PR that does the abstract function thing (even though I'm totally against the concept... what we have now is testable and flexible enough to support representation-options). If I write it up like we did for Resolution, representation-options are still not going to be a thing because we have no way of expressing them.

@msporny msporny force-pushed the msporny-cr-representation-entries branch 2 times, most recently from a9d8b1f to 33e2f67 Compare February 19, 2021 20:51
index.html Outdated Show resolved Hide resolved
Copy link
Member

@brentzundel brentzundel left a comment

Choose a reason for hiding this comment

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

I believe this PR includes changes from PR #678. Not sure if that was deliberate.

index.html Outdated Show resolved Hide resolved
@OR13
Copy link
Contributor

OR13 commented Feb 20, 2021

We should have either just kept all of it to a testable algorithm in prose, like https://www.w3.org/TR/json-ld11-api/#expansion-algorithm does... or actually defined a concrete set of functions using Javascript/Typescript/WebIDL.

I agree... it's not too late to fix this ambiguity. I personally find function definitions far more useful than english sentences....

didDocument =  « 
didDocumentEntries: INFRA<map>, 
didDocumentRepresentationEntries: INFRA<map> 
»

resolve(
did: INFRA<string>, 
resolutionOptions: INFRA<map>
) →
   « 
   didResolutionMetadata: INFRA<map>, 
   didDocument: INFRA<map>, 
   didDocumentMetadata: INFRA<map> 
   »
   
resolveRepresentation(
did: INFRA<string>, 
resolutionOptions: INFRA<map>
) →
   « 
   didResolutionMetadata: INFRA<map>, 
   didDocumentStream: INFRA<ByteStream>, 
   didDocumentMetadata: INFRA<map> 
   »
      
consume(
contentType: INFRA<string>, 
didDocumentStream: INFRA<ByteStream>
) →
  « 
  didDocumentEntries: INFRA<map>, 
  representationEntries: INFRA<map> 
  »
  
produce(
contentType: INFRA<string>, 
didDocumentEntries: INFRA<map>, 
didDocumentRepresentationEntries: INFRA<map>
) →
  « 
  didDocumentStream: INFRA<ByteStream> 
  »

@msporny
Copy link
Member Author

msporny commented Feb 20, 2021

@OR13 wrote:

I personally find function definitions far more useful than english sentences....

Sure, I do as well, and W3C has a formal language in which to do that... it's called WebIDL... which people objected to using.

Instead, we've got a interface definition language that we invented on top of INFRA -- totally bespoke, no one understands it except for us... and we're using that to define the interfaces in the specification, which BTW, we're totally not chartered to do. The WG decided to look the other way wrt. the Resolution section on that particular point.

That's what I mean by doubling down. I agree that it's valuable, but skirting dangerously close to going outside of our charter and speaking in tongues (bespoke IDL) is the thing that's concerning to me.

@msporny
Copy link
Member Author

msporny commented Feb 21, 2021

This PR is blocked waiting on a review from: @peacekeeper and @jricher

Copy link
Contributor

@peacekeeper peacekeeper left a comment

Choose a reason for hiding this comment

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

bespoke abstract function language

I don't have the same spec writing experience you do, so I'm not sure what other specs do when you want to define an interface (with inputs and outputs) for a process that can have many concrete realizations.

We can still add examples for WebIDL (#587) and TypeScript (#591), I think that would be useful as long as it's clear that they are just examples, not the authoritative definitions.

I also like @OR13 's proposal in #679 (comment) to add function definitions for produce() and consume(). We can again define them in an abstract way with concrete examples in WebIDL and TypeScript.

close to going outside of our charter

My preference at the Amsterdam F2F was also to not include DID Resolution in DID Core, but since then we've operated under the assumption that we want to define the "contract" (without its implementation) of DID Resolution (and DID URL Dereferencing).

Comment on lines +2480 to +2481
A <a>conforming consumer</a> MUST detect any representation-specific
entry across all known <a>representations</a> and place the entry into a
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we want to say "MUST detect any representation-specific entry known to the representation being consumed", and "SHOULD/MAY detect representation-specific entries across all other known representations"? Anyway I'm also fine with this as-is.

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, I had considered that and decided not to go that route because it makes detection of other representation-specific entries optional, which would ultimately lead to interoperability issues on a critical feature needed to solve the problem we're trying to solve. Making this behavior mandatory removes the possibility of non-interoperability at the representation-specific entries data model.

@msporny
Copy link
Member Author

msporny commented Feb 22, 2021

@peacekeeper wrote:

We can still add examples for WebIDL (#587) and TypeScript (#591), I think that would be useful

WebIDL is normative -- it's not for examples. When one writes the WebIDL, you can say: "Do this <WebIDL thing>, or something that is functionally equivalent." It's the "something that is functionally equivalent" that gives us the ability to handwave over the other valid options. In other words, it's easier to look at a WebIDL interface and then see if someone has done effectively the same thing in another programming environment. The alternative is our bespoke INFRA-based IDL language that we've invented... which requires human intervention and tribal knowledge to read the tea leaves.

as long as it's clear that they are just examples, not the authoritative definitions.

I don't understand this... what is the authoritative definition, then? Where is it?

My preference at the Amsterdam F2F was also to not include DID Resolution in DID Core, but since then we've operated under the assumption that we want to define the "contract" (without its implementation) of DID Resolution (and DID URL Dereferencing).

Then I think it's time to revisit that decision given new information. As far as I can tell, there are a number of people in the WG that are 1) asking for this without grasping the ramifications, 2) asserting that we shouldn't set the bar that high, 3) feel like our bespoke IDL is appropriate, or 4) feel like the Resolution contract is fundamental to the specification.

With respect to 1 -- the people that have to write tests on this section are concerned and we should hear them out.

With respect to 2 -- yes, we can lower the bar; that leads to interoperability risks.

With respect to 3 -- our bespoke IDL requires tribal knowledge and human intervention to reason through, which is problematic.

With respect to 4 -- there are multiple others parts in our specification where we point out to sections of other specifications -- for example, URI, JSON, CBOR, INFRA, HASHLINK -- we can point out to an external RESOLUTION specification. I have tried to be very careful about the references to the RESOLUTION section such that they could be moved out if need be... that section is not intertwined with the specification and could easily be moved out if we needed to do so. Both specifications would be strengthened by doing so -- RESOLUTION could be put on a path to become entirely normative instead of being just a contract. This is what's going to happen with that specification in time, anyway.

Like @peacekeeper, my preference would be to publish Resolution in a separate NOTE where there could be bindings defined. It would be analogous to our ADM + representations mechanism in DID Core. In the Resolution spec, it would be the interfaces + bindings. Perhaps we do this if it becomes clear that volunteers for writing the tests and two independent implementations are not forthcoming by the end of the first Candidate Recommendation phase? I'd like to raise this concern on Tuesdays call.

@msporny
Copy link
Member Author

msporny commented Feb 22, 2021

@brentzundel wrote:

I believe this PR includes changes from PR #678. Not sure if that was deliberate.

Yes, that was deliberate... this PR is built on top of #678 because it needs those changes to make sense. Given that people have approved this PR, the assumption is that they're ok with #678 as well (but I will make doubly sure that's true by checking the reviews on both PRs. If there is agreement on #678, I'll merge that to main, and then retarget this PR to main and merge.

@msporny msporny force-pushed the msporny-cr-representation-entries branch from 6c53bfa to 723f751 Compare February 22, 2021 14:20
@msporny msporny changed the base branch from msporny-cr-fix-data-model to main February 22, 2021 14:45
@msporny msporny force-pushed the msporny-cr-representation-entries branch from 723f751 to bd42543 Compare February 22, 2021 14:47
@msporny
Copy link
Member Author

msporny commented Feb 22, 2021

Substantive, multiple reviews, changes requested and made, no blocking objections (but a standing request from @OR13 exists to possibly define abstract functions), merging.

@msporny msporny merged commit b27ae27 into main Feb 22, 2021
@msporny msporny deleted the msporny-cr-representation-entries branch May 30, 2021 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
substantive This is a substantive change to the specification.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants