-
Notifications
You must be signed in to change notification settings - Fork 40
adapter.aasx: Allow reading AASX with wrong relationship #381
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
Conversation
|
Thank you a lot for your contribution! The issue is well known (see also my comment in #379), but it is actually a problem with the specification compliance of the AASX, rather than with our SDK. (Here's the link to the relevant page in the specification). We will discuss your suggestion in the next Dev-Team meeting and will get back to you. |
|
Here's the result of the discussion in our meeting today: We deem it important to point out that there's a balance to be found regarding problems with the compliance towards the specification. The more non-compliance we accept, the less interoperable the whole system of AAS software becomes. Adherence to the specification is extremely important for the interoperability of AAS. This is why we decided that our SDK should support reading the AASX files with wrong relationship URL, however with the caveat that this should a very noticable warning to the user who is reading that file. How should we proceed? |
|
I totally get your point and agree that the specification should be strictly followed. However, i came across this myself as many of the AASX examples from https://admin-shell-io.com/samples/ have the wrong/old relationship URL set. I added a warning about the violation as you proposed. Cheers |
s-heppner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code will make the core of the AASX readable by the AASXReader, meaning the JSON or XML file with the AAS data inside, but it may still mean that thumbnails or additional files are not properly found.
I've suggested to adapt the warning accordingly, to notify the user that they may not get the whole data out of the AASX and they still need to fix the file.
|
I've changed the target branch to our develop branch, since this is where we collect changes until the next release. And of course, all CI checks need to run through before we can merge this. |
|
Thanks for your suggestions, i fully applied them. |
|
Thank you! The one check that is failing ( |
|
Okay, after some digging, this is in fact a dependency problem of our autodocumentation compiler (see #385). The code now LGTM. I'll adapt your PR message slightly and merge it. |
…yx#381) There are many AASX in the wild where there is an additional `www` in the relationship definition that does not belong there: ``` http://www.admin-shell.io/aasx/relationships/aasx-origin ``` whereas it should be according to the [specification]: ``` http://admin-shell.io/aasx/relationships/aasx-origin ``` Previously, our SDK did not allow for reading these AASX files with wrong relationship URL. However, since there are so many of them, this change enables them to be read, while notifying the user with a big warning message that their AASX file is actually not standard compliant and needs to be changed (see discussion in eclipse-basyx#381). Fixes eclipse-basyx#379 Fixes eclipse-basyx#383 [specification]: (https://industrialdigitaltwin.org/wp-content/uploads/2024/06/IDTA-01005-3-0-1_SpecificationAssetAdministrationShell_Part5_AASXPackageFileFormat.pdf#Page=18)
There are many AASX in the wild where there is an additional
wwwin the relationship definition that does not belong there:whereas it should be according to the specification:
Previously, our SDK did not allow for reading these AASX files with wrong relationship URL.
However, since there are so many of them, this change enables them to be read, while notifying the user with a big warning message that their AASX file is actually not standard compliant and needs to be changed (see discussion in #381).
Fixes #379
Fixes #383