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

Bugfix V3.0.2 #222

Merged
merged 25 commits into from
Jul 3, 2024
Merged

Bugfix V3.0.2 #222

merged 25 commits into from
Jul 3, 2024

Conversation

sebbader-sap
Copy link
Contributor

Successor of #202

@sebbader-sap sebbader-sap added this to the 3.0.2 milestone Jan 29, 2024
@sebbader-sap
Copy link
Contributor Author

Repeating comment #202 (comment):

arnoweis: I think this PR should already include a solution to #199

@sebbader-sap
Copy link
Contributor Author

Repeating #202 (comment):

BirgitBoss: I think there is no bugfix for the lookup of shells with encoding. It needs to be completely removed from the openAPI specification und substituted by #19

@sebbader-sap
Copy link
Contributor Author

Closes #191

* remove "submodelElements" from SubmodelValue

* Update Part2-API-Schemas/openapi.yaml
sebbader-sap and others added 6 commits March 12, 2024 10:24
* change servicedescription/profiles enum to an open list, providing predefined values only as examples
* remove the Level modifier from $metadata requests

* remove allOf SubmodelElementAttributes from SubmodelElementMetadata
add array<pathitem> where missing + new pathitem regex
@BirgitBoss
Copy link
Collaborator

@sebbader-sap shouldn't we remove this PR? In the end we will make a PR from IDTA-01001_preparation to main. For V3.0.2 we first need to do the changes in IDTA-01002_preparation as well. Otherwise, no real review is possible.

@sebbader-sap
Copy link
Contributor Author

Depends on #289

@sebbader-sap sebbader-sap marked this pull request as ready for review June 21, 2024 08:08
@sebbader-sap
Copy link
Contributor Author

@sebbader-sap shouldn't we remove this PR? In the end we will make a PR from IDTA-01001_preparation to main. For V3.0.2 we first need to do the changes in IDTA-01002_preparation as well. Otherwise, no real review is possible.

@BirgitBoss there is no other preparation branch for the V3.0.2 bugfix, only this one. As soon as #288 is approved and merged, we can merge this PR, and by that finish the bugfix.

@BirgitBoss BirgitBoss self-requested a review June 25, 2024 18:08
Copy link
Collaborator

@BirgitBoss BirgitBoss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comments

major: encoded values within array of assetLinks, encode each value individually, not the array (not explained at all in Change Notes in Part 2 V3.0.2?)

Copy link

@waltersve waltersve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comments

Part1-MetaModel-Schemas/openapi.yaml Show resolved Hide resolved
Part1-MetaModel-Schemas/openapi.yaml Outdated Show resolved Hide resolved
Part1-MetaModel-Schemas/openapi.yaml Show resolved Hide resolved
@@ -566,7 +551,7 @@ components:
aasIds:
items:
type: string
pattern: "^[\\x09\\x0A\\x0D\\x20-\\uD7FF\\uE000-\\uFFFD\\U00010000-\\U0010FFFF]*$"
pattern: "^([\\x09\\x0a\\x0d\\x20-\\ud7ff\\ue000-\\ufffd]|\\ud800[\\udc00-\\udfff]|[\\ud801-\\udbfe][\\udc00-\\udfff]|\\udbff[\\udc00-\\udfff])*$"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that really required to allow tab, line feed and carriage return (newline) for an id?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's what the metamodel defines, therefore, we have to use exactly this pattern...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We wanted to support IRI, does IRI allow tab, line feed and carriage return (newline)?

oneOf:
- $ref: "#/components/schemas/StringValue"
- $ref: "#/components/schemas/NumberValue"
- $ref: "#/components/schemas/BooleanValue"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case of date range it could help to be more precise and define a DateValue:

DateValue: 
  type: string
  format: date-time

Without that every date format could be used and would be be not interoperable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Sven, this would be a topic for https://github.com/admin-shell-io/aas-specs and then also for the next minor (major?) release version. Therefore, we won't change it in this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure this would be backward compatible to include it in V3.1 but I added a corresponding issue: admin-shell-io/aas-specs#444

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The addition of string was part of the API bugfix für ValueOnly, it was not there before, only numerical. So it would be possible to also add DateValue as part of the bugfix.

@sebbader-sap
Copy link
Contributor Author

@BirgitBoss @waltersve please have a look again. I have incorporated your comments as much as they were still valid for the current state. It looks like some of the comments have been overruled by #289 , at least that's the only way how I can explain the inconsistent state of comments and what is on the branch...

Copy link

@waltersve waltersve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

@sebbader-sap sebbader-sap merged commit 724248b into main Jul 3, 2024
1 check passed
@sebbader-sap sebbader-sap deleted the IDTA-01002-3-0_bugfix-2-preparation branch July 3, 2024 15:50
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

Successfully merging this pull request may close these issues.

4 participants