Skip to content

Conversation

tim-lai
Copy link
Contributor

@tim-lai tim-lai commented Aug 30, 2022

Ref: #1975, #1976, #1977, #1978, #1979, #1980, #1981, #1982, #1983, #1984, #1985

Description

Render documentation in Monaco for the following elements:

  • externalDocumentation
  • license
  • openapi3_1
  • operation
  • paths
  • pathItem
  • parameter
  • responses
  • server
  • serverVariable
  • tag

refer to comments for additional progressive setup/screenshots/notes.

Motivation and Context

Build out apidom-ls rules for OAS3.1

Ref: #1975
Ref: #1976
Ref: #1977
Ref: #1978
Ref: #1979
Ref: #1980
Ref: #1981
Ref: #1982
Ref: #1983
Ref: #1984
Ref: #1985

How Has This Been Tested?

local. No new tests yet.

Screenshots (if appropriate):

Checklist

My PR contains...

  • No code changes (src/ is unmodified: changes to documentation, CI, metadata, etc.)
  • Dependency changes (any modification to dependencies in package.json)
  • Bug fixes (non-breaking change which fixes an issue)
  • Improvements (misc. changes to existing features)
  • Features (non-breaking change which adds functionality)

My changes...

  • are breaking changes to a public API (config options, System API, major UI change, etc).
  • are breaking changes to a private API (Redux, component props, utility functions, etc.).
  • are breaking changes to a developer API (npm script behavior changes, new dev system dependencies, etc).
  • are not breaking changes.

Documentation

  • My changes do not require a change to the project documentation.
  • My changes require a change to the project documentation.
  • If yes to above: I have updated the documentation accordingly.

Automated tests

  • My changes can not or do not need to be tested.
  • My changes can and should be tested by unit and/or integration tests.
  • If yes to above: I have added tests to cover my changes.
  • If yes to above: I have taken care to cover edge cases in my tests.
  • All new and existing tests passed.

@tim-lai
Copy link
Contributor Author

tim-lai commented Aug 30, 2022

Initial definition (modified fixture):

openapi: 3.1.0
info:
  title: deref
  version: 1.0.0
servers:
  - description: local
    url: http://localhost:8082/
paths:
  /a:
    get:
      operationId: aget
      parameters:
        - $ref: '#/components/parameters/userId'
    servers:
    - description: extendedm
      url: http://localhost:8080
    parameters:
    - name: id
    in: path
    description: ID of pet to use
    required: true
    schema:
      type: array
      items:
        type: string  
    style: simple
    post:
      operationId: apost
  /b:
    get:
      operationId: bget
      parameters:
        - $ref: '#/components/parameters/userId'
    post:
      operationId: bpost
      requestBody:
        content:
          application/json:
            schema:
              $ref: '#/components/schemas/foo'
components:
  schemas:
    foo:
      type: string
    bar:
      $id: http://localhost:8082/
      type: string
  parameters:
    userId:
      $ref: '#/components/parameters/indirection1'
      description: override
    indirection1:
      $ref: '#/components/parameters/indirection2'
      summary: indirect summary
    indirection2:
      $ref: '#/components/parameters/userIdRef'
      summary: indirect summary
    userIdRef:
      name: userId
      in: query
      description: ID of the user
      required: true
    externalRef:
      $ref: ./ex.json#/externalParameter
      description: another ref

apidom-ls-pr-1946-1

@tim-lai
Copy link
Contributor Author

tim-lai commented Aug 31, 2022

Notes:

@tim-lai
Copy link
Contributor Author

tim-lai commented Aug 31, 2022

@frantuma working on serverVariables in dff8c8e, but I can't get it to display.

If this snippet here is uncommented, then server.variables will render this "target". In addition, syntax highlighting will be present if servers-variables is enabled in symbols.ts and config.ts (currently commented out). Note the plural "s".

apidom-ls-pr-1946-3-serverVariables-as-target

I can verify in apidom-playground, that the element is found and described as "content": "server-variables". Note the singular "server".

I tried different naming conventions in symbols.ts and config.ts, to no avail. The alternative names are currently commented out.

Here's the minimal spec used for apidom-playground:

openapi: 3.1.0
info:
  title: deref
  version: 1.0.0
servers:
  - description: local
    url: http://localhost:8082/
    variables:
      username:
        # note! no enum here means it is an open value
        default: demo
        description: this value is assigned by the service provider, in this example `gigantic-server.com`
      port:
        enum:
          - '8443'
          - '443'
        default: '8443'
      basePath:
        # open meaning there is the opportunity to use special base paths as assigned by the provider, default is `v2`
        default: v2

Also the syntax highlight doesn't apply in this screenshot:

apidom-ls-pr-1946-2-serverVariables

Seems like there's a difference between apidom parsing as server-variables vs apidom-ls interpreting as servers-variables. Any suggestions on where I can go investigate further and resolve?

@frantuma
Copy link
Contributor

frantuma commented Sep 1, 2022

@tim-lai

If this snippet here is uncommented, then server.variables will render this "target".

This is correct, as variables is a field of server and therefore it's picked up. This is however true only if there is no "root docs" defined for server-variables element If you defined (instead) docs for server-variables, they will be picked up with preference over the one defined with target

@frantuma working on serverVariables in dff8c8e, but I can't get it to display.

Not sure here what you mean? display the docs? or? see above

In addition, syntax highlighting will be present if servers-variables is enabled in symbols.ts and config.ts (currently commented out). Note the plural "s".

I don't think this is correct: servers-variables is not a defined "classifier" (either element field or member of classes of an apidom ns element, see also here).

The "classifier" defined for the server-variables element is server-variables. I have seen you added to tokens.ts in the branch, and this is correct and sufficient to have it "considered" in syntax highlighting.

File symbols.ts is not related to syntax highlight. I would remove server-variables from there, as this is the list of elements considered when F1->Go to Symbol is triggered, and I would say server variables would "pollute" the list.

In order to have some visible highlight, obviously it must be configured on the editor side as well

@frantuma
Copy link
Contributor

frantuma commented Sep 1, 2022

@tim-lai 2 additional not so related comments:

  • you added pathItem-parameters to symbols.ts, again I would remove it because of same reason I mentioned above (also the correct "classifier" would be path-item-parameters)
  • I would remove the comma from An optional, string description in path item description docs, as you did for summary

@tim-lai
Copy link
Contributor Author

tim-lai commented Sep 1, 2022

@frantuma

Not sure here what you mean? display the docs? or? see above

It's not picking up docs from src/config/openapi/server-variable/documentation.ts.

apidom-ls-pr-1946-3-serverVariables-parent-no-render

I also tried adding the following to src/config/openapi/config.ts, without effect

serverVariable: serverVariableMeta,

@tim-lai
Copy link
Contributor Author

tim-lai commented Sep 1, 2022

2 additional not so related comments:

  • you added pathItem-parameters to symbols.ts, again I would remove it because of same reason I mentioned above (also the correct "classifier" would be path-item-parameters)
  • I would remove the comma from An optional, string description in path item description docs, as you did for summary

Resolved. Also removed the above mentioned server-variables in symbols.ts.

At this moment, only remaining issue is rendering docs from server-variable in above comment

@frantuma
Copy link
Contributor

frantuma commented Sep 1, 2022

@tim-lai

It's not picking up docs from src/config/openapi/server-variable/documentation.ts.

Still not clear to me if we are referring to singular (serverVariable) or plural (server-variables).

A couple of points to recap/clarify:

  1. There are still some inconsistencies in naming classifiers. Some are using CamelCase (e.g. serverVariable) and some kebab-case (e.g. server-variables); this is a known issue which will be fixed.

  2. server-variables: the variables field of Server. As mentioned you can either add docs to server with target variables (currently commented out in your branch), or add a "meta" for element server-variables; in your branch you have currently serverVariables defined, this should be changed to 'server-variables': serverVariablesMeta,.

Notice also that you are mixing plural/singular as in current code relates variables to variable (singular) meta. You should instead add a meta directory for server-variables and set docs there, and import/use that one in config.

Alternatively, and possibly easier in this case, uncommenting the docs in server for target variables would do the work, also removing line above.

  1. serverVariable singular: a variable within the variables. your current code in line 37 should be ok, the docs would appear hovering a variable key

@tim-lai
Copy link
Contributor Author

tim-lai commented Sep 1, 2022

@frantuma

2. this should be changed to 'server-variables': serverVariablesMeta,.

This did the trick! This is my preferred solution, to import the meta docs. PR updated in 393ca65.

For clarity:

  • the meta directory is for server-variable, singular. This is consistent with Server Variable Object, even though the Server Object fixed fields has a field name for variables, plural.
  • the element name found by apidom is server-variables, plural.
  • will deal with syntax highlighting for variables in SwaggerEditor later.
  1. There are still some inconsistencies in naming classifiers. Some are using CamelCase (e.g. serverVariable) and some kebab-case (e.g. server-variables); this is a known issue which will be fixed.

Ok, good to know. Let me know if/when changes are needed. Also, where would I have been able to see these naming classifiers?

@frantuma
Copy link
Contributor

frantuma commented Sep 1, 2022

@tim-lai not sure if we are there yet

IIANM, your current code would cause the rendering of this doc when hovering the key variables. This is not correct, as the docs for variables should be

Map[string, [Server Variable Object](https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.1.0.md#serverVariableObject)] 	A map between a variable name and its value. The value is used for substitution in the server's URL template.

As mentioned in comment above:

Notice also that you are mixing plural/singular as in current code relates variables to variable (singular) meta. You should instead add a meta directory for server-variables and set docs there, and import/use that one in config.

@frantuma
Copy link
Contributor

frantuma commented Sep 1, 2022

about "classifiers", these are either element field or member of classes of an apidom ns element, see also here.

In local code, the most handy way atm is to search in root path for this.element = ', this.classes.push(' and primaryClass = ' in .ts files

@tim-lai
Copy link
Contributor Author

tim-lai commented Sep 1, 2022

@frantuma

IIANM, your current code would cause the rendering of this doc when hovering the key variables. This is not correct, as the docs for variables should be

Map[string, [Server Variable Object](https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.1.0.md#serverVariableObject)] 	A map between a variable name and its value. The value is used for substitution in the server's URL template.

That was my intent, because Server Variable Object has its own meta documentation.

If we go back and revisit pathItem.operation (paths -> /a -> get), the hover displays the Operation meta, and not the table description in Path Item Object fixed fields

That said, upon further review, server.variables is a Map of a string or object, in which case it is more correct to use the table description. There's another case for PathItem.parameters, of an array of Parameter Object OR Reference Object, to which the table description would be more correct to use.

So would this be an acceptable standard?

  • if only a single Object type, and meta is available, use the meta. E.g. Operation object
  • if array of singe Object type, and meta is available, use the meta. E.g. Servers object
  • if multiple type, use the table description. E.g. Server Variable

fwiw, the Operation Object meta documentation follows this standard, but that's because I haven't gotten to building out the available meta fields for it. :)

Another question, with the above standard, how would a user would visualize a meta data object like server-variables, or tag, or the Reference Object (which is always paired with a different Object)?

Separately, I'll change the meta directory name to server-variables, which would match more closely with apidom rather that the literal naming in the specification.

@frantuma
Copy link
Contributor

frantuma commented Sep 2, 2022

@tim-lai

I am having difficulties understanding your point(s):

That was my intent, because Server Variable Object has its own meta documentation.

A Variable has its own doc, which should be rendered in hover when hovering over the variable key. variables has its own docs, as mentioned above, which render in hover when hovering the variables field in server.

This is exactly the same as the logic applied to asyncapi, which got its variables and stuff from openapi. Please try hovering the following doc loaded in editor next:

asyncapi: 2.4.0
info:
  title: Streetlights Kafka API
  version: 1.0.0
servers:
  test:
    url: test.mykafkacluster.org:8092
    protocol: kafka-secure
    description: Test broker
    variables: 
      foo: 
        description: test

If we go back and revisit pathItem.operation (paths -> /a -> get), the hover displays the Operation meta, and not the table description in Path Item Object fixed fields

see above, operation is a "single" value representing an operation, comparable to variable, not variables

That said, upon further review, server.variables is a Map of a string or object, in which case it is more correct to use the table description. There's another case for PathItem.parameters, of an array of Parameter Object OR Reference Object, to which the table description would be more correct to use.

I am not sure I understand what you mean, please check how done in asyncapi with parameters and references

So would this be an acceptable standard?

if only a single Object type, and meta is available, use the meta. E.g. Operation object
if array of singe Object type, and meta is available, use the meta. E.g. Servers object
if multiple type, use the table description. E.g. Server Variable

Not sure I understand, puzzled by "multiple type", "table description". I would again point to asyncapi behavior, the "standard" is to use the docs provided by the spec relative to the field. So in your example Operation is here, Servers is here and Server Variable is here

fwiw, the Operation Object meta documentation follows this standard, but that's because I haven't gotten to building out the available meta fields for it. :)

Not sure I get this, please look at AsyncAPI operation, this is the same. In your current code it looks ok I guess, with fields docs (with target) currently only partially implemented

Another question, with the above standard, how would a user would visualize a meta data object like server-variables, or tag, or the Reference Object (which is always paired with a different Object)?

Assuming "visualize" means rendering the docs on hover, this is covered by previous points, see asyncapi:

server-variables: Map[string, [Server Variable Object](https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.1.0.md#serverVariableObject)] A map between a variable name and its value. The value is used for substitution in the server's URL template.

tag: https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.1.0.md#tagObject

$ref: see async, e.g. for parameter

@tim-lai
Copy link
Contributor Author

tim-lai commented Sep 2, 2022

@frantuma

Alrighty, I think I am understanding better now. Cumulative updates for review in ef487ad.

Based on all the comments and changes, I'm thinking this line can safely be removed (along with the import and the referenced directory)?

// 'server-variables': serverVariablesMeta,
as it now lives here:

@tim-lai
Copy link
Contributor Author

tim-lai commented Sep 6, 2022

@frantuma

current fixture, up to e9d8130.

  • paths
  • pathItem
  • parameters
  • servers
  • server
  • serverVariable
  • operation (now also built out)
  • license
  • tags
  • externalDocumentation
  • responses
openapi: 3.1.0
info:
  title: deref
  version: 1.0.0
  license:
    name: Apache 2.0
    identifier: Apache-2.0
  contact:
    name: API Support
    url: https://www.example.com/support
    email: support@example.com
servers:
  - description: local
    url: http://localhost:8082/
    variables:
      username:
        # note! no enum here means it is an open value
        default: demo
        description: this value is assigned by the service provider, in this example `gigantic-server.com`
      port:
        enum:
          - '8443'
          - '443'
        default: '8443'
      basePath:
        # open meaning there is the opportunity to use special base paths as assigned by the provider, default is `v2`
        default: v2
tags:
  - name: first tag
    description: basic first tag
  - name: second tag with external
    description: tag description with externalDoc
    externalDocs:
      description: Find out more
      url: http://swagger.io
externalDocs:
  description: externalDocs for main schema
  url: http://localhost:8080
webhooks:
security:
jsonSchemaDialect:
paths:
  /a:
    get:
      operationId: aget
      tags:
        - name: apple
          description: operation apple
        - name: banana
          description: operation banana
      summary: short explanation
      description: longer explanation
      deprecated: false
      parameters:
        - $ref: '#/components/parameters/userId'
      externalDocs:
        description: externalDocs for operation /get a
        url: http://localhost:8080
      servers:
      - description: extendedm
        url: http://localhost:8080
      - description: secondServ
        url: http://localhost:8081
      variables:
        color:
          default: blue
      responses:
        '200':
          description: a pet to be returned
          content: 
            application/json:
              schema:
                $ref: '#/components/schemas/Pet'
          headers:
            description: The number of allowed requests in the current period
            schema:
              type: integer
          links:
        default:
          description: Unexpected error
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/ErrorModel'
    post:
      operationId: apost
      parameters:
      - name: id
        in: path
        description: ID of pet to use
        required: true
        schema:
          type: array
          items:
            type: string  
        style: simple
        examples:
        example:
  /b:
    get:
      operationId: bget
      parameters:
        - $ref: '#/components/parameters/userId'
    post:
      operationId: bpost
      requestBody:
        content:
          application/json:
            schema:
              $ref: '#/components/schemas/foo'

@tim-lai tim-lai changed the title Feat/oas31 documentation 1 feat(apidom-ls): oas31 documentation Sep 7, 2022
Copy link
Contributor

@char0n char0n left a comment

Choose a reason for hiding this comment

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

@tim-lai thanks for starting the work on this. I've finished my deep code review and suggested many changes and enhancements. The goal now should be to stabilize code in this PR, not produce any new stuff and get this PR into state that is mergable, so that rules for OpenAPI 3.0.x production can start in parallel.

Then it would be good if process of producing further documentation for OpenAPI 3.1.0 changes and instead of producing huge PRs, small PRs containing documentation for one/two specification objects would be more appropriate. It would make code reviews more effective and quicker.

@tim-lai tim-lai force-pushed the feat/oas31-documentation-1 branch from 9f29206 to ae23041 Compare September 7, 2022 21:22
@tim-lai
Copy link
Contributor Author

tim-lai commented Sep 7, 2022

@char0n Thanks for the detailed review! I've either resolved or commented on all requested changes, please take another look.

Summary:

  • add targetSpecs anywhere that there is a link to 3.1 in the docs
  • moved tags and servers to openapi3_1 module
  • replaced k/v pair's value with the actual string instead of a const obj=''
  • commented on externalDocs, left as unresolved comment/change
  • removed directories no longer needed
  • cleanup all comments/placeholders

2 additional questions:

  1. CI is reporting commitlint errors, but they don't appear to be from this PR directly. Thoughts? Here's the latest CI run: https://github.com/swagger-api/apidom/runs/8237567429?check_suite_focus=true
  2. I did experiment with adding the documentation in config/openapi/oas3_1 to config/common/schema instead, but the change was not registering. Did not investigate further, as the request was for the config/openapi/oas3_1 module. But I am curious if there's an obvious reason or something I missed.

@char0n
Copy link
Contributor

char0n commented Sep 8, 2022

@char0n Thanks for the detailed review! I've either resolved or commented on all requested changes, please take another look.

Summary:

  • add targetSpecs anywhere that there is a link to 3.1 in the docs
  • moved tags and servers to openapi3_1 module
  • replaced k/v pair's value with the actual string instead of a const obj=''
  • commented on externalDocs, left as unresolved comment/change
  • removed directories no longer needed
  • cleanup all comments/placeholders

Thanks for changes. Doing second round of CR now.

2 additional questions:

  1. CI is reporting commitlint errors, but they don't appear to be from this PR directly. Thoughts? Here's the latest CI run: https://github.com/swagger-api/apidom/runs/8237567429?check_suite_focus=true

Nothing to worry about. Only linting of commit messages failed. This PR is going to be squash-merged anyway and new commit msg will be provided.

image

  1. I did experiment with adding the documentation in config/openapi/oas3_1 to config/common/schema instead, but the change was not registering. Did not investigate further, as the request was for the config/openapi/oas3_1 module. But I am curious if there's an obvious reason or something I missed.

Let's deal with this separately after this PR. We can schedule a session where we can talk about rules of configs production.

@char0n
Copy link
Contributor

char0n commented Sep 8, 2022

commented on externalDocs, left as unresolved comment/change

@tim-lai I've reacted there, have a look at it please.

After we address rest of this comments, we should be able to go for merge during today/tommorow.

@tim-lai
Copy link
Contributor Author

tim-lai commented Sep 8, 2022

@char0n

After we address rest of this comments, we should be able to go for merge during today/tommorow.

Requested changes should now all be resolved. Please have one more look. Thanks!

@tim-lai tim-lai changed the title feat(apidom-ls): oas31 documentation feat(apidom-ls): oas3.1 documentation Sep 8, 2022
@char0n char0n added enhancement New feature or request OpenAPI 3.1 ApiDOM labels Sep 9, 2022
Copy link
Contributor

@char0n char0n left a comment

Choose a reason for hiding this comment

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

LGTM, nicely done.

Let me just reiterate what I wrote before: It would be good if process of producing further documentation for OpenAPI 3.1.0 changes, and instead of producing huge PRs, small PRs containing documentation for one/two specification objects would be more appropriate. It would make code reviews more effective and quicker.

Last thing: I recommend squash+merging this PR into one if possible with following commit msg:

feat(ls): add documentation rules for OpenAPI 3.1.0

Ref <issue number>

@tim-lai tim-lai changed the title feat(apidom-ls): oas3.1 documentation feat(ls): add documentation rules for OpenAPI 3.1.0 Sep 9, 2022
@tim-lai tim-lai merged commit e358892 into main Sep 9, 2022
@tim-lai tim-lai deleted the feat/oas31-documentation-1 branch September 9, 2022 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants