Enhancement: clients can use REST url for all methods #219

davidalexandru7013 · 2024-08-13T08:28:43Z

Description of changes

In this modification, I created the opportunity for users to decide between using v1 api or rest api route in the SnykClient. Previously, only the get method let user to use the rest api. Patch method was created for use cases in which the verb PATCH is necessary, thing that didn't exist previously.

Technical details

added patch method in the SnykClient class.
updated old tests and added new test cases for the new modifications.
redirect delete project from v1 to rest in the client.
updated rest api version, because some endpoints were not available in the old version.

Context

These changes were made to enable users use the rest api, because v1 is approaching its end of life. It provides more flexibility to the developers who wants to interact with different instances of Snyk APIs.

How to test

This modification should not impact the existing functionality of the library. Everything should work as intended.

Example Usage

Update an existing project

client = SnykClient(token=api_key)
client.patch(
    f"orgs/org_id/projects/project_id",
    body={
        "data": {
            "attributes": {
                "business_criticality": ["critical", "medium", "low"],
                "tags": [
                    {"key": "keytest1", "value": "valuetest1"},
                    {"key": "keytest2", "value": "valuetest2"},
                ],
                "environment": ["backend", "frontend"],
                "lifecycle": ["development"],
            },
            "id": "project_id",
            "relationships": {},
            "type": "project",
        }
    },
)

iulia-ionce · 2024-08-13T09:31:50Z

snyk/client.py

-            fkwargs={"json": body, "headers": {**self.api_post_headers, **headers}},
+            fkwargs={
+                "json": body,
+                "headers": {**self.api_post_headers, **headers},


feels a bit confusing to user the api_post_headers in the put. Maybe use the api_headers or create a similar api_put_headers?

This line was not changed from the initial version, it is marked as modified, because I've added params and the linter splits them on multiple lines.

iulia-ionce · 2024-08-13T09:33:26Z

snyk/client.py

+        body: Dict[str, Any],
+        headers: Dict[str, str] = {},
+        params: Dict[str, Any] = {},
+    ) -> requests.Response:


I'm not sure if we should keep consistency here with a rest parameter or not. I know it would be useless, and need to be set to true. It's just triggering my OCD having this lack of uniformity of methods 🙈

According to the documentation(https://snyk.docs.apiary.io/#reference) there is no endpoint which makes use of patch verb in V1.

iulia-ionce · 2024-08-13T09:56:12Z

snyk/test_client.py

@@ -329,3 +327,140 @@ def test_rest_limit_deduplication(self, requests_mock, rest_client):
        )
        params = {"limit": 10}
        rest_client.get(f"orgs/{REST_ORG}/projects?limit=100", params)
+
+    def test_patch_update_project_should_return_new_project(


maybe a typo? patch should not return a "new" project

iulia-ionce · 2024-08-13T09:58:25Z

snyk/test_client.py

+        assert response.status_code == 200
+        assert response_data == project
+
+    def test_token_added_to_patch_headers(self, client):


sorry, don't know enough python 😅 how does this test work? What does it test?

It makes sure that there is a value in Authorization header and nobody deletes the line which add the value, by mistake.

client is a fixture. See the function client() in the top of the class.
When you add an argument to the test it runs the function and uses the returned value as parameter.

iulia-ionce

Nice job! Just left a few comments (some just asking for clarification 😄 )

pablo-snyk · 2024-08-19T12:47:31Z

snyk/client.py

@@ -40,13 +42,18 @@ def __init__(
            "Authorization": "token %s" % self.api_token,
            "User-Agent": user_agent,
        }
-        self.api_post_headers = self.api_headers
+        self.api_post_headers = copy.deepcopy(self.api_headers)


Suggested change

self.api_post_headers = copy.deepcopy(self.api_headers)

self.api_post_headers = dict(self.api_headers)

Why do we need to copy them?

Since both api post headers and patch post headers have different content-type, I wanted a way to avoid side-effects(since simple assigning creates a shallow copy and dictionaries are mutable in python) when changing content-type. The solution given by you might be better rather than creating a deep-copy.

So my suggestion is to set those headers explicitly in the specific function.
Otherwise it's confusing.

This might introduce duplicate code, since both V1 and REST API are using the same headers.

Again, it's a suggestion only. In my opinion it is better to duplicate code and make them explicit rather than start looking at who modified what header. Moreover, if you do that you will be able to get rid of the api_post_headers property.

pablo-snyk · 2024-08-19T12:48:19Z

snyk/client.py

        self.api_post_headers["Content-Type"] = "application/json"
+        self.api_patch_headers = copy.deepcopy(self.api_headers)


Suggested change

self.api_patch_headers = copy.deepcopy(self.api_headers)

self.api_patch_headers = dict(self.api_headers)

Same here. Why do we need to duplicate them?

The same reason as stated above.

pablo-snyk · 2024-08-19T12:51:49Z

snyk/client.py

+    def post(
+        self,
+        path: str,
+        body: Any,


If this is a dict, so can use Dict. If it's either a dict or a str you can do something like Union[Dict, str].

pablo-snyk · 2024-08-19T12:53:32Z

snyk/client.py

+        self,
+        path: str,
+        body: Any,
+        headers: dict = {},


What version of Python do you use? The type dict is allowed only from 3.9. I saw in the pyproject.toml file that the allowed versions of Python are from version 3.7 and up. If this is still the case I suggest using Dict instead.

This was previously in the project, I can change it, but I need to make sure every instance gets changed.

I'm not suggesting to upgrade. But if we still want to support Python 3.7 we need to change line 96 to be something like:

headers: Dict = {}. # note the capital D.

pablo-snyk · 2024-08-19T12:54:04Z

snyk/client.py

        logger.debug(f"POST: {url}")

+        if use_rest and "version" not in params:
+            params["version"] = self.version if self.version else "2024-06-21"


Suggested change

params["version"] = self.version if self.version else "2024-06-21"

params["version"] = self.version or "2024-06-21"

Also, any chance we define the default version in a constant?

pablo-snyk · 2024-08-19T12:56:43Z

snyk/client.py

+        logger.debug(f"PATCH: {url}")
+
+        if "version" not in params:
+            params["version"] = self.version if self.version else "2024-06-21"


Suggested change

params["version"] = self.version if self.version else "2024-06-21"

params["version"] = self.version or "2024-06-21"

Any chance to define the default version in a constant?

pablo-snyk

The last commit to package retry's repo was in 2016.
I suggest replace it.

davidalexandru7013 added 18 commits August 9, 2024 15:26

feat: added project v1 to rest conversion class

a5803bd

feat: added method to check if a path is project v1

5cee33b

fix: updated delete project tests to mock rest route

39d9486

chore: black formatting of client, managers and models files

62e3c3b

feat: moved get project by id from v1 to rest

0ee912d

fix: replace old route with new route for projects in tests

8fc8a36

feat: patch method in client

cfe46fe

chore: removed debug from examples

eeddafe

chore: removed ununsed class

956394e

chore: replaced old api version with new one

659827e

feat: added patch update project test

5c21f1d

feat: tests for patch update project

3b76531

chore: added get project by id and tests back

f0a05e2

chore: use rest boolean for clients and tests

d6db244

chore: send query params

a04902a

feat: tests to avoid side-effects for api headers

2bc763f

feat: clients can use rest path for delete and tests

90f67bc

chore: removed example file

a823df4

iulia-ionce reviewed Aug 13, 2024

View reviewed changes

iulia-ionce approved these changes Aug 13, 2024

View reviewed changes

chore: rename test from new project to updated project

b849478

pablo-snyk reviewed Aug 19, 2024

View reviewed changes

davidalexandru7013 added 4 commits August 20, 2024 09:44

chore: replace dict with Dict and extract latest version in a variable

657d4ec

chore: replace type from any to dict

45d2ec8

fix: added rest content type for post requests when calling rest route

0998780

fix: added rest content type for put method and tests

d4fe020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancement: clients can use REST url for all methods #219

Enhancement: clients can use REST url for all methods #219

davidalexandru7013 commented Aug 13, 2024 •

edited

Loading

iulia-ionce Aug 13, 2024

davidalexandru7013 Aug 13, 2024

iulia-ionce Aug 13, 2024

davidalexandru7013 Aug 13, 2024

iulia-ionce Aug 13, 2024

iulia-ionce Aug 13, 2024 •

edited

Loading

davidalexandru7013 Aug 13, 2024

pablo-snyk Aug 19, 2024

iulia-ionce left a comment

pablo-snyk Aug 19, 2024

davidalexandru7013 Aug 20, 2024

pablo-snyk Aug 20, 2024

davidalexandru7013 Aug 26, 2024 •

edited

Loading

pablo-snyk Aug 26, 2024

pablo-snyk Aug 19, 2024

davidalexandru7013 Aug 20, 2024

pablo-snyk Aug 19, 2024

pablo-snyk Aug 19, 2024

davidalexandru7013 Aug 19, 2024

pablo-snyk Aug 19, 2024

pablo-snyk Aug 19, 2024

pablo-snyk Aug 19, 2024

pablo-snyk Aug 19, 2024

pablo-snyk left a comment

	self.api_post_headers = copy.deepcopy(self.api_headers)
	self.api_post_headers = dict(self.api_headers)

		self.api_post_headers["Content-Type"] = "application/json"
		self.api_patch_headers = copy.deepcopy(self.api_headers)

	self.api_patch_headers = copy.deepcopy(self.api_headers)
	self.api_patch_headers = dict(self.api_headers)

	params["version"] = self.version if self.version else "2024-06-21"
	params["version"] = self.version or "2024-06-21"

Enhancement: clients can use REST url for all methods #219

Are you sure you want to change the base?

Enhancement: clients can use REST url for all methods #219

Conversation

davidalexandru7013 commented Aug 13, 2024 • edited Loading

Description of changes

Technical details

Context

How to test

Example Usage

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

iulia-ionce Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iulia-ionce left a 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

davidalexandru7013 Aug 26, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pablo-snyk left a comment

Choose a reason for hiding this comment

davidalexandru7013 commented Aug 13, 2024 •

edited

Loading

iulia-ionce Aug 13, 2024 •

edited

Loading

davidalexandru7013 Aug 26, 2024 •

edited

Loading