Version 1.4.0 (legacy)#31
Conversation
mkllnk
left a comment
There was a problem hiding this comment.
The problem with this release is that it's still using the same namespace. So if I load this gem and the 2.0 gem in the same app then SuppliedProduct will have the methods of both gems in it and the one that was loaded later will override the first one. We need to rename something. My best idea is to rename the Connector module to ConnectorV1. The files then move from connector to connector_v1.
Ruby can't separate different modules that have the same name.
|
OK right, changing the name of the package was indeed not sufficient. So now I've renamed the module to ConnectorV1, is that better? I've published this new version at https://rubygems.org/gems/datafoodconsortium-connector-v1/versions/1.4.0.pre.beta3. |
mkllnk
left a comment
There was a problem hiding this comment.
The module name looks good now but I think that you have to rename the directory as well. Otherwise is statement like require "datafoodconsortium/connector/address" will be ambiguous and won't get resolved correctly. The Rails autoloader Zeitwerk also expects class names to match the file path.
|
OK I've changed the package name and folder to connector_v1 and released this new version as https://rubygems.org/gems/datafoodconsortium-connector-v1/versions/1.4.0.pre.beta4. Is that better? |
mkllnk
left a comment
There was a problem hiding this comment.
I tested this version and the v1 gem works as drop-in replacement. I can change all references to ConnectorV1 and it works.
But this class is still a problem when trying to use both versions at the same time. We are calling JSON::LD::Context.add_preloaded and the alias methods here which are global. Both gems call the same methods and the last one wins. So for now, I'm loading v1 last in OFN to keep all existing code working. But if someone wants to use the default context URI to refer to the latest v2 context then OFN is not resolving that at the moment. If I load v1 first then all the old integrations referring to the default URI fail because the parser tries to load the document with the v2 context.
The situation is ambiguous anyway but we should be able to choose a version via HTTP header. We then just need a way to resolve the context in the right way.
Option one would be to change the json-ld gem to resolve the context in a different way. But, they are actually not doing anything wrong.
So maybe we need to hack the data for compatibility. We could introduce a middleware that detects the DFC version according to headers and referenced context and then it can replace the context URI with the detected context or context URI in the document before it's parsed. That's probably the best approach, patch outdated data to conform to the standard before parsing.
Similarly, I have been wondering about converting incoming v1 data to v2 to then just parse v2 in our code. On the other side, we could convert v2 data to v1 before replying to an old client. That's probably easier to manage than having to different data builders which share a lot of code but build different objects... If we did that on JSON level then we wouldn't need this compatibility gem here. I'm not sure what is easier, transforming JSON or mapping all Ruby objects. 🤷
|
OK so we can't not use anymore the global methods Normally, incoming requests that are using the version 1 of the DFC standard can't use the www.datafoodconsortium.org context URI anymore (because it's pointing to v2.0.0) but should nowadays refer to a certain version of the context directly: "@context": "https://w3id.org/dfc/ontology/context/context_1.16.0.json"So, in the connector v1.4.0, can't we just remove the preloading of www.datafoodconsortium.org and only preload the 1.16 context: URL_NORMALISED = "http://w3id.org/dfc/ontology/context/context_1.16.0.json"
# Remove this line
alias_preloaded("http://www.datafoodconsortium.org/", URL_NORMALISED)
# Keep preloading context v1.16.0
add_preloaded("https://w3id.org/dfc/ontology/context/context_1.16.0.json") { parse(json) }
# We could add an alias to the jsdelivr CDN:
alias_preloaded(
"https://cdn.jsdelivr.net/gh/datafoodconsortium/ontology/context/context_1.16.0.json",
"https://w3id.org/dfc/ontology/context/context_1.16.0.json"
) |
Yes, but legacy apps often don't get updated. V1 apps started using that context URI when the DFC promised to not make breaking changes. Now this is a breaking change. While you are technically correct and the context URI refers to the latest version of the standard, this context URI is pretty useless unless apps always update their code immediately when a new version is published. It's almost impossible to get the timing right. Looking at the reality of used integrations, the latest context URI is ambiguous and a platform needs to decide how to resolve it. We will be able to decide depending on the other platform, authenticated user or header fields, if the URI is meant to be latest v1 or v2. And within our code, we have the choice to load the connector gems in a certain order or override the alias to whatever the current default for our integrations is. I wouldn't change anything. Otherwise the v1 gem is not a drop-in replacement anymore. I just wanted to highlight the issue I found. I don't think that it should be solved in the connector though. We can do that within our app. |
|
Yes you're right, if we want to use the www.datafoodconsortium.org context URI, the context should not contain breaking changes like schema.org does: they only add new things and never delete old things. From my memory I think it was the plan for DFC. So I was wrong, normally incoming requests that are using the version 1 of the DFC standard should still be able to use the www.datafoodconsortium.org context URI. Do you now why exactly the parser fails with the v2 context @mkllnk? When I diff the two files (context 1.16 and context 2.0), I see that there might be an issue with the When I look into the connector v1.4.0 code, the URI of the semantic objects are using URIs prefixed with |
|
Hi @mkllnk I appreciate this is causing pain for you and I'm sorry about that.
I don't think it's realistic to expect anyone to never make a breaking change. In particular the URI change is difficult, but we are not anticipating ever changing those again (assuming w3id.org continues to function 🤞🏻). The more implementations of the DFC Standard there are, the harder it becomes to make changes like these and I'm very glad we got them done now, whilst we only have a handful of production implementations. We adopted semantic versioning to be clear whether there were breaking changes.The v2 change was advertised, including the fact it was a breaking change & that we would be updating all URI's, for nearly 6 months before release. Also worth noting that the breaking change that initiated the V2.0.0 release was the issue OFN raised around currency codes. There is a clear fix - utilise the versioned context files that are now available via the redirect. I'm confused about why we don't want to do that with connectorV1 ? 😕 The other solution that I can see would be to utilise Content Negotiation by Profile we just discussed here and request the appropriate context version from datafoodconsortium.org - that may be beyond the capabilities of the WP plugin though. 🤷🏻 |
|
It feels like we might be mixing things here. Breaking changes can appear in the ontology and that's normal. But I think we might be able to avoid to break the context by always keeping old property definitions? If I'm right the issue of Maikel is caused by one or several missing properties in the context file linked from www.datafoodconsortium.org. Even if the URI of the prefixes "@context": "https://www.datafoodconsortium.org",
"@graph": [
"http://object.example": {
"@type": "dfc-b:SuppliedProduct",
"dfc-b:name": "Test"
}
]In the above example of v1 data, the So we might have to check that the v2 context contains all the properties of all the different v1 versions: from context_1.8.2.json to context_1.16.0.json. |
Is that realistic? In v1, there were breaking changes between the minor upgrades. For example, the phone number changed from a simple string to an object. Parsing breaks. I can't remember all the breaking changes in v2 now. But the most common for us was that the isVariantOf property moved and the SuppliedProduct doesn't have it anymore. Well, and Enterprise doesn't exist in v2. I don't think that it's worth upholding a promise for v2 that had been broken before already. If you want to make that commitment now and never break v2 then that would be welcome, I guess. But I also agree with Garethe that it may not be realistic. Too many versions make interoperability hard. My main concern is that the default URI Even if you keep the context backwards compatible by only adding things and never removing things then our application logic still needs to handle different versions. And we would need to detect the used format to generate the same on output, otherwise the other platform gets incompatible data, e.g. they post an enterprise and get back an organization. The content profile can tell us the version, but so far only v1 or v2, no minor versions. And the profile is optional so far, right? Or will it be mandatory?
I'm sorry if my post sounds like a rant. It's not meant that way. I'm just trying to pin you down to be consistent with whatever you decide you are going to do. We can fix anything. But the more we fulfill the set expectations, the less friction and work there will be.
We do that. I changed the v1 connector before the v2 release to use a fixed version context. Especially because the connector is generated from one particular version without any compatibility code, I would always use a versioned context here. Maxime introduced the version-less default URI for the v2 connector again which is technically correct and if you promise to never do breaking changes then that may be okay in practice, too, but I have my doubts. So I guess it all comes down to one question: Do you want to stop making breaking changes and use the version-less context URI or do you want to play it safe and use versioned context URIs in v2 as well? And with breaking changes, I don't mean just on parsing the document level. If you move a property from one class to another and just add it the context without removing the old property then the document can be parsed as RDF with the context but the content would still contain breaking changes for the application logic. This can be resolved with proper versioning. |
|
I've regenerated from the data-model-uml v3.4.0 which contains fixes for the ontology v1 series. It has introduced two little breaking changes:
We can manually restore this changes, directly in this repo and not in the code generator's one, if you don't want any breaking changes @mkllnk? |
mkllnk
left a comment
There was a problem hiding this comment.
We can manually restore this changes, directly in this repo and not in the code generator's one, if you don't want any breaking changes
I would prefer to keep the old version stable so that we don't have to touch any existing integrations for now. The country is a very simple change but the changes in transformations would break the Shopify integration which is not funded anymore. We would need to update both sides for this.
|
I'm confused because @RaggedStaff asked me on May, 4th on Slack to introduce breaking changes in the TypeScript connector v1 (replace
The issue is datafoodconsortium/data-model-uml#30 and have been fixed by the TypeScript v1.0.0-alpha.12 release. The changelog says:
So I suppose the Shopify app (using the connector-v1) is also expecting these changes to be provided by the Ruby connector-v1 version too @RaggedStaff? It's very difficult for me to follow here. I need a clear decision without opposition between you @RaggedStaff and @mkllnk. Normally the law is the protocol. But the protocol is not defining the entire vocabulary yet and delegates this to the ontology. So now the law is the ontology. So normally we should follow the ontology. The ontology team did a bad job and introduced undesired breaking changes. I think we have to either change the ontology back to revert these breaking changes or incorporate them into the child components which are the UML model and the connectors. Otherwise we are introducing complexity and risk of incompatibility. A standard should be followed by implementers even if it contains bad things. And bad things should not happen. |
mkllnk
left a comment
There was a problem hiding this comment.
Waiting on @RaggedStaff staff to decide. Options for me:
- Keep the current version of the gem, not upgrading to this newer release.
- Upgrade and hope that Shopify gets updated, too, or break it.
- Upgrade and add compatibility code for Shopify integration. But I really don't want to do this.
mkllnk
left a comment
There was a problem hiding this comment.
Nice work. It looks correct with what you have but there are still two open issues:
- The breaking change in transformations (breaking Shopify integrations?).
- The Enterprise not affiliating Enterprises.
For OFN this means that we won't update to these latest versions.
Still no news from @RaggedStaff, it's been a while now, do you know if he is still involved? We could release an OFN specific version reverting the transformations changes so Shopify won't break.
Were you able to have an Enterprise being affiliated to another one using the v1.3.0? I don't see any link of this type. So why you won't update to the v1.4.0 version then? |
I manually added the attribute in OFN when I thought we had agreed to it but it wasn't in the Connector yet. So we are using an attributes that is not standardised at the moment. |
|
Hi both, Shopify is already broken. I'm testing an update with the latest v1 connector (1.0.0-beta-2) so should be ok with the transformer changes from shopify side. |
If we don't want to add things to the version 1 of the connector, we can merge this branch to main and tag it to v1.4.0 @mklink?
I have released https://rubygems.org/gems/datafoodconsortium-connector-v1 from this branch.