admin-shell-io / aas-test-engines

Official test tooling for the Asset Administration Shell
https://certification.admin-shell-io.com
Apache License 2.0
5 stars 2 forks source link

Unexpected errors while validating AAS JSON file #13

Closed juileetikekar closed 1 month ago

juileetikekar commented 2 months ago

While validating the AAS Json file via test-engine (check_json_file method), the engine generates some unexpecred errors. For instance, even though the submodel consists of only a property and a reletionship element, the validation result consists of error messages for the submodel elements like blob, range, file, etc which are not present in the model itself. Additionally, it gives a message "property modelType is invalid", which seems unexpected.

The AAS Json and the error message attached: test.json error.txt

The attached AAS Json file consists of below intentional errors:

  1. empty globalAssetId in AssetInformation
  2. missing first and second in RelationshipElement
mjacoby commented 2 months ago

I'm neither one of the developer nor have used the tool before but it seems to me there are multiple issues with your input file which you can find in the error message but somewhat hidden as there are a lot of other messages that are not really relevant. This issues I was able to identify are the followin

There is another issue reported, namely that the conceptDescriptions is empty. However, to my knowledge this is an outdated requirement that is no longer valid and therefore should not be validated.

juileetikekar commented 2 months ago

Hi @mjacoby,

Thank you for your input.

As I mentioned, empty GlobalAssetId and empty references are intentional errors added to the AAS. I wanted to test the invalid file with the test engine (negative test scenario).

With this file, I expect errors regarding GlobalAssetId and empty eferences. But all other errors w.r.t. all the SMEs which are not event present in the AAS are unexpected.

mjacoby commented 2 months ago

I'm sorry, I did miss the part about the intentional errors at the bottom. And I understand that you dislike the output you are getting. I totally agree that this is not user-friendly.

To me it seems that the output is generated automatically by some generic JSON Schema validator which would explain all these mentionings of Blob, File, and other elements that are not present. The reason seams to be that the JSON Schema defines e.g. that a each submodel element must be one of the known element types (called SubmodelElement_choice) and the validator than checks each submodel element if they match any of the known valid types. As the submodel element is not valid, it does not match any of the cases and the validator then prints one error for each failing check, i.e. element is not a File, it is not a Blob, etc.

I guess it is going to be very hard if not impossible to get more accurate error messages without writing a lot of custom code for the validation and still than it is not always clear what an error text should look like, e.g., what would be the "correct" error code for the following JSON (mixing elements of Property and Entity classes)

{
   "entityType": "CoManagedEntity",
   "modelType": "Property"
}

Should it say that modelType is wrong because the JSON starts with the property entityType which indicates that this should be an instance of Entity or should it prioritize the modelType and rather give the error that property entityType is not defined for type Property? Or maybe both?

What I'm trying to say is that I can understand both the reason why the messages look like they do and also why you are unhappy with it. And I'm not sure if there is any easy way to improve this,unfortunately.

However, it would be nice to hear from the developers what they think about this and what are their plans to improve the error messages.

otto-ifak commented 2 months ago

Hello everyone and thanks for @mjacoby for helping out :smile:

@mjacoby, you are right we are doing a JSON schema check so it is difficult to get a "directed" output. I though about rewriting the schema using if, then, else. However, this would add some additional effort, as we are currently deriving our schema from the official one (https://github.com/admin-shell-io/aas-specs/tree/master/schemas/json).

Concerning the conceptDescription Is there a section in the spec about this? According to https://github.com/admin-shell-io/aas-specs/blob/2ab08f92bdd1d44edc1cfee52552fe5429d2178e/schemas/json/aas.json#L311 there shall be at least one item.

mjacoby commented 2 months ago

You are right, the JSON schema says an Environment must have at least 1 conceptDescription. However, the AAS specification (Section 5.3.9, page 81) says it can have zero or more elements.

At first glance, this seems like a bug. However, I could find the following sentence in a README on Github

Optional attributes, i.e., the attributes with the cardinality 0..1, are modeled as [non-required properties].

This indicates, that in JSON the property conceptDescriptions should not be present at all if empty and if present must be non-empty. This would mean that the provided JSON is actually invalid.

However, I would argue that this sentence is not part of the specification itself and therefore not normative.

The correct way would be either to put this sentence in the specification or changes the JSON schema to reflect what is written in the spec, i.e. allow the property to be empty.

otto-ifak commented 2 months ago

I have created a new issue about this https://github.com/admin-shell-io/aas-test-engines/issues/15

juileetikekar commented 2 months ago

Hi @otto-ifak,

The above created issue addresses the issue with concept descriptions. But we should also address the additional unexpected issues being generated by test engine, for example, the errors w.r.t. the submodel elements, that are not even present in the AAS model.

BirgitBoss commented 2 months ago

Many of the validation problems it seems to me would be obsolete if the semantics of the submodel (…as defined in the submodel template) would be taken into account for the test. In the case of submodel instances this is what we really need and not a purely syntactic check.

BirgitBoss commented 2 months ago

I added https://github.com/admin-shell-io/aas-specs/issues/418

BirgitBoss commented 2 months ago

For validation the schemas are the master in case of conflicts. But please add issues if you find such inconsistencies.

otto-ifak commented 1 month ago

Fixed using discriminator from OpenAPI (https://swagger.io/specification/#discriminator-object)