Skip to content

Add undefined-term context. #1525

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

Merged
merged 3 commits into from
Jul 20, 2024
Merged

Add undefined-term context. #1525

merged 3 commits into from
Jul 20, 2024

Conversation

msporny
Copy link
Member

@msporny msporny commented Jul 6, 2024

This PR is a partial attempt to address issue #1514 by adding an issuer-dependent context that uses @vocab for localized deployments.

@msporny msporny requested a review from selfissued as a code owner July 6, 2024 20:23
@msporny msporny added editorial Purely editorial changes to the specification. CR1 This item was processed during CR1 labels Jul 6, 2024
@msporny
Copy link
Member Author

msporny commented Jul 6, 2024

This PR suggests a bad development pattern that the VCWG shouldn't condone for the following reasons:

  1. Not defining terms and documentation for production applications is lazy, even in a two party ecosystem, and that sort of laziness will bite you in a variety of ways (name clashes, vague semantics, interop failures, lack of good developer experience) as the application scales.
  2. It would be better for developers that want to use "private terms" to specify their own namespace that they control instead of using a global one that is guaranteed to create semantic conflicts over time.

That said, if we have WG Members that insist on this design pattern, I'll be a -1, but I won't block or formally object to the feature.

Copy link
Contributor

@decentralgabe decentralgabe left a comment

Choose a reason for hiding this comment

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

Thanks! Is it worth adding an example or text around this being available?

Copy link
Contributor

@selfissued selfissued left a comment

Choose a reason for hiding this comment

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

There's already a @vocab value in the base context, so this is redundant.

@PatStLouis
Copy link
Contributor

@selfissued I believe you refer to the vcdm 2.0 context, the VC data model has multiple versions and this pr will enable implementers to have the same benefit with say the vcdm 1.1. This is irrelevant to the version used and just a great resource to have for developers getting started in the space overall.

I support this PR.

Copy link
Contributor

@dlongley dlongley left a comment

Choose a reason for hiding this comment

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

Approving as-is, though I've offered an alternative URL because I think the "issuer dependent" vocabulary approach has led to some confusion and trouble that would best be avoided (e.g., vendor lock-in, developers making unsafe assumptions). But I'll accept whichever approach can get consensus.

Copy link

@aniltj aniltj left a comment

Choose a reason for hiding this comment

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

@msporny said:

It would be better for developers that want to use "private terms" to specify their own namespace
that they control instead of using a global one that is guaranteed to create semantic conflicts over time.

+1

We need to do a better job signalling that 1) usage of the [local term / private] context will inevitably lead to conflicts at scale, and 2) usage of the context is meant to be for local ecosystems only (not global ones).

+1

@dlongley said:

I've offered an alternative URL because I think the "issuer dependent" vocabulary approach has led to some
confusion and trouble that would best be avoided (e.g., vendor lock-in, developers making unsafe assumptions)

+1

I've been reading the multiple threads on this and adjacent topics. I see the value of @vocab for development and attribute bundle refinement purposes by everyone who is using VCDM 2.0. I also know that there are entities with deep expertise in JSON-LD and vocabularies who leverage it with full awareness of how best to utilize it properly (I do not count myself to be in this category).

At the same time, I also recognize (as shared in other threads) that @vocab is being used in VCDM 2.0 for a purpose it was not intended, which is a choice that tends to be a source of many unintended consequences, so that is and should be very concerning!

Because I don't know what I don't know, I am approving this with the caveats above, and defer to the folks with practical @vocab implementation expertise in the hope that they will be able to help craft the normative language and options around this in a manner that can strike a balance between utility and protection from foot-guns.

@TallTed
Copy link
Member

TallTed commented Jul 8, 2024

For all the reasons argued above, I'm in favor of this PR. My favorite option for the @vocab value is undefined-term, closely followed by local-term.

@PatStLouis
Copy link
Contributor

+1 for undefined-term, which is the json-ld error which would be raised. This avoids raising this error but marks the terms as undefined.

Copy link
Member

@iherman iherman left a comment

Choose a reason for hiding this comment

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

I am fine having this (I'd prefer private-term or undefined-term, with a slight preference to the latter). However,

  • Is this additional to, or replacing, the @vocab usage in the core context? I would prefer replacing. Having both is useless; if this context file comes after the core context, it will, I believe, override the @vocab statement in the core.
  • There is, currently, vocab/credentials/v2/issuer-dependent.html file on the repo, which is used as a redirection target (at the moment) for the default @vocab. Just flagging that if we rename the URL, that file must be renamed/copied, too!

@msporny
Copy link
Member Author

msporny commented Jul 9, 2024

I am fine having this (I'd prefer private-term or undefined-term, with a slight preference to the latter). However,

Noted, it looks like "undefined-term" is getting the most support so far.

  • Is this additional to, or replacing, the @vocab usage in the core context? I would prefer replacing. Having both is useless; if this context file comes after the core context, it will, I believe, override the @context statement in the core.

It's mean to be a replacement, not in addition to (because as you say, "in addition to" wouldn't make much sense at all).

  • There is, currently, vocab/credentials/v2/issuer-dependent.html file on the repo, which is used as a redirection target (at the moment) for the default @vocab. Just flagging that if we rename the URL, that file must be renamed/copied, too!

Yep, noted, we'll want to make that update in this PR once we settle on the final terminology.

@iherman
Copy link
Member

iherman commented Jul 9, 2024

There's already a @vocab value in the base context, so this is redundant.

It is not redundant; it is a replacement for a @vocab in the base context. Put it another way, it is moving the (slightly hidden) feature in the base context more into the open.

@decentralgabe
Copy link
Contributor

decentralgabe commented Jul 9, 2024

I like private-term. I dislike undefined-term since the term is being defined (I think..) by the default namespace. Private also aligns with JWT Private Claims so this should be an easier mental model shift for folks newer to this ecosystem.

@TallTed

This comment was marked as resolved.

@msporny
Copy link
Member Author

msporny commented Jul 9, 2024

@decentralgabe wrote:

I dislike undefined-term since the term is being defined (I think..) but the default namespace.

The term is, specifically, not being defined. Here's an example of a term that is defined:

https://www.w3.org/2018/credentials/#issuer

As a comparison, this one isn't defined:

https://www.w3.org/ns/credentials/issuer-dependent#foo

That is, there is no fragment identifier, there is no human-readable definition, and no way to discover where the actual definition is... and that will never change (because the "issuer-dependent" vocabulary isn't designed to ever resolve to any definition). Even the "issuer dependent" vocabulary says this:

The base context has been constructed in such a way that issuers may use new, hitherto undefined terms without further ado, and these terms will be mapped onto URLs of the form https://www.w3.org/ns/credentials/issuer-dependent#TERM.

The only thing that give you is a URL that will resolve to a human-readable page that contains no definition for the term used.

@iherman

This comment was marked as resolved.

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.

Approving, assuming the change to undefined-term

@msporny msporny changed the title Add issuer-dependent context. Add undefined-term context. Jul 15, 2024
@msporny
Copy link
Member Author

msporny commented Jul 15, 2024

Barring @selfissued's objection to the @vocab changes, I /think/ we have consensus for "undefined term", but I want to make sure before merging this PR. We'll check on the call this week.

@iherman
Copy link
Member

iherman commented Jul 17, 2024

The issue was discussed in a meeting on 2024-07-17

  • no resolutions were taken
View the transcript

3.2. Remove @vocab from the base context. (pr vc-data-model#1520)

See github pull request vc-data-model#1520.

Brent Zundel: also talking about #1525 simultaneously.

Brent Zundel: 1520 and 1525 exist as a pair, really.

See github pull request vc-data-model#1525.

Brent Zundel: One of them 1520 takes vocab out of base context. 1525 adds optional new context with vocab in it.
… which allows people to test/develop.

Dmitri Zagidulin: quick question. about adding vocab to undefined terms context. Is that an option in addition to the examples context?
… we already have @vocab in the examples.
… I am a fan of taking it out of core.

Manu Sporny: You're right, Dmitri. 1525 would add a separate @vocab in the undefined terms context.
… that one would be able to be used in production for those who want to use it. We recommend not to use @vocab in production unless you know what you're doing.
… the compromise we got to was that Gabe wanted something other than the examples contexts that leverages issuer-defined terms.
… Gabe liked this approach, so it met his concerns.
… The main difference is that the examples context is not for production.
… The new one is.

Michael Jones: I take exception that the working group has decided to remove @vocab.

Brent Zundel: I did not see it that way, so I didn't call it out as chair.
… ok. two PRs. anyone other than Mike who objects, please speak up.

Brent Zundel: as the only objector, is this is merged in, do you expect to formally object if this goes in.

Michael Jones: I hope we'll never get there.
… We should have stability. This is destabilizing.
… The reason we put this in is so that all terms are processed as RDF. This is a security problem.

Brent Zundel: I felt there was additional information that justified reopening the discussion.
… I agree it is destabilizing, but I felt it was necessary.

Michael Jones: this is a security degradation.

Ivan Herman: +1 to dmitriz.

Manu Sporny: +1 to dmitriz.

Dmitri Zagidulin: Just taking out @vocab is destabilizing, but adding it back to undefined terms to restore stability.

Benjamin Young: +1 to dmitriz; definitely makes things clearer for everyone.

Dmitri Zagidulin: we are offering the same security options, we're just adding a flag for it.

Michael Jones: it's still a security degradation because its still optional.
… developers are lazy.
… deployments will occur where they forget to add the additional context.

Ivan Herman: I'm surprised by "security degradation". But the current situation has a security problem as well.
… so the question is which of these are worse?

Manu Sporny: I also take exception to the concept that there is a security degradation.
… we are repeating things that have been discussed in the issues multiple times.
… If a term is undefined, the processor throws an error. That's what happens. I don't see how a processor throwing an error is a security degradation.
… This is a security enhancement.
… You seem to be arguing from a position from a year ago that we have already addressed.

Dmitri Zagidulin: +1 I like that suggestion.

Brent Zundel: I believe I have a suggestion for addressing Mike's concern. How about "If you define terms that are not in the default context, you must use the undefined terms context".

Joe Andrieu: The only concern I have with what you said, Brent, is that you should still be able to define terms in our own context and not use the undefined terms context.

Joe Andrieu: +1.

Brent Zundel: You must either define terms in your own context or use the undefined terms context.

Ivan Herman: +1 to brent's option.

Dave Longley: +1 to say that you must do either of these things.

Brent Zundel: does this sound like a viable path forward?
… I'm seeing +1s.
… anyone who can't live with this moving forward, speak now.
… This feels like consensus emerging.
… Manu, these are your PRs, will you make those mods?

Manu Sporny: yes.
… can we merge it once we do that?

Brent Zundel: Mike?

Michael Jones: I'd like to get a few people's eyes on the new semantics before we merge it.

Brent Zundel: Manu, thanks for the willingness to make the changes. Ping me or Mike once you have the changes in.
… If those changes go in this afternoon, can you re-review by the end of the week?

Michael Jones: Yes.

Brent Zundel: So we'll have feedback by Friday, which if cleared, means you can merge.
… one last thing on charter. We don't need to hold off on a bunch of PRs as the charter explicitly includes the details we need.
… next up: Issues.

@msporny
Copy link
Member Author

msporny commented Jul 17, 2024

Per the call today, this PR will be merged by Friday if there are no more objections.

#1520 (comment)

@msporny msporny requested a review from selfissued July 17, 2024 21:00
@tplooker
Copy link

As with #1520 I won't block this PR, however I believe the framing is wrong. @vocab is a valid way to define a term and it confuses developers trying to understand JSON-LD to talk about terms that are defined with this @vocab entry as "undefined", I would prefer "private-term" or something else to that affect.

Not defining terms and documentation for production applications is lazy

This framing is problematic, if implementations simply wish to share a single global namespace without having to define it themselves, they should be able to do that without having to become the definer of that namespace. Its not being lazy its being pragmatic.

It would be better for developers that want to use "private terms" to specify their own namespace that they control instead of using a global one that is guaranteed to create semantic conflicts over time.

See above, if developers wish to share a single global namespace without having to define it themselves that is a valid requirement, this is a different usecase which they should also be free to do if they so wish.

@msporny
Copy link
Member Author

msporny commented Jul 20, 2024

Editorial, multiple reviews, changes requested and made, no remaining objections, WG-imposed merge delay requirement in #1520 met, merging.

@msporny msporny merged commit ccbe460 into main Jul 20, 2024
1 check passed
@msporny msporny deleted the msporny-issuer-dependent branch July 20, 2024 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CR1 This item was processed during CR1 editorial Purely editorial changes to the specification.
Projects
None yet
Development

Successfully merging this pull request may close these issues.