Added $filters to AAS QL & enhanced Fragment Fieldidentifiers#532
Added $filters to AAS QL & enhanced Fragment Fieldidentifiers#532Martin187187 wants to merge 1 commit into
Conversation
Martin187187
requested review from
BirgitBoss,
alexgordtop,
aorzelskiGH,
danielporta,
g1zzm0,
mjacoby,
sebbader-sap,
waltersve and
zrgt
as code owners
|
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
I would really like to remove the following section in the Json Security specification: This is redundant because of the newly introduced FILTERLIST: I didnt remove it to not cause any breaking changes. |
mdanish98
left a comment
mdanish98
left a comment
There was a problem hiding this comment.
Hi @Martin187187 ,
thanks a lot for this pull request.
I have reviewed this PR and provided the following review remarks mainly for naming inconsistency. Please note that the naming inconsistency might not be limited to what I pointed out, so please check other files and locations.
| [source,json] | ||
| ---- | ||
| { | ||
| "condition": { |
There was a problem hiding this comment.
This should be either $condition or $formula depending on the final decision, as pointed out in another comment.
| "pattern": "^id$" | ||
| }, | ||
| "$condition": { | ||
| "$formula": { |
There was a problem hiding this comment.
There are some naming inconsistency:
Here in query-json-schema.json uses $formula (lines 750 & 761)
openapi.yaml still uses $condition (lines 802 & 812)
query-language.adoc also has $condition (e.g., line 690)
We should use either $condition or $formula to be consistent.
There was a problem hiding this comment.
I agree that $condition is the better choice, and I will update $formula accordingly. That said, the security layer currently relies on FORMULA, as defined in the specification, which introduces a naming inconsistency.
| @@ -784,6 +801,13 @@ components: | |||
| pattern: ^id$ | |||
| $condition: | |||
There was a problem hiding this comment.
Highlighting the naming inconsistency as explained in the above comment.
| items: | ||
| $ref: '#/components/schemas/QueryFilter' | ||
| required: | ||
| - $condition |
There was a problem hiding this comment.
Highlighting the naming inconsistency as explained in the above comment.
There was a problem hiding this comment.
Please update all JSON examples that use $condition to use $formula. Also, I have noticed that some examples uses condition without a $ prefix, so please consider this also when you search and replace.
There was a problem hiding this comment.
I would rather use $condition. I there a reason why you prefer $formula?
There was a problem hiding this comment.
No, I don't have any preferences, please use whatever makes sense and aligns with the AAS QL. This remarks is only to make sure whichever we decide, we use it everywhere and not a mix.
There was a problem hiding this comment.
I think in Security in ACL it is FORMULA (and CONDITION) whereas in the query language only "condition" is used, there is no $formula, only a $condition in Part 2
|
Also, I am seeing some alerts from GitHub code scanning, please either fix it or dismiss it. |
Thanks alot for your review @mdanish98. I fixed the naming inconsistencies. |
There was a problem hiding this comment.
I changed a lot here because:
- added Fragment Field Identifier that are used to express what to filter (different to normal field identifier
- I noticed huge gaps between the json and bnf. I fixed the inconsistencies in the BNF so JSON schema has no breaking changes
|
I’ve updated this PR after identifying several inconsistencies and gaps between the JSON Schema and the BNF. Specifically:
If you’d like to review the test cases or see how I validated the schema, you can find them here: Happy to provide additional test data or walk through the validation approach if needed. |
| "USEFORMULA": { | ||
| "type": "string" | ||
| }, | ||
| "FRAGMENT": { |
There was a problem hiding this comment.
There was a significant discrepancy between the security JSON schema and the API JSON schema. In the security specification, the fragment object is nested inside the filter object, but this is not the case in the API schema. I have now updated both documents to use the security version for consistency.
There was a problem hiding this comment.
So is it a bug for 3.0.2?
There was a problem hiding this comment.
yes it is fixed already
|
@Martin187187 is the PR still in "draft" or now in the "ready for review" state? |
I will have to resolve conflicts caused by the bugfixes. So it is currenlty only a draft. |
|
Do we really add |
There was a problem hiding this comment.
probably it would be better to include a .json file for better checks (same in Part 1 vor Value-Only Schema)
|
I recommend to add an annex with complete examples like in Security: https://industrialdigitaltwin.io/aas-specifications/IDTA-01004/v3.0.2/annex/text-access-rule-examples.html |
| "modelStringPattern": { | ||
| "FieldIdentifier": { | ||
| "type": "string", | ||
| "pattern": "^(?:\\$aas#(?:idShort|id|assetInformation\\.assetKind|assetInformation\\.assetType|assetInformation\\.globalAssetId|assetInformation\\.specificAssetIds\\[[0-9]*\\]\\.(?:name|value|externalSubjectId(?:\\.(?:type|keys\\[[0-9]*\\]\\.(?:type|value)))?)|submodels\\[[0-9]*\\]\\.(?:type|keys\\[[0-9]*\\]\\.(?:type|value)))|\\$sm#(?:semanticId(?:\\.(?:type|keys\\[[0-9]*\\]\\.(?:type|value)))?|idShort|id)|\\$sme(?:\\.[A-Za-z](?:[A-Za-z0-9_-]*[A-Za-z0-9_])?(?:\\[[0-9]*\\])*(?:\\.[A-Za-z](?:[A-Za-z0-9_-]*[A-Za-z0-9_])?(?:\\[[0-9]*\\])*)*)?#(?:semanticId(?:\\.(?:type|keys\\[[0-9]*\\]\\.(?:type|value)))?|idShort|value|valueType|language)|\\$cd#(?:idShort|id)|\\$aasdesc#(?:idShort|id|assetKind|assetType|globalAssetId|specificAssetIds\\[[0-9]*\\]\\.(?:name|value|externalSubjectId(?:\\.(?:type|keys\\[[0-9]*\\]\\.(?:type|value)))?)|endpoints\\[[0-9]*\\]\\.(?:interface|protocolinformation\\.href)|submodelDescriptors\\[[0-9]*\\]\\.(?:semanticId(?:\\.(?:type|keys\\[[0-9]*\\]\\.(?:type|value)))?|idShort|id|endpoints\\[[0-9]*\\]\\.(?:interface|protocolinformation\\.href)))|\\$smdesc#(?:semanticId(?:\\.(?:type|keys\\[[0-9]*\\]\\.(?:type|value)))?|idShort|id|endpoints\\[[0-9]*\\]\\.(?:interface|protocolinformation\\.href)))$" |
Check warning
Code scanning / QDJVMC
Typo Warning documentation
| "FragmentFieldIdentifier": { | ||
| "type": "string", | ||
| "pattern": "^(?:\\$aas#(?:idShort|id|assetInformation\\.assetKind|assetInformation\\.assetType|assetInformation\\.globalAssetId|assetInformation\\.specificAssetIds\\[[0-9]*\\]\\.(?:name|value|externalSubjectId(?:\\.(?:type|keys\\[[0-9]*\\]\\.(?:type|value)))?)|submodels\\[[0-9]*\\]\\.(?:type|keys\\[[0-9]*\\]\\.(?:type|value)))|\\$sm#(?:semanticId(?:\\.(?:type|keys\\[[0-9]*\\]\\.(?:type|value)))?|idShort|id)|\\$sme(?:\\.[A-Za-z](?:[A-Za-z0-9_-]*[A-Za-z0-9_])?(?:\\[[0-9]*\\])*(?:\\.[A-Za-z](?:[A-Za-z0-9_-]*[A-Za-z0-9_])?(?:\\[[0-9]*\\])*)*)?#(?:semanticId(?:\\.(?:type|keys\\[[0-9]*\\]\\.(?:type|value)))?|idShort|value|valueType|language)|\\$cd#(?:idShort|id)|\\$aasdesc#(?:idShort|id|assetKind|assetType|globalAssetId|specificAssetIds\\[[0-9]*\\]\\.(?:name|value|externalSubjectId(?:\\.(?:type|keys\\[[0-9]*\\]\\.(?:type|value)))?)|endpoints\\[[0-9]*\\]\\.(?:interface|protocolinformation\\.href)|submodelDescriptors\\[[0-9]*\\]\\.(?:semanticId(?:\\.(?:type|keys\\[[0-9]*\\]\\.(?:type|value)))?|idShort|id|endpoints\\[[0-9]*\\]\\.(?:interface|protocolinformation\\.href)))|\\$smdesc#(?:semanticId(?:\\.(?:type|keys\\[[0-9]*\\]\\.(?:type|value)))?|idShort|id|endpoints\\[[0-9]*\\]\\.(?:interface|protocolinformation\\.href)))$" | ||
| "pattern": "^(?:\\$aas#(?:idShort|assetInformation\\.assetType|assetInformation\\.globalAssetId|assetInformation\\.specificAssetIds\\[[0-9]*\\](?:\\.externalSubjectId(?:\\.keys\\[[0-9]*\\])?)?|submodels\\[[0-9]*\\](?:\\.keys\\[[0-9]*\\])?)|\\$sm#(?:semanticId(?:\\.keys\\[[0-9]*\\])?|idShort|id)|\\$sme(?:\\.[A-Za-z](?:[A-Za-z0-9_-]*[A-Za-z0-9_])?(?:\\[[0-9]*\\])*(?:\\.[A-Za-z](?:[A-Za-z0-9_-]*[A-Za-z0-9_])?(?:\\[[0-9]*\\])*)*)?(?:#(?:semanticId(?:\\.keys\\[[0-9]*\\])?|idShort|value|valueType|language))?|\\$cd#idShort|\\$aasdesc#(?:idShort|description|displayName|extension|administration|assetKind|assetType|globalAssetId|specificAssetIds\\[[0-9]*\\](?:\\.externalSubjectId(?:\\.keys\\[[0-9]*\\])?)?|endpoints\\[[0-9]*\\]|submodelDescriptors\\[[0-9]*\\](?:\\.(?:semanticId(?:\\.keys\\[[0-9]*\\])?|idShort|endpoints\\[[0-9]*\\]))?)|\\$smdesc#(?:semanticId(?:\\.keys\\[[0-9]*\\])?|idShort|endpoints\\[[0-9]*\\]))$" |
Check warning
Code scanning / QDJVMC
Typo Warning documentation
| "$ref": "#/definitions/SecurityQueryFilter", | ||
| "additionalProperties": false | ||
| }, | ||
| "FILTERLIST": { |
Check warning
Code scanning / QDJVMC
Typo Warning documentation
Martin187187
left a comment
Martin187187
left a comment
There was a problem hiding this comment.
I resolved all conflicts. This is ready for final review now
| "USEFORMULA": { | ||
| "type": "string" | ||
| }, | ||
| "FRAGMENT": { |
There was a problem hiding this comment.
yes it is fixed already
|
For review you can use https://regex101.com/ to verify regex changes and https://bnfplayground.pauliankline.com/ for bnf changes |
|
@Martin187187 I don't remember that the content of this PR was in the V3.2 scope. Can you confirm that we need it? If so, it still has the |
I added the label |
6 participants
Query Language Filterchapter inQuery Languagechapter.FieldIdentifier FragmentDefinition from just plain string to a BNF specification. => allow non leaf paths and disallow required paths.