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

Initial CAMARA OAS linting ruleset #74

Merged
merged 16 commits into from
Feb 9, 2024

Conversation

rartych
Copy link
Collaborator

@rartych rartych commented Oct 12, 2023

What type of PR is this?

Add one of the following kinds:

  • enhancement/feature

What this PR does / why we need it:

The list of core Spectral OAS rules and proposed additional rules to be used for linting OAS files within subprojects

Which issue(s) this PR fixes:

Fixes #15

Special notes for reviewers:

Draft version for discussion, evolution and implementation details expected in the next round

Changelog input

Documentation for OAS linting ruleset added

Additional documentation

Spectral docs

### Rule severity

The `severity` keyword is optional in rule definition and can be `error`, `warn`, `info`, `hint`, or `off`.
The default value is `warn`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should discuss what is the expected outcome for each severity. For example should warn result in a change and re-lint, or can it be ignored?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point. Maybe we can differentiate along the following lines:

  • warnings can be ignored when introducing linting first time
    • OR (alternative) warnings can be ignored if correcting them would introduce a breaking change
  • warnings shall not be introduced with a change (e.g. within a merge, that's the point where linting is done)

But that's to clarify in a next version. Will you open an issue for that @Kevsy ?

@Kevsy
Copy link
Collaborator

Kevsy commented Oct 12, 2023

Excellent start @rartych - I 've submitted an Approval, and have made one question in the review about the expected outcome for each severity, which we can include n the next version.


*Severity*: `hint`

## 3. API Definition
Copy link
Collaborator

@sachinvodafone sachinvodafone Oct 17, 2023

Choose a reason for hiding this comment

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

Here are a few valid scenarios that we can consider for API validation.

  Topic   Description
Openapi version Camara APIs MUST be based on OpenAPI specification with minimum Version 3.0.0.
Request body structure on PUT, POST, PATCH POST / PUT / PATCH methods must have a requestBody. The requestBody content must be "application/json" or "application/xml".
Info object: title, version, description "Info" section is mandatory. Title, Description and Version must be documented.Note: It is provided by Spectral.
Info object: API version format API version within the Info section must follow the format: X.Y.Z
Valid basepath validation API must have valid basepath
Camel case / lower case /Snake_Case Paths URI (tasks, individual resources, etc.) MUST be  lower case and hyphens. OperationIds must be defined in lowerCamelCase. Objects must be defined in CamelCase inside properties field. The event type must be written in UPPER_SNAKE_CASE
Path URI URIs must contain the "major version" of the API.
Resource path must not contain the base URL Path must not contain the base path.
Sensitive data (msisdn/imsi) cannot be a path or query parameter Sensitive data (msisdn/imsi) cannot be a path or query parameter.Note: Needs to list down if we have other sensitive parameters other than MSISDN/IMSI
Valid methods Valid methods method are: GET, PUT, POST, DELETE, PATCH, OPTIONS, HEAD
GET/DELETE methods must not accept a "body" 'GET' and 'DELETE' http methods MUST NOT accept a 'requestBody' attribute
Define common error response pattern Following error responses must be define: 400, 401, 403, 404, 405, 406, 429, 500, 501, 502, 503, 504.Note: List needs to be validated as per Camara API documentation
Verb methods cannot be in the Path Resources must not contain the method name get, put, post, delete, patch.
Parameters must have a description All parameters must have a description.
Path version must be major versions only Path must contain the major API version with format /vX.Example: paths: "/device-status/v0/roaming"
GET/DELETE methods must not accept a "body" 'GET' and 'DELETE' http methods MUST NOT accept a 'requestBody' attribute
Mandatory request body with POST, PATCH and PUT methods The requestBody object MAY be present when the operation is used with one of PUT, POST, PATCH HTTP Verbs.
Summary associated with each operation Summary must be defined on each operation, describing with a short summary what the operation does.
Include valid response examples For each response code, we must define response payload examples

@rartych
Copy link
Collaborator Author

rartych commented Oct 19, 2023

Thank you @sachinvodafone for your input. I have added comments inline:
${\color{green} green }$ indicates possible new rules ${\color{blue} blue}$ marks already proposed rules and ${\color{red} red }$ shows some issues.

Some topics have to wait for clarification and update of Design Guidelines.

Topic Description
1. Openapi version Camara APIs MUST be based on OpenAPI specification with minimum Version 3.0.0.
Currently version 3.0.3 is recommended cf. API Design Guidelines: 11. Definition in OpenAPI
New rule: ${\color{green} camara-oas-version }$
2. Request body structure on PUT, POST, PATCH POST / PUT / PATCH methods must have a requestBody. The requestBody content must be "application/json" or "application/xml".
It can be expressed as:
  • A request body is mandatory for PUT, POST, PATCH calls :
    New rule: ${\color{green} camara-post-require-request-body }$
  • The requestBody content must be "application/json" or "application/xml":
    New rule: ${\color{green} camara-request-body-content-type }$ - update of Design Guidelines needed
  • 3. Info object: title, version, description "Info" section is mandatory. Title, Description and Version must be documented.Note: It is provided by Spectral.
    Spectral rules ${\color{blue} info-contact; info-description; info-license }$
    Do we need rules for ${\color{green} info-title; info-version}$ ?
    4. Info object: API version format API version within the Info section must follow the format: X.Y.Z
    New rule: ${\color{green} camara-info-version-format }$ - example in https://lornajane.net/posts/2020/custom-openapi-style-rules-with-spectral
    5. Valid basepath validation API must have valid basepath
    Issue on basePath: https://github.com/camaraproject/Commonalities/issues/71
    6. Camel case / lower case /Snake_Case Paths URI (tasks, individual resources, etc.) MUST be lower case and hyphens. OperationIds must be defined in lowerCamelCase. Objects must be defined in CamelCase inside properties field. The event type must be written in UPPER_SNAKE_CASE
    _Proposed rules ${\color{blue} camara-operationid-casing-convention;}$ ${\color{blue}camara-parameter-casing-convention; }$ ${\color{blue}camara-property-casing-convention; }$ ${\color{blue} camara-schema-casing-convention }$
    - possible update of Design Guidelines
    ${\color{red} event \space type \space format \space has\space changed\space with\space CloudEvents }$
    7. Path URI URIs must contain the "major version" of the API.
    Issue on alpha/beta labels https://github.com/camaraproject/Commonalities/issues/36
    8. Resource path must not contain the base URL Path must not contain the base path.
    Issue on basePath: https://github.com/camaraproject/Commonalities/issues/71
    9. Sensitive data (msisdn/imsi) cannot be a path or query parameter Sensitive data (msisdn/imsi) cannot be a path or query parameter.Note: Needs to list down if we have other sensitive parameters other than MSISDN/IMSI
    New rule: ${\color{green} camara-security-no-secrets-in-path-or-query-parameters}$
    10. Valid methods Valid methods are: GET, PUT, POST, DELETE, PATCH, OPTIONS, HEAD
    New rule: ${\color{green} camara-http-methods }$
    ${\color{red} HEAD\space method\space no\space longer\space supported }$
    11. GET/DELETE methods must not accept a "body" 'GET' and 'DELETE' http methods MUST NOT accept a 'requestBody' attribute
    _New rule: ${\color{green} camara-get-no-request-body }$
    12. Define common error response pattern Following error responses must be define: 400, 401, 403, 404, 405, 406, 429, 500, 501, 502, 503, 504.Note: List needs to be validated as per Camara API documentation - update of Design Guidelines needed
    13. Verb methods cannot be in the Path Resources must not contain the method name get, put, post, delete, patch.
    New rule: ${\color{green} camara-resource-restricted-words}$ - possible update of Design Guidelines
    14. Parameters must have a description All parameters must have a description.
    API Design Guidelines: 11.3 Request Parameters
    New rule: ${\color{green} camara-parameters-description }$
    15. Path version must be major versions only Path must contain the major API version with format /vX.Example: paths: "/device-status/v0/roaming"
    Issue on alpha/beta labels https://github.com/camaraproject/Commonalities/issues/36
    16. GET/DELETE methods must not accept a "body" 'GET' and 'DELETE' http methods MUST NOT accept a 'requestBody' attribute
    ${\color{red} DUPLICATION\space of\space topic\space 10\space ? }$
    17. Mandatory request body with POST, PATCH and PUT methods The requestBody object MAY be present when the operation is used with one of PUT, POST, PATCH HTTP Verbs.
    ${\color{red} DUPLICATION\space of\space topic\space 2\space ? }$
    18. Summary associated with each operation Summary must be defined on each operation, describing with a short summary what the operation does.
    API Design Guidelines: 11.2 Published Routes
    New rule: ${\color{green} camara-operation-summary }$
    19. Include valid response examples For each response code, we must define response payload examples
    possible update of Design Guidelines

    @sachinvodafone
    Copy link
    Collaborator

    sachinvodafone commented Oct 20, 2023

    Hi @rartych , we can ignore duplicate items (16 & 17) that you have highlighted . It seems , some issue in copy/paste . Also I am aligned with point 6 (Event Type) and 10 (HEAD) comments. Apart from this , few others validation can be considered:

      Topic   Description
    20. All schema properties should have a type.
    21. All schema should have properties
    22. Response body schema should use a valid Ref Object The response body schema definition SHOULD use a Reference Object ($ref), MAY be defined inline as well.
    23. requestBody must contain a schema with a valid ref object The schema definition of the request body MUST use a Reference Object ($ref).
    24. Mandatory request header parameter "X-Correlator" All Camara APIs MUST define at least the header parameter that is used to track the inbound requests and outbound responses.

    Example of 20 & 21 is

    **properties**:
            status:
              **type**: integer
              description: HTTP response status code
            code:
              **type**: string
              description: Code given to this error
            message:
              **type**: string
              description: Detailed error description
    

    @rartych rartych changed the title OAS linting ruleset - draft Initial CAMARA OAS linting ruleset Oct 23, 2023
    New rules for topics: 1, 2, 3, 4, 9, 10, 11, 13, 14, 18 added
    Update Linting-rules.md
    OperationId casing clarified - no contradiction with Spectral core rules
    Copy link
    Contributor

    @jlurien jlurien left a comment

    Choose a reason for hiding this comment

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

    Very good starting point. Just some minor comments. the camara-* spectral rules are yet to be developed in a later iteration, right?

    documentation/Linting-rules.md Outdated Show resolved Hide resolved
    |Name| Desc| Recom mended|CAMARA use|Spectral severity | CAMARA severity
    |---|---|---|--|---|--|
    |contact-properties| contact object is full of the most useful properties: `name`, `url`, `and email`|No|No | Warning | |
    |duplicated-entry-in-enum| Each value of an `enum` must be different from one another |Yes | Yes | Warning | |
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    This should be an error

    documentation/Linting-rules.md Outdated Show resolved Hide resolved
    documentation/Linting-rules.md Outdated Show resolved Hide resolved
    documentation/Linting-rules.md Outdated Show resolved Hide resolved
    documentation/Linting-rules.md Outdated Show resolved Hide resolved
    Co-authored-by: Jose Luis Urien <jlurien@gmail.com>
    @rartych
    Copy link
    Collaborator Author

    rartych commented Nov 3, 2023

    @jlurien Thank you for your comments.

    Just some minor comments. the camara-* spectral rules are yet to be developed in a later iteration, right?

    We (within DT) are developing these additional rules (camara-*) and intend to extend this PR with the code.
    Linting-rules.md file is high-level documentation of the ruleset. We also need guidelines for use of linting in Subprojects (as Github Actions or locally). I prefer to have all parts in one PR, but we can split the task into more iterations starting from current definition of rules scopes. Any views on that?

    Another question is testing the ruleset against current API specification in subprojects to minimize false positives.
    Having rules implemented I would need the help of Subprojects maintainers to verify the rules by executing them off-line/locally before the ruleset is released for integration with workflows.

    @jlurien
    Copy link
    Contributor

    jlurien commented Nov 3, 2023

    @jlurien Thank you for your comments.

    Just some minor comments. the camara-* spectral rules are yet to be developed in a later iteration, right?

    We (within DT) are developing these additional rules (camara-*) and intend to extend this PR with the code. Linting-rules.md file is high-level documentation of the ruleset. We also need guidelines for use of linting in Subprojects (as Github Actions or locally). I prefer to have all parts in one PR, but we can split the task into more iterations starting from current definition of rules scopes. Any views on that?

    Another question is testing the ruleset against current API specification in subprojects to minimize false positives. Having rules implemented I would need the help of Subprojects maintainers to verify the rules by executing them off-line/locally before the ruleset is released for integration with workflows.

    My feeling is that this task is too big to keep all work in one PR, so we may iterate, but I let you decide what to include here. I can help testing with Device Location APIs

    Clarifications on Spectral core ruleset
    Table summarizing camara- linting rules
    camara-oas-version severity
    More granular rules for objects descriptions
    Artifacts are proposed in the other PR
    @rartych rartych merged commit b386150 into camaraproject:main Feb 9, 2024
    @rartych rartych deleted the Ruleset-draft branch June 14, 2024 13:37
    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.

    Extend the API guidelines with a concrete ruleset
    6 participants