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

Defined entity names should not be re-defined in UNTP or its extensions #323

Open
absoludity opened this issue Mar 11, 2025 · 9 comments
Open
Assignees

Comments

@absoludity
Copy link
Contributor

absoludity commented Mar 11, 2025

We are running into a couple of issues while testing a UNTP extension - the aatp DigitalLivestockPassport.

The main trigger for these issues is that we've updated the aatp DigitalLivestockPassport extension to inherit from the UNTP DPP credential, so that now a DLP is a DPP which is a VC (where as previously a DLP was a VC only, but not necessarily a DPP, which would mean it could not be validated as a DPP). This update then requires including the DPP context in the DLP which should not be a problem, except that:

  • We define some entities in untp-core, such as Product and Facility, which we then re-define in other domains, such as DCC, adding extra fields. Normally on its own this shouldn't be an issue since (as I understand it) the untp-core entities aren't meant to be included directly in another domain's context unless they are used explicitly as a property type, as they are more like building blocks, but
  • Jargon includes all "Blue" classes from an imported domain in that domain's own generated context file (if the imported domain is context-generating itself, which untp-core needs to be so that when entities like RenderTemplate2024 are included in other domains, they are included as untp-core entities, rather than redefined for each domain)

As a result, if a domain such as DCC (or DLP) imports untp-core, Jargon will want to include all the "blue" classes such as Product and Facility etc. in the context for that importing domain. But if that importing domain, say DCC, also defines its own Product, then Jargon has to start making decisions about which Product is the correct one and currently merges a bit of both, which causes a re-definition. The reality is that I think we are asking for Jargon to do different things in different contexts, so for example we'd want:

  • the DCC Product to have its own "id": "dcc:Product" since it is a new entity with extra fields, but
  • the DCC RenderTemplate2024 should not have its own "id": "dcc:RenderTemplate2024" but rather use "id": "untp-core:RenderTemplate2024" since we just want to re-use the core one in all domains (without re-defining it)

It is hard for Jargon to have a "correct" way of interpreting a redefinition of an imported entity and we could avoid this whole situation if we just have a rule that within UNTP (and its extensions) we don't redefine previously defined entities, ever (Alastair can make a rule that ensures that is the case for extensions too).

I'll follow-up with a comment that analyses all the current entities causing this and the different options for avoiding the redefinition while retaining the same functionality.

See jargon-sh/issues#42 for details of where this issue was initially discussed on jargon.

@absoludity absoludity self-assigned this Mar 11, 2025
@absoludity absoludity changed the title Defined entity names should not be re-defined Defined entity names should not be re-defined in UNTP or its extensions Mar 11, 2025
@absoludity
Copy link
Contributor Author

Summary of entity redefinitions in the UNTP domains

Scanning the domain definitions, it is only Product and Facility that are redefined in three domains:

Digital Product Passport

Product is redefined from core with extra fields.

Conformity Credential

Product and Facility are redefined from core with extra fields

Digital Facility Record

Facility is redefined from core with extra fields.

Digital Identity Anchor

No redefinitions

Digital Traceability Event

No redefinitions

@absoludity
Copy link
Contributor Author

absoludity commented Mar 12, 2025

Solution overview

Renaming the base class

Since the base Product and Facility classes in untp-core are only used for extending in other classes [1], the simplest solution would be to rename them ProductBase and FacilityBase, or CoreProduct and CoreFacility. The DPP Product would inherit from ProductBase, and the DLP BovineAnimal would inherit from dpp Product.

But this has two disadvantages:

  1. It sets the path for long inheritance chains, so for example, the BovineAnimal's type would be ["CoreProduct", "Product", "BovineAnimal"]
  2. It means different domains are still defining conflicting terms (both DPP and DCC would define different Product terms) which doesn't bite us yet, but may later (eg. a jsonld doc with a graph of credentials?)

Renaming the inheriting class

An improvement would be to rename the inheriting class, so the DPP Product becomes something like PassportProduct: untp-core:Product. This still has the same disadvantage of the long inheritance chain, but removes the conflicting terms between DPP and DCC.

Composition over inheritance

One further improvement could be not to inherit the core Product in DPP at all to add the claim-related fields (ie. not have an "IS-A" relationship) but instead compose the core Product (ie. have a "HAS-A" relationship) into an entity which includes the claim related fields. So, for example, in DPP, rather than redefining Product with extra data about the claims, we'd define:

ProductClaims:
  product: untp-core:Product
  # The extra claim fields added in DPP that are always applicable (currently, just means also used in DLP)
  granularityLevel:Code(granularityCode)
  conformityClaim:Claim[]
  emissionsScorecard:untp.EmissionsPerformance
  traceabilityInformation:TraceabilityPerformance

# Separating out the claim info which isn't always relevant (ie. isn't used in DLP)
ManufacturedProductClaims: ProductClaims
  dueDiligenceDeclaration:untp.Link
  materialsProvenance:untp.Material[]
  circularityScorecard:untp.CircularityPerformance

So a DPP ProductClaims has-a product as well as the claims made about that product in the passport. Direct users of DPP can choose either ProductClaims or ManufacturedProductClaims as the subject of their DPP credential

The DigitalLivestockPassport extension can then extend both the core Product (adding bovine characteristics) and the dpp ProductClaim directly (so it doesn't get the unrelated dueDiligenceDeclaration, materialsProvenance and circularityScorecard properties) and add its own specific healthTreatment claim field, which is simple:

BovineAnimal: Product
  characteristics:aatp.BovineCharcteristics

BovineAnimalClaims: ProductClaims
  healthTreatment:aatp.HealthTreatment[]

With this, the DigitalLivestockPassport is-a DigitalProductPassport, the DLP credentialSubject is-an instance of ProductClaims which has-a product and claims, the product can be a BovineAnimal (since it is-a core Product), and the DLP claims can include extra specific terms such as healthTreatment while still passing as a DPP (since BovineAnimalClaims is-an instance of ProductClaims .

At the same time, the inheritance chain is short due to the combination of composition and inheritance, so a BovineAnimal's type is just ["BovineAnimal", "Product"], and a DLP passport's type is ["DigitalLivestockPassport", "DigitalProductPassport", "VerifiedCredential"] as expected.

A similar model would be applied to Facility.

Note: we could potentially get even better/simpler composability in the future using Jargon's interfaces to avoid, for example, ManufacturedProductClaims having to inherit from ProductClaims and instead composing, but it'd be too big a change/risk for now (Alastair and I are unsure about the implications of using interfaces here).

Summary

Option 2, "Renaming the inheriting class" would be the simplest change to avoid the current issue, especially if we want to release quickly. Option 3 "Composition over inheritance" seems like a neater solution that enables better extensibility, but may be riskier.

@onthebreeze
Copy link
Contributor

My instinct says go ahead with option 3 - composition over inheritance. Over-use of inheritance is a painful lesson that has been learned the hard way before. The only thing is I might change some of the choice of class names but that doesn't change the preinciple.

@PatStLouis
Copy link
Contributor

@absoludity I also think option 3 is best and it's what we have adopted for the BC Mines Act Permit. The extensions should only define terms related to itself. At the end of the day, implementers are free to create extensions as they see fit, we can only make recommendations. As long as the end product is conformant, this is an implementation detail. If my extension only defines one additional field (ex: permitNumber), it makes more sense for my context to only list an additional type with this property, and my schema to only check if that property is there (with whatever rule I want to set for it), instead of testing it as a whole.

This way extensions are sheltered from core model updates so I wont need to publish a new extension update every time the core model updates.

@absoludity
Copy link
Contributor Author

Thanks @PatStLouis, @onthebreeze . I've gone ahead with option 3 and released the five untp domains with 0.6.0-beta9 and then used that to create the 0.4.2-beta1 release of the DigitalLivestockPassport (though I think there's a manual release for that with @ashleythedeveloper does) , all of which are valid jsonld (passing jsonld lint --safe), but I've not yet checked the schemas.

There are two small items related to Jargon which I need to pick next week and see if we can improve them:

  • ProductClaim has an unnecessary id: As VCDM defines VerifiableCredential.credentialSubject we cannot (and should not) redefine that field. For this reason we have the [jsonld.contextOmit]=true set for it on our credentials such as the DPP, but additionally, the type assigned to it needs to be a type with an id to ensure Jargon sees it as a "blue" entity and defines the ProductClaim term. Without it, jargon treats it as a green entity which is defined inline, but in this case due to the required contextOmit (and not being able to define it inline without redefining credentialSubject) it is not defined at all. Hence adding the id property even though it's not otherwise required on ProductClaim.
  • The sample data for the DigitalLivestockPassport uses an electric battery - obviously not ideal. This is because the BovineAnimalClaim needs to be a ddp.ProductClaim so that it is a valid DPP, and the dpp.ProductClaim.product property is a untp.Product. We cannot (and should not) redefine this, nor do we need to since a BovineAnimal IS-A untp.Product and can be used there validly, but the sample data can't know that without some hint from us. For now, we can manually update the sample data (just pasting in the BovineAnimal sample data over the product in the DLP instance data), which I've done and verified it as valid. I'll attach that here [1]

aatp-dlp-instance-with-bovineanimal-0.4.2-beta1.json (note that the third context item has been updated to point at the jargon artefact, since the actual one for DLP will be published manually and isn't done yet).

[1] Actually, looking at the sample data specifically for BovineAnimal, I see it's also using the inherited examples rather than the overridden ones (eg. producedAtFacility has "Greenacre batteries" rather than the farm example. I'll include that in the jargon issue when created after double-checking I've not missed something.

@colugo
Copy link

colugo commented Mar 13, 2025

I haven't read through this in it's entirety, but did want to point out that Jargon has the capability to override example values, even those on deeply imported properties:

Image

@colugo
Copy link

colugo commented Mar 13, 2025

[1] Actually, looking at the sample data specifically for BovineAnimal, I see it's also using the inherited examples rather than the overridden ones (eg. producedAtFacility has "Greenacre batteries" rather than the farm example. I'll include that in the jargon issue when created after double-checking I've not missed something.

producedAtFacility is a property in the DLP domain, but it's type is ultimately untp-core.Facility - so giving producedAtFacility a [jargon.example] value won't actually do anything, as Facility is a class and doesn't hold a value in the json instance. If you want to set the example values inside producedAtFacility, you'll either need to use the method shown above, or declare a subclass of Facility, pull down its properties and set example values on those - which is probably not what you're after. I'd use the example values workflow from my above comment.

What you've got currently would work, but only if producedAtFacility was ultimately a primitive type.

Image

@absoludity
Copy link
Contributor Author

Thanks @colugo . RE producedAtFacility - I'd just copied and pasted the example texts from the older 0.4.0, I wasn't checking those so much as the product name. But I'll take a look at the method above first thing next week and see how it applies to this situation.

@colugo
Copy link

colugo commented Mar 13, 2025

I'll see if I can get around to having Jargon throw a warning when [jargon.example] is applied on a complex type. It will air like the issue on line 10 in the above screenshots.

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

No branches or pull requests

4 participants