API versioning using accept header #4239

Marishka17 · 2022-01-25T20:17:58Z

Motivation and context

This PR contains a new API versioning scheme using AcceptHeaderVersioning
I've tried to implement an approach like API versioning for HTTP REST interfaces (best practices) (application/vnd.cvat.v1.0+json) but in this case, I couldn't fix unavailable drf-spectacular documentation. I guess It may be not supported in the current implementation

How has this been tested?

Checklist

I submit my changes into the develop branch
I have added a description of my changes into CHANGELOG file
I have updated the documentation accordingly
I have added tests to cover my changes
I have linked related issues (read github docs)
I have increased versions of npm packages if it is necessary (cvat-canvas,
cvat-core, cvat-data and cvat-ui)

License

I submit my code changes under the same MIT License that covers the project.
Feel free to contact the maintainers if that's a concern.
I have updated the license header for each file (see an example below)

# Copyright (C) 2022 Intel Corporation
#
# SPDX-License-Identifier: MIT

…eter idea

nmanovic · 2022-02-01T18:31:52Z

.github/workflows/schedule.yml

        run: |
          docker-compose -f docker-compose.yml -f docker-compose.dev.yml -f ./tests/docker-compose.email.yml -f tests/docker-compose.file_share.yml -f components/serverless/docker-compose.serverless.yml up -d --build
-          /bin/bash -c 'while [[ "$(curl -s -o /dev/null -w ''%{http_code}'' ${API_ABOUT_PAGE})" != "401" ]]; do sleep 5; done'
+          /bin/bash -c 'while [[ $(curl -s -o /dev/null -H "Accept: application/vnd.cvat+json; version=1.0" -w "%{http_code}" ${API_ABOUT_PAGE}) != "401" ]]; do sleep 5; done'


@Marishka17 , what do you think if by default we will use the latest version of API? Thus if we don't specify accept header, DRF will use the latest one. I know that the link https://github.com/interagent/http-api-design/blob/master/en/foundations/require-versioning-in-the-accepts-header.md says that it is a bad practice. But GitHub uses the strategy: https://docs.github.com/en/rest/overview/media-types

@nmanovic , Also rest_framework documentation doesn't say that this is a bad practice, let's use the default version. In this case, I guess we can miss accept header in client/tests. I'll update it.

CHANGELOG.md

nmanovic · 2022-02-01T18:38:19Z

cvat/apps/dataset_manager/tests/test_formats.py

@@ -36,24 +37,30 @@ def generate_image_file(filename, size=(100, 100)):
    return f

 class ForceLogin:
-    def __init__(self, user, client):
+    def __init__(self, user, client, version=settings.BACKEND_VERSIONS.V1_0):


@Marishka17 , One more problem if we don't have a default version. I don't think that it is good idea to have BACKEND_VERSIONS.V1_0 and ACCEPT_HEADER_TEMPLATE in settings.

nmanovic · 2022-02-01T18:44:01Z

cvat/apps/engine/versioning.py

+    Accept: application/vnd.cvat+json; version=1.0
+    """
+
+    def determine_version(self, request, *args, **kwargs):


@Marishka17 , Do we need the method if we have a default version?

nmanovic · 2022-02-01T18:47:24Z

cvat/settings/base.py

    # Need to add 'api-docs' here as a workaround for include_docs_urls.
-    'ALLOWED_VERSIONS': ('v1', 'api-docs'),
+    'ALLOWED_VERSIONS': (*BACKEND_VERSIONS.list(), 'api-docs', 'downloading'),


Let's try to remove api-docs and downloading from the list.

nmanovic · 2022-02-01T18:49:49Z

site/content/en/docs/contributing/rest-api-design.md

@@ -36,7 +36,7 @@ Common scheme for our REST API is `<VERB> [namespace] <objects> <id> <action>`.
 - Allow filtering, sorting, and pagination
 - Maintain good security practices
 - Cache data to improve performance
- Versioning our APIs (e.g. `/api/v1`, `/api/v2`). It should be done when you
+- Versioning our APIs (e.g. `/api`, `/api/v2`). It should be done when you


@Marishka17 , need to update the statement. It looks wrong now (e.g., /api/v2)

nmanovic · 2022-02-01T18:54:45Z

tests/cypress/support/commands_organizations.js

        headers: {
            Authorization: `Token ${authKey}`,
+            Accept: acceptHeader,


@Marishka17 , @bsekachev , should UI work with the latest version (=default) of REST API always? Thus we can skip acceptHeader for tests and UI.

nmanovic · 2022-02-01T18:59:52Z

tests/rest_api/test_0001_users_api.py

@@ -12,12 +12,14 @@

 def test_non_admin_cannot_see_others():
    for username in ['dummy1', 'worker1', 'user1', 'business1']:
-        response = requests.get(config.get_api_url('users'), auth=(username, config.USER_PASS))
+        response = requests.get(config.get_api_url('users'), auth=(username, config.USER_PASS),


@Marishka17 , tests should test all versions. Thus if we have a default version, right now it will be enough. Let's discuss. Really want to find the right approach.

nmanovic · 2022-02-01T19:01:33Z

utils/cli/cli.py

@@ -34,6 +34,8 @@ def main():
    with requests.Session() as session:
        api = CVAT_API_V1('%s:%s' % (args.server_host, args.server_port), args.https)
        cli = CLI(session, api, args.auth)
+        #TODO add version indication
+        session.headers.update({'Accept': 'application/vnd.cvat+json; version=1.0'})


Again, probably CLI should work with the latest (default version) of API.

Marishka17 · 2022-02-03T07:53:58Z

@nmanovic , I've updated the API version to 2.0 as you said yesterday (I didn't do it before because I believed that we should have done tasks with server code redesigning and separating it to standalone applications).
Also, I've added a test for checking the version for server/about endpoint.
I've found several ways to version codebase (simply extend serializer, an approach like migrations, and others) and I guess we should discuss this moment with the team to choose the best approach for us. So It should be another task.

nmanovic · 2022-02-03T19:46:09Z

cvat/settings/base.py

-    # Need to add 'api-docs' here as a workaround for include_docs_urls.
-    'ALLOWED_VERSIONS': ('v1', 'api-docs'),
+        'rest_framework.versioning.AcceptHeaderVersioning',
+    'ALLOWED_VERSIONS': ('1.0', '2.0'),


@Marishka17 , do we support 1.0?

nmanovic

LGTM

Marishka17 · 2022-02-11T18:28:35Z

@nmanovic, Should we also register vnd MIME type?

nmanovic · 2022-02-14T04:18:11Z

@Marishka17 , we should. Need to contact OSPDT.

init

2b433e8

Marishka17 requested a review from nmanovic January 25, 2022 20:17

Marishka17 requested review from azhavoro, bsekachev and zhiltsov-max as code owners January 25, 2022 20:17

Marishka17 added 4 commits January 26, 2022 12:24

Update tests & cli

969bbe7

Fix esLint issues

e7e55ad

use custome media type instead of param

fe56373

fix displaying documentation && return to version definition as param…

51244df

…eter idea

Marishka17 changed the title ~~[WIP] API versioning~~ API versioning using accept header Jan 28, 2022

Marishka17 added 4 commits January 28, 2022 17:23

fix workflow

9ec3eb6

Fix REST API tests

08c677e

Update workflow

2451855

Update CHANGELOG

112e86f

Marishka17 force-pushed the mk/versioning branch 5 times, most recently from b33bf82 to 06a0ee3 Compare January 31, 2022 14:07

Update workflows

7b841d3

Marishka17 force-pushed the mk/versioning branch from 06a0ee3 to 7b841d3 Compare January 31, 2022 15:39

Marishka17 and others added 5 commits January 31, 2022 19:12

Update cypress tests

aa64bf8

Fix eslint issues

a2697b3

Merge branch 'develop' into mk/versioning

0967701

Merge branch 'develop' into mk/versioning

26fa005

Dummy fix for downloading resources

4a31881

nmanovic reviewed Feb 1, 2022

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

nmanovic reviewed Feb 1, 2022

View reviewed changes

Marishka17 changed the title ~~API versioning using accept header~~ [WIP]API versioning using accept header Feb 2, 2022

Marishka17 added 5 commits February 2, 2022 11:46

Use default version

39564ea

Update rest api design

abe510a

Remove extra

6c3d537

Resolve conflicts

67e5752

Remove unused imports && fix headers

904b21f

Marishka17 changed the title ~~[WIP]API versioning using accept header~~ API versioning using accept header Feb 2, 2022

Marishka17 added 3 commits February 2, 2022 14:29

Update assets

9ccff04

Add test && API version 2.0

3c8c61e

Update tests & cli

fa88418

nmanovic reviewed Feb 3, 2022

View reviewed changes

nmanovic approved these changes Feb 3, 2022

View reviewed changes

nmanovic merged commit 83126c7 into develop Feb 3, 2022

nmanovic deleted the mk/versioning branch February 3, 2022 19:50

azhavoro mentioned this pull request Feb 16, 2022

Added simple test for analytics availability #4308

Merged

3 tasks

nmanovic mentioned this pull request Mar 4, 2022

Release v2.0.0 #4422

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API versioning using accept header #4239

API versioning using accept header #4239

Marishka17 commented Jan 25, 2022 •

edited

nmanovic Feb 1, 2022

Marishka17 Feb 2, 2022

nmanovic Feb 1, 2022

nmanovic Feb 1, 2022

nmanovic Feb 1, 2022

nmanovic Feb 1, 2022

nmanovic Feb 1, 2022

nmanovic Feb 1, 2022

nmanovic Feb 1, 2022

Marishka17 commented Feb 3, 2022

nmanovic Feb 3, 2022

nmanovic left a comment

Marishka17 commented Feb 11, 2022

nmanovic commented Feb 14, 2022

API versioning using accept header #4239

API versioning using accept header #4239

Conversation

Marishka17 commented Jan 25, 2022 • edited

Motivation and context

How has this been tested?

Checklist

License

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marishka17 commented Feb 3, 2022

Choose a reason for hiding this comment

nmanovic left a comment

Choose a reason for hiding this comment

Marishka17 commented Feb 11, 2022

nmanovic commented Feb 14, 2022

Marishka17 commented Jan 25, 2022 •

edited