Skip to content
This repository has been archived by the owner on Sep 2, 2024. It is now read-only.

API design guidelines #72

Closed
wants to merge 2 commits into from
Closed

API design guidelines #72

wants to merge 2 commits into from

Conversation

jordonezlucena
Copy link
Contributor

@jordonezlucena jordonezlucena commented Sep 6, 2022

This is to update the guidelines, with some modifications on filtering, pagination, headers, error codes, etc. A .MD file has been also generated, to simplify review.
The related issue is #32

This is to update the guidelines, with some modifications on filtering, pagination, headers, error codes, etc. A .MD file has been also generated, to simplify review
Copy link
Contributor

@Kevsy Kevsy left a comment

Choose a reason for hiding this comment

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

Agree that User Consent needs further discussion

@jordonezlucena
Copy link
Contributor Author

Agree that User Consent needs further discussion

This document reports on guidelines for API design in CAMARA. The discussion regarding user consent management is captured in PR#68

@shilpa-padgaonkar
Copy link
Contributor

shilpa-padgaonkar commented Sep 20, 2022

Doc LGTM.

Just some points for the versioning section:

  1. We need to differentiate API spec versioning from the underlying service implementation versioning. The doc addresses currently the later?
  2. How will we map the API spec version to the service implementation version (in case both are contributed, as is the case with QoD). If this is not addressed, we will have to maintain on top some sort of compatibility matrix.

For API spec versioning, we could specify what each digit in the version represents:

  • 1st digit: Major version
    This version can constitute major or breaking changes. When this changes, minor and patch versions are reset to 0
    Some examples are listed below:
  1. Removing, renaming, or moving API entities such as:
  2. endpoints
  3. HTTP methods associated with endpoints
  4. operation query, body, or header parameters
  5. schema properties
  6. authorization roles
  7. Making optional parameter mandatory
  8. Changing functionality/behavior
  • 2nd digit: Minor version (non-breaking)
    This change is backwards compatible. When this changes, patch version is reset to 0
    Some examples are listed below:
  1. Adding new error response codes
  • 3rd digit: Patch version (implementation remains unchanged)
    Patch updates are corrections or improvements of descriptions or examples in the API specification. This does not constitute any implementation specific changes.

@RubenBG7
Copy link
Contributor

Hi shilpa, My comments bellow:

  1. The document specifies how the API designs must be versioned and recommends how the implementation of each of the services should be versioned, as well as their deployments.

  2. Regarding to the link between implementation and contract it should be totally unlinked. I mean, the contract specifies that all the API services paths contains the API name and the major version. In the other way you could need deploy lot of times the same version of the APIs due to bug fixing or other... What we have here? lots of service versions (all as deployments you had) but all of them contain the same contract version functionality.

e.g. V1.0.0 of QoD API contract could have 10 diferent deployments and here you have:

  • API functionality is v1.0.0 and its services path contains quality-on-demand/v1/
  • API Service 10th deployment due to fixes in v0.1.8
    - FIrst digit (0) -> Means that we are in the first major version of the service deployment
    - Second digit (1) -> Means that we are in the second level of minor version adding functionality in the second deploy so minor was be upgrade
    - Thrid digit (8) -> Means that we solved some bugs (9) one by one but there no add any functionality.

@jordonezlucena
Copy link
Contributor Author

@shilpa-padgaonkar: let us know whether there are further comments from DT side.

@shilpa-padgaonkar
Copy link
Contributor

@jordonezlucena : After a discussion in last commonalities call with @RubenBG7, he wanted to provide an updated version of some sections of the doc & also check internally about the versioning topic when service implementation is contributed to Camara.

| Name | Description | Type | Pattern | Longitude | Required | Example |
|---|---|---|---|---|---|---|
| `X-Version` | Service version indentificator | String| N/A | | No | |
| `X-Correlator`| Service correlator to make observability| String | UUID (8-4-4-4-12) | Max 36 | Yes | b4333c46-49c0-4f62-80d7-f0ef930f1c46 |
Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree with X-Correlator being mandatory, as API consumers often don't care and just set this to a fixed value. If they don't care, far better that they don't set anything, and let the network do that to ensure a unique value for tracing purposes. Rejecting duplicate X-Correlator values is stateful behaviour, and many API providers won't do this if the request is anyway idempotent.

I can see that there may be cases where the request is not idempotent and the only way to tell between two separate transactions and a duplicated transaction might be such an X-Correlator header, In such a case, the X-Correlator header should indeed be mandatory and duplicates detected and rejected, but this should decided on a case by case basis, and not be mandatory for all API paths.

Incidentally - since 2012, RFC 6648 has deprecated the use of the "X-" prefix for custom headers. It might be worth taking this into account for design guidelines being developed in 2022.

@RubenBG7
Copy link
Contributor

Regarding to the API, Repository Code and Microservices deployment versioning, from TEF we share the next proposal:

API CONTRACT

  • 1st digit: Major version

This version can constitute major or breaking changes. When this changes, minor and patch versions are reset to 0
Some examples are listed below:

  1. Removing, renaming, or moving API entities such as:
  2. endpoints
  3. HTTP methods associated with endpoints
  4. operation query, body, or header parameters
  5. schema properties
  6. authorization roles
  7. Making optional parameter mandatory
  8. Changing functionality/behavior
  • 2nd digit: Minor version (non-breaking)

This change is backwards compatible. When this changes, patch version is reset to 0
Some examples are listed below:

  1. Adding new error response codes
  • 3rd digit: Patch version (implementation remains unchanged)

Patch updates are corrections or improvements of descriptions or examples in the API specification. This does not constitute any implementation specific changes.

SHARED CODE ON REPOSITORIES

MAJOR - MAJOR OF API CONTRACT
MINOR - MINOR OF API CONTRACT
PATCH - NEW UPDATES / CONTRIBUTIONS OF SHARED CODE

MICROSERVICE DEPLOYMENTS (NOT MANDATORY BUT RECOMMENDED)

MAJOR - MAJOR OF API CONTRACT
MINOR - MINOR OF API CONTRACT
PATCH - NEW MICROSERVICES DEPLOYMENT

@shilpa-padgaonkar
Copy link
Contributor

shilpa-padgaonkar commented Oct 14, 2022

@RubenBG7 : This is an exact match to us.

@RubenBG7
Copy link
Contributor

@shilpa-padgaonkar and @eric-murray , changes related to versioning and correlator header applied in API Guidelines md and pull request created,

@eric-murray
Copy link
Contributor

@shilpa-padgaonkar and @eric-murray , changes related to versioning and correlator header applied in API Guidelines md and pull request created,

Is this PR therefore to be closed without merging? There are 2 PRs open on API guidelines.

@shilpa-padgaonkar
Copy link
Contributor

@RubenBG7 : Should this be marked as duplicate and simply closed?

@RubenBG7
Copy link
Contributor

RubenBG7 commented Oct 19, 2022

Simply Closed. Thanks Shilpa

@shilpa-padgaonkar
Copy link
Contributor

Duplicate of #96

@shilpa-padgaonkar shilpa-padgaonkar marked this as a duplicate of #96 Oct 19, 2022
@sachinvodafone
Copy link
Contributor

Under section 8.3 , it seems , we don’t have option for strict greater or less than option. As per TM forum , I believe , following would be easy to follow:

.gt > (URL Encoded equivalent : %3E)
.gte >= (URL Encoded equivalent : %3E%3D)
.lt < (URL Encoded equivalent :%3C)
.lte (URL Encoded equivalent : %3C%3D)
.eq (URL Encoded equivalent : %3D%3D)

@sachinvodafone
Copy link
Contributor

Hi Team, Under section 8.1 (Pagination), I believe, X-Total-Count header should be optional unless they're strictly needed for pagination requirement. It seems, browsers only access some response headers by default when it comes to CORS request, the default response headers are the ones listed below:

  1. Cache-Control
  2. Content-Language
  3. Content-Type
  4. Expires
  5. Last-Modified
  6. Pragma

So we need to set the Access-Control-Expose-Headers header on our server so it will be allowed to be read on your client app for other headers.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants