OAGi / Score

Score
MIT License
9 stars 6 forks source link

OpenAPI - refine naming convention and check for duplicate topLevelBIE by id #1603

Closed dubnemo closed 4 months ago

dubnemo commented 7 months ago

Naming Convention

This naming convention is based on Enhanced Bachus Naur Form (EBNF) which has been used to express programming languages and syntax and semantics for decades. There are specific notational semanticssuch as [ ] means 'optional'.

The OpenAPI Document Expression should follow this naming convention:

<SchemaObject Name> ::= <UniqueTopLevelBIEName>[List][<MessageBodyType>][<ActionVerb>][Entry]

<UniqueTopLevelBIEName> ::= <BIEName> when a DEN only has one distinct BIEId in the list of usages  
[List] ::= **conditional** name; only used when the outer schema object represents an array or if the BIE has few properties selected in an inner array typically on a GET operation
[<MessageBodyType>] ::= 'Request' | 'Response'; **optional** name, only used to distinguish distinct BIEs, across different message bodies
[<ActionVerb>] :: = 'Update' | 'Delete' | 'Create' | 'Replace' | 'Read'; **optional** name used to further assist in unique DEN when multiple BIE ids exist for the same DEN.
[Entry] ::= **optional** name; used for the inner schema object of an array -- seldom used.

If there are multiple BIE ids for the same DEN, then expand to additional naming options until distinct (similar logic to how we named the columns in CSV). For example, if there to two BIE id references for the same DEN, expanding to will help make the BIE name distinct. This will cover use cases where a requestBody of a DEN will not have an id or uuid until store in a database, and a responseBody will have the id and uuid after the database auto-creates the identifier onInsert().

If more distinction is required, then add .

Expression Logic

-get list of BIE usages in OpenAPI document -for the BIEs that list, generate all reused BIEs (does not currently duplicate) -place reused BIEid, DEN in hashtable -count(BIEId), DEN and for-each BIEid where count(BIEid)=1 and if not an already generated reused BIE, then generated the Schema Object as <UniqueTopLevelBIEName> -if its usage is an array, then create another Schema object with <List>appended, and reference <UniqueTopLevelBIEName>. -Create Path entries for these; add references to the Schema Objects either <UniqueTopLevelBIEName><List>or <UniqueTopLevelBIEName> ; keep track of what has been serialized. -for-each DENs where count(BIEId) > 1, determine if the conflict can be differentiated by adding <MessageType> -Create Path entries for these, add references to the Schema Objects <UniqueTopLevelBIEName><List><MessageType> or <UniqueTopLevelBIEName><MessageType> -if it still cannot be differentiated, then add <ActionVerb> and so forth.

Moving DEN to the Extensions:

The DEN be part of an extension (x-oagis-bie-den):

      x-oagis-bie-guid: "bce7462b32d549e48a4f029c4e46f102"
      x-oagis-bie-date-time: "2023-11-15T22:54:29+0000"
      x-oagis-bie-version: "V2"
      x-oagis-bie-den: "WIP Status"
      x-oagis-bie-uri: "https://test.oagiscore.net/profile_bie/883"
      x-oagis-release: "10.10.1"
      x-oagis-release-date: "2023-11-01"

Example schema objects:

So ignoring the ASCCPs that reused with in the selected BIEs, instead of the fourteen (14) I see in the yml, of the six (6) displayed BIEs below, there should only be eight (8) schema objects appearing in this sequence:

List of BIEs for the OpenAPI document: image

@hakjuoh @elena-jelisic

dubnemo commented 6 months ago

I just updated the BNF.

If the same BIE is across multiple verbs and message types, makes sense to suppress the message type.

ShippedItemInstance in InFieldProductID API may be another good example.

elena-jelisic commented 6 months ago

@dubnemo @hakjuoh

Scott, please review the current content of the OAS document.

AgGateway In-Field Product Id-V4-1709651453849.yml.zip

I made the following changes:

  1. Order statements are added,
  2. The naming convention is changed,
  3. DEN is moved to the Extensions,
  4. ASCCPs that are reused within the selected BIEs are ignored.

I have the following questions:

  1. Where can I see the SME Manufacturing Connect Kit example you used?
  2. What should I do with reusable ASCCPs that are arrays? They are now represented as global schemas (e.g., Substance List in the file I sent). Do you think I should change this as well?
  3. You don’t want to see ASCCPs used as Request/Response message bodies as global schemas. Am I correct?
  4. Also, if I remember well, you said that you don’t need to distinguish between two BIEs that profile the same ASCCP (e.g., Shipped Item Instance with IDs 896 and 893) because they are the same, just use different examples. Am I correct?
dubnemo commented 6 months ago

See inline responses:

@dubnemo @hakjuoh

Scott, please review the current content of the OAS document.

AgGateway In-Field Product Id-V4-1709651453849.yml.zip

I opened it, and it seems the DEN is still being used. Its is not right as PUT and POST should only appear once. Or just how the operationId is setup.

image

I made the following changes:

  1. Order statements are added,
  2. The naming convention is changed,
  3. DEN is moved to the Extensions,
  4. ASCCPs that are reused within the selected BIEs are ignored.

I am assuming that you mean the first BIE occurrence (with the same Id, not DEN) is processed, and the duplicate references are ignored.

I have the following questions:

  1. Where can I see the SME Manufacturing Connect Kit example you used?

It was in oagscore.net but I deleted the BIE references to test the other delete error. I can recreate it based on the screen shot.

  1. What should I do with reusable ASCCPs that are arrays? They are now represented as global schemas (e.g., Substance List in the file I sent). Do you think I should change this as well?

No.

  1. You don’t want to see ASCCPs used as Request/Response message bodies as global schemas. Am I correct?

Our aim is to maximize the use of global schemas, as that reduces the length of the class name that gets generated by Swagger CodeGen and NSwag. So all request and response message bodies and their companion should be global schemas.

  1. Also, if I remember well, you said that you don’t need to distinguish between two BIEs that profile the same ASCCP (e.g., Shipped Item Instance with IDs 896 and 893) because they are the same, just use different examples. Am I correct?

That was a separate topic. Because the functional gap we have with a finalized solution for OpenAPI, we are forced to create multiple BIEs. Once we are able to double click on a request or response for an endpoint (see https://github.com/OAGi/Score/issues/862) to add HTTP headers, statuses, parameters, AND examples for that response, only then will we see the reduction of the number of BIEs created. I mentioned that multiple BIEs are created since the only 'example' capability we have today is at the property level in the schema object, and not at the resource level.

dubnemo commented 6 months ago

@elena-jelisic Recreated SME Manufacturing connectKit in oagiscore.net.

image

hakjuoh commented 6 months ago

@elena-jelisic Recreated SME Manufacturing connectKit in oagiscore.net.

image

@dubnemo SME Manufacturing Express Pack-V1-1709915050415.yml.zip

    wipStatus:
      x-oagis-bie-guid: "462f05ad0bf74ae283ef5cb786042556"
      x-oagis-bie-date-time: "2023-11-15T23:01:52-0500"
      x-oagis-bie-version: "V2"
      x-oagis-bie-den: "WIP Status"
      x-oagis-bie-uri: "http://localhost:4200/profile_bie/884"
      x-oagis-release: "10.10.1"
      x-oagis-release-date: "2023-11-01"
Screenshot 2024-03-08 at 11 26 31 AM
dubnemo commented 6 months ago

See the naming convention, to have resourceName first, then messageBodyType.

Plus we are back to where we were, which we have too many schema objects created. For example, WIP Status would only have two, versus four shown. S/b: WipStatusRequest WipStatusResponse

hakjuoh commented 6 months ago

See the naming convention, to have resourceName first, then messageBodyType.

Plus we are back to where we were, which we have too many schema objects created. For example, WIP Status would only have two, versus four shown. S/b: WipStatusRequest WipStatusResponse

@dubnemo SME Manufacturing Express Pack-V1-1709923045827.yml.zip

This is the result written according to the new naming convention, excluding the creation of reused BIEs as global schemas.

Screenshot 2024-03-08 at 1 38 29 PM

There are eight (8) schema objects appearing in this schema:

@elena-jelisic Here's a commit for this: https://github.com/OAGi/Score/commit/f0ed4b50405027a10a87b238395a55ad180f3141

dubnemo commented 6 months ago

@hakjuoh When you say excluding the reuse BIEs, are you still planning to make those global schemas? That would be great. The rest of it is looking good, thank you. @elena-jelisic

hakjuoh commented 6 months ago

When you say excluding the reuse BIEs, are you still planning to make those global schemas? That would be great. The rest of it is looking good, thank you.

@dubnemo SME Manufacturing Express Pack-V1-1710165325566.yml.zip

If it creates reused BIEs as global schemas, the following additional schemas will be generated. Please let me know if this is okay.

Screenshot 2024-03-11 at 9 55 56 AM
dubnemo commented 6 months ago

@hakjuoh @elena-jelisic and I met, and we did a code review, and I think it is good enough for now. I changed the title of this Issue. Let's get 3.2.2 out as soon as possible.

We also talked about the remaining items to complete OpenAPI, and populate other aspects of the model including other request and response details. I will create other issues for incremental releases, such as

dubnemo commented 6 months ago

@jim-wilson-kt @michaelfigura @sfohn @kbserm FYI @elena-jelisic and I noticed yesterday we lost all the OpenAPI wireframes in Confluence and it did not seem to retain even a PNG rendition. I spent probably 30 hours on those wireframes. Perhaps this was due to the branding to Atlas.

dubnemo commented 6 months ago

@hakjuoh Hate to inform you that we still have some work to do on this one. I still see multiple schema objects for the same BIE id (scope of this issue). For example for AgGateway In-Field Product Id, there are two schema objects for https://oagiscore.net/profile_bie/909.

I have refined the 'naming convention' above to hopefully provide additional clarity. It has been difficult to articulate. Please note that BNF has optional semantics with the square bracket notation. That meant we only use the feature as needed when further uniqueness is required.

Screenshot from 2024-03-17 18-49-38

We may need a quick 3.2.3 release. @elena-jelisic @kbserm

I have attached a zip with the corrected and the currently expressed yml. Hopefully naming convention matches what I am trying to achieve.

AgGateway In-Field Product Id-V4-Corrected_withOriginal.zip

dubnemo commented 6 months ago

You may have missed my comment @hakjuoh.
We are still creating too many schema objects with the same BIE id. See my comment of last week, and more importanly yesterday when I tested In-Field Product Id.
I edited the title last week to capture the too many schema objects for the same BIE id. This issue was to consolidate the prior issue(s) (e.g., #1591) related to the multiple schema objects for the same BIE id.

hakjuoh commented 6 months ago

@dubnemo @kbserm @elena-jelisic I missed your comment. I hadn't noticed the changes regarding this issue.

<SchemaObject Name> ::= <UniqueTopLevelBIEName>[List]][<MessageBodyType>][<ActionVerb>][Entry]

<UniqueTopLevelBIEName> ::= <BIEName> when a DEN only has one BIE Id.  
[List] ::= optional name; used when the outer schema object represents an array
[<MessageBodyType>] ::= optional name, used to distinguish distinct BIEs, across different message bodies
[<ActionVerb>] :: = 'Update' | 'Delete' | 'Create' | 'Replace'; optional name used to further assist in unique DEN when multiple BIE ids exist for the same DEN.
[Entry] ::= optional name; used for the inner schema object of an array

When applying this new naming rules, the SchemaObjectName will be determined as follows.

BIE DEN Verb Message Type SchemaObjectName
BOM. BOM GET Request Unsupported
BOM. BOM GET Response bom(List)Response(Entry)
BOM. BOM PUT Request bom(List)RequestReplace(Entry)
BOM. BOM PUT Response bom(List)ResponseReplace(Entry)
BOM. BOM POST Request bom(List)RequestCreate(Entry)
BOM. BOM POST Response bom(List)ResponseCreate(Entry)
BOM. BOM DELETE Request Unsupported
BOM. BOM DELETE Response bom(List)ResponseDelete(Entry)
BOM. BOM PATCH Request bom(List)RequestUpdate(Entry)
BOM. BOM PATCH Response bom(List)ResponseUpdate(Entry)

To completely eliminate duplicate names, however, the 'Operation Name' should also be considered. Otherwise, it will make duplicate names as follows.

BIE DEN Verb Message Type Operation Name SchemaObjectName
BOM. BOM GET Response /manufacturing bom(List)Response(Entry)
BOM. BOM GET Response /engineering bom(List)Response(Entry)

Alternatively, allowing users to modify the schema object name for each operation on the UI would be another option. Please let me know what you think.

dubnemo commented 5 months ago

@hakjuoh The optional values should only be added to the name ONLY AS-NEEDED to avoid duplicates. If you look across the unique DEN list, and there is only one id for a DEN, you know the schema object name can be the DEN.

I attached in my response above an edited In-Field Product ID yaml as an example. Note it does not have the in this case for reasons below.

However there are multiple outer or 'wrapper' schema object that reference that single DEN / BIE Id:

Therefore the example 'wrapper' schema objects in the yaml do NOT have the because they both point to the same BIE id.

If the DEN referred in the GET, PUT or the POST pointed to a different BIE id, then the rule would kick in and the wrapper would reference the different DEN. Let's say, for this scenario, the GET /shipped-item-instance-list endpoint pointed to a different BIE id=919. Then in addition to the above schema objects, we would have:

This naming convention keeps the schema object and class names shorter.

The operation id is explicitly mapped to the controller name in both NSwag and CodeGen. Not sure how that matters from a schema object naming. It is not a path as you show. I do try to ensure the operation ids are named the same for pairs of requests and responses.

@kbserm @elena-jelisic

hakjuoh commented 4 months ago

@dubnemo

The following are the results of generating OpenAPI documents for three items worked on oagiscore.net, applying the updates you provided. Please review them.

SME Manufacturing Express Pack-V1-1713970117116.yml.zip AgGateway In-Field Product Id-V4-1713970053102.yml.zip Traceability API-V1-1713969959147.yml.zip

In 'Traceability API,' there is duplication occurring as it reuses Container [ID=862] from Critical Event [ID=873] BIE, which is not defined within the corresponding OpenAPI Document. As a result, an object named 'container862' is generated.

@kbserm @elena-jelisic

dubnemo commented 4 months ago

@hakjuoh You will want to see the version attached (diff) we have edited down for AgGateway In-Field Product Id. It follows the convention. There is a shippedItemInstance schema object for the profiled BIE, and a shippedItemInstanceList schema object. All the verb-request and verb-response combinations that use that BIE, all reference the ShippedItemInstanceList. No Response or Request required.

I am getting close to completion of the state machine, but may not be completely ready by the time of the meeting. But we can go through the first layer of it, before we get to verb/message type details. I really follows how I manually edit the connectCenter output today. @kbserm @elena-jelisic

AgGateway.In-Field.Product.Id-V4_openapi_manualEdits.zip

hakjuoh commented 4 months ago

@dubnemo

Here's a fixed one. All schema object names are referred to shippedItemInstanceList. I uploaded this version of the connectCenter to the test instance with the prod instance's data.

AgGateway In-Field Product Id-V4-1714062510597.yml.zip

dubnemo commented 4 months ago

Summary of Score Meeting 2024-04-25:

Scott ( @dubnemo ) walked through the state diagram showing:

The ArgoUML model got corrupted with 'ghost' state in it, and Scott will determine if I can repair by deleted from the XMI (done that in the past), or have to start over.

@kbserm @hakjuoh @joshklm @tekratz @michaelfigura @elena-jelisic

hakjuoh commented 4 months ago

@dubnemo

The following is the result of applying the modified logic discussed in the last meeting. You can also test it on the test instance.

SME Manufacturing Express Pack-V1-1714654264466.yml.zip AgGateway In-Field Product Id-V4-1714654245755.yml.zip Traceability API-V1-1714654223071.yml.zip

dubnemo commented 4 months ago

@hakjuoh InFieldProductId still has shippedItemInstanceListEntry. Should be shippedItemInstance. No different than how measurementListreferences measurement.

This relates to this part of the logic: