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

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

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
a5803bd
feat: added project v1 to rest conversion class
davidalexandru7013 Aug 9, 2024
5cee33b
feat: added method to check if a path is project v1
davidalexandru7013 Aug 9, 2024
39d9486
fix: updated delete project tests to mock rest route
davidalexandru7013 Aug 11, 2024
62e3c3b
chore: black formatting of client, managers and models files
davidalexandru7013 Aug 11, 2024
0ee912d
feat: moved get project by id from v1 to rest
davidalexandru7013 Aug 12, 2024
8fc8a36
fix: replace old route with new route for projects in tests
davidalexandru7013 Aug 12, 2024
cfe46fe
feat: patch method in client
davidalexandru7013 Aug 12, 2024
eeddafe
chore: removed debug from examples
davidalexandru7013 Aug 12, 2024
956394e
chore: removed ununsed class
davidalexandru7013 Aug 12, 2024
659827e
chore: replaced old api version with new one
davidalexandru7013 Aug 12, 2024
5c21f1d
feat: added patch update project test
davidalexandru7013 Aug 12, 2024
3b76531
feat: tests for patch update project
davidalexandru7013 Aug 12, 2024
f0a05e2
chore: added get project by id and tests back
davidalexandru7013 Aug 12, 2024
d6db244
chore: use rest boolean for clients and tests
davidalexandru7013 Aug 12, 2024
a04902a
chore: send query params
davidalexandru7013 Aug 12, 2024
2bc763f
feat: tests to avoid side-effects for api headers
davidalexandru7013 Aug 13, 2024
90f67bc
feat: clients can use rest path for delete and tests
davidalexandru7013 Aug 13, 2024
a823df4
chore: removed example file
davidalexandru7013 Aug 13, 2024
b849478
chore: rename test from new project to updated project
davidalexandru7013 Aug 13, 2024
657d4ec
chore: replace dict with Dict and extract latest version in a variable
davidalexandru7013 Aug 20, 2024
45d2ec8
chore: replace type from any to dict
davidalexandru7013 Aug 20, 2024
0998780
fix: added rest content type for post requests when calling rest route
davidalexandru7013 Aug 20, 2024
d4fe020
fix: added rest content type for put method and tests
davidalexandru7013 Aug 20, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/api-demo-1-list-projects.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,4 @@ def parse_command_line_args():
print(" Issues Found:")
print(" High : %s" % proj.issueCountsBySeverity.high)
print(" Medium: %s" % proj.issueCountsBySeverity.medium)
print(" Low : %s" % proj.issueCountsBySeverity.low)
print(" Low : %s" % proj.issueCountsBySeverity.low)
116 changes: 105 additions & 11 deletions snyk/client.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import copy
import logging
import re
import urllib.parse
from typing import Any, List, Optional
from typing import Any, Dict, List, Optional, Pattern
from urllib.parse import parse_qs, urlparse

import requests
Expand Down Expand Up @@ -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)

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

Copy link
Author

@davidalexandru7013 davidalexandru7013 Aug 26, 2024

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

The same reason as stated above.

self.api_patch_headers["Content-Type"] = "application/vnd.api+json"
self.tries = tries
self.backoff = backoff
self.delay = delay
self.verify = verify
self.version = version
self.__uuid_pattern = (
r"[0-9a-f]{8}-[0-9a-f]{4}-[4][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}"
)

# Ensure we don't have a trailing /
if self.api_url[-1] == "/":
Expand Down Expand Up @@ -82,14 +89,28 @@ def request(
raise SnykHTTPError(resp)
return resp

def post(self, path: str, body: Any, headers: dict = {}) -> requests.Response:
url = f"{self.api_url}/{path}"
def post(
self,
path: str,
body: Any,

Choose a reason for hiding this comment

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

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].

headers: dict = {},

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.

params: Dict[str, Any] = {},
use_rest: bool = False,
) -> requests.Response:
url = f"{self.rest_api_url if use_rest else self.api_url}/{path}"
logger.debug(f"POST: {url}")

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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


resp = retry_call(
self.request,
fargs=[requests.post, url],
fkwargs={"json": body, "headers": {**self.api_post_headers, **headers}},
fkwargs={
"json": body,
"headers": {**self.api_post_headers, **headers},
"params": params,
},
tries=self.tries,
delay=self.delay,
backoff=self.backoff,
Expand All @@ -103,14 +124,64 @@ def post(self, path: str, body: Any, headers: dict = {}) -> requests.Response:

return resp

def put(self, path: str, body: Any, headers: dict = {}) -> requests.Response:
url = "%s/%s" % (self.api_url, path)
def patch(
self,
path: str,
body: Dict[str, Any],
headers: Dict[str, str] = {},
params: Dict[str, Any] = {},
) -> requests.Response:

Choose a reason for hiding this comment

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

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 🙈

Copy link
Author

Choose a reason for hiding this comment

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

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

url = f"{self.rest_api_url}/{path}"
logger.debug(f"PATCH: {url}")

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

Choose a reason for hiding this comment

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

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?


resp = retry_call(
self.request,
fargs=[requests.patch, url],
fkwargs={
"json": body,
"headers": {**self.api_patch_headers, **headers},
"params": params,
},
tries=self.tries,
delay=self.delay,
backoff=self.backoff,
logger=logger,
)

if not resp.ok:
logger.error(resp.text)
raise SnykHTTPError(resp)

return resp

def put(
self,
path: str,
body: Any,
headers: dict = {},
params: Dict[str, Any] = {},
use_rest: bool = False,
) -> requests.Response:
url = "%s/%s" % (
self.rest_api_url if use_rest else self.api_url,
path,
)
logger.debug("PUT: %s" % url)

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

resp = retry_call(
self.request,
fargs=[requests.put, url],
fkwargs={"json": body, "headers": {**self.api_post_headers, **headers}},
fkwargs={
"json": body,
"headers": {**self.api_post_headers, **headers},

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

"params": params,
},
tries=self.tries,
delay=self.delay,
backoff=self.backoff,
Expand Down Expand Up @@ -196,14 +267,26 @@ def get(

return resp

def delete(self, path: str) -> requests.Response:
url = f"{self.api_url}/{path}"
def delete(self, path: str, use_rest: bool = False) -> requests.Response:
is_v1_project_path: bool = self.__is_v1_project_path(path)
if is_v1_project_path:
ids = re.findall(rf"{self.__uuid_pattern}", path)
path = f"orgs/{ids[0]}/projects/{ids[1]}"
url = f"{self.rest_api_url}/{path}"
use_rest = True
else:
url = f"{self.rest_api_url if use_rest else self.api_url}/{path}"

params = {}
if use_rest:
params["version"] = self.version if self.version else "2024-06-21"

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

resp = retry_call(
self.request,
fargs=[requests.delete, url],
fkwargs={"headers": self.api_headers},
fkwargs={"headers": self.api_headers, "params": params},
tries=self.tries,
delay=self.delay,
backoff=self.backoff,
Expand Down Expand Up @@ -293,3 +376,14 @@ def groups(self):
# https://snyk.docs.apiary.io/#reference/reporting-api/issues/get-list-of-issues
def issues(self):
raise SnykNotImplementedError # pragma: no cover

def __is_v1_project_path(self, path: str) -> bool:
v1_paths: List[Pattern[str]] = [
re.compile(f"^org/{self.__uuid_pattern}/project/{self.__uuid_pattern}$")
]

for v1_path in v1_paths:
if re.match(v1_path, path):
return True

return False
1 change: 0 additions & 1 deletion snyk/managers.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import abc
import json
from typing import Any, Dict, List

from deprecation import deprecated # type: ignore
Expand Down
3 changes: 1 addition & 2 deletions snyk/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
from dataclasses import InitVar, dataclass, field
from typing import Any, Dict, List, Optional, Union

import requests
from deprecation import deprecated # type: ignore
from mashumaro.mixins.json import DataClassJSONMixin # type: ignore

Expand Down Expand Up @@ -245,7 +244,7 @@ def import_project(self, url, files: Optional[List[str]] = None) -> bool:
# https://snyk.docs.apiary.io/#reference/users/user-organisation-notification-settings/modify-org-notification-settings
# https://snyk.docs.apiary.io/#reference/users/user-organisation-notification-settings/get-org-notification-settings
def notification_settings(self):
raise SnykNotImplemented # pragma: no cover
raise SnykNotImplementedError # pragma: no cover

# https://snyk.docs.apiary.io/#reference/organisations/the-snyk-organisation-for-a-request/invite-users
def invite(self, email: str, admin: bool = False) -> bool:
Expand Down
143 changes: 139 additions & 4 deletions snyk/test_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@

REST_ORG = "39ddc762-b1b9-41ce-ab42-defbe4575bd6"
REST_URL = "https://api.snyk.io/rest"
REST_VERSION = "2022-02-16~experimental"
REST_VERSION = "2024-06-21"

V3_ORG = "39ddc762-b1b9-41ce-ab42-defbe4575bd6"
V3_URL = "https://api.snyk.io/v3"
Expand Down Expand Up @@ -207,9 +207,7 @@ def test_non_existent_project(self, requests_mock, client, organizations, projec

@pytest.fixture
def rest_client(self):
return SnykClient(
"token", version="2022-02-16~experimental", url="https://api.snyk.io/rest"
)
return SnykClient("token", version="2024-06-21", url="https://api.snyk.io/rest")

@pytest.fixture
def v3_client(self):
Expand Down Expand Up @@ -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(

Choose a reason for hiding this comment

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

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

self, requests_mock, rest_client, projects
):
project = projects["data"][0]
matcher = re.compile(
f"orgs/{REST_ORG}/projects/{project['id']}\\?([^&=]+=[^&=]+&?)+$"
)
body = {
"data": {
"attributes": {
"business_criticality": ["critical"],
"environment": ["backend", "internal"],
"lifecycle": ["development"],
"tags": [{"key": "key-test", "value": "value-test"}],
}
}
}
project["attributes"] = {**project["attributes"], **body["data"]["attributes"]}
requests_mock.patch(matcher, json=project, status_code=200)

response = rest_client.patch(
f"orgs/{REST_ORG}/projects/{project['id']}",
body=project,
params={"expand": "target"},
)

response_data = response.json()
assert response.status_code == 200
assert response_data == project

def test_token_added_to_patch_headers(self, client):
Copy link

@iulia-ionce iulia-ionce Aug 13, 2024

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.

assert client.api_patch_headers["Authorization"] == "token token"

def test_patch_headers_use_correct_mimetype(self, client):
assert client.api_patch_headers["Content-Type"] == "application/vnd.api+json"

def test_patch_has_version_in_query_params(self, client, requests_mock):
matcher = re.compile("\\?version=2[0-9]{3}-[0-9]{2}-[0-9]{2}$")
requests_mock.patch(matcher, json={}, status_code=200)
client.patch(
f"{REST_URL}/orgs/{REST_ORG}/projects/f9fec29a-d288-40d9-a019-cedf825e6efb",
body={},
)

assert requests_mock.call_count == 1

def test_patch_update_project_when_invalid_should_throw_exception(
self, requests_mock, rest_client
):
matcher = re.compile(
"projects/f9fec29a-d288-40d9-a019-cedf825e6efb\\?version=2[0-9]{3}-[0-9]{2}-[0-9]{2}$"
)
body = {"attributes": {"environment": ["backend"]}}

requests_mock.patch(matcher, json=body, status_code=400)
with pytest.raises(SnykError):
rest_client.patch(
f"orgs/{REST_ORG}/projects/f9fec29a-d288-40d9-a019-cedf825e6efb",
body=body,
)

assert requests_mock.call_count == 1

def test_post_request_rest_api_when_specified(self, requests_mock, client):
matcher = re.compile(
f"{REST_URL}/orgs/{REST_ORG}/projects/f9fec29a-d288-40d9-a019-cedf825e6efb\\?version={REST_VERSION}$"
)
requests_mock.post(matcher, json={}, status_code=200)
params = {"version": REST_VERSION}
client.post(
f"{REST_URL}/orgs/{REST_ORG}/projects/f9fec29a-d288-40d9-a019-cedf825e6efb",
body={},
params=params,
use_rest=True,
)

assert requests_mock.call_count == 1

def test_put_request_rest_api_when_specified(self, requests_mock, client):
matcher = re.compile(
f"{REST_URL}/orgs/{REST_ORG}/projects/f9fec29a-d288-40d9-a019-cedf825e6efb\\?version={REST_VERSION}$"
)
requests_mock.put(matcher, json={}, status_code=200)
params = {"version": REST_VERSION}
client.put(
f"{REST_URL}/orgs/{REST_ORG}/projects/f9fec29a-d288-40d9-a019-cedf825e6efb",
body={},
params=params,
use_rest=True,
)

assert requests_mock.call_count == 1

def test_put_request_v1_api_when_specified(self, requests_mock, client):
matcher = re.compile(
f"^https://api.snyk.io/v1/org/{REST_ORG}/project/f9fec29a-d288-40d9-a019-cedf825e6efb"
)
requests_mock.put(matcher, json={}, status_code=200)
client.put(
f"org/{REST_ORG}/project/f9fec29a-d288-40d9-a019-cedf825e6efb",
body={},
use_rest=False,
)

assert requests_mock.call_count == 1

def test_delete_use_rest_when_specified(self, requests_mock, client):
matcher = re.compile(
"^%s/orgs/%s\\?version=2[0-9]{3}-[0-9]{2}-[0-9]{2}$" % (REST_URL, REST_ORG)
)
requests_mock.delete(matcher, json={}, status_code=200)

client.delete(f"orgs/{REST_ORG}", use_rest=True)
assert requests_mock.call_count == 1

def test_delete_use_v1_when_specified(self, requests_mock, client):
matcher = re.compile("^%s/orgs/%s" % ("https://api.snyk.io/v1", REST_ORG))
requests_mock.delete(matcher, json={}, status_code=200)

client.delete(f"orgs/{REST_ORG}")
assert requests_mock.call_count == 1

def test_delete_redirects_to_rest_api_for_delete_project(
self, client, requests_mock, projects
):
project = projects["data"][0]
matcher = re.compile(
"orgs/%s/projects/%s\\?version=2[0-9]{3}-[0-9]{2}-[0-9]{2}$"
% (REST_ORG, project["id"])
)

requests_mock.delete(matcher, json={}, status_code=200)

client.delete(f"org/{REST_ORG}/project/{project['id']}")

assert requests_mock.call_count == 1
2 changes: 1 addition & 1 deletion snyk/test_data/rest_targets_page1.json
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,6 @@
}
],
"links": {
"next": "/orgs/39ddc762-b1b9-41ce-ab42-defbe4575bd6/targets?limit=10&version=2022-02-16~experimental&excludeEmpty=true&starting_after=v1.eyJpZCI6IjMyODE4ODAifQ%3D%3D"
"next": "/orgs/39ddc762-b1b9-41ce-ab42-defbe4575bd6/targets?limit=10&version=2024-06-21&excludeEmpty=true&starting_after=v1.eyJpZCI6IjMyODE4ODAifQ%3D%3D"
}
}
4 changes: 2 additions & 2 deletions snyk/test_data/rest_targets_page2.json
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@
}
],
"links": {
"next": "/orgs/39ddc762-b1b9-41ce-ab42-defbe4575bd6/targets?limit=10&version=2022-02-16~experimental&excludeEmpty=true&starting_after=v1.eyJpZCI6IjI5MTk1NjgifQ%3D%3D",
"prev": "/orgs/39ddc762-b1b9-41ce-ab42-defbe4575bd6/targets?limit=10&version=2022-02-16~experimental&excludeEmpty=true&ending_before=v1.eyJpZCI6IjMyODE4NzkifQ%3D%3D"
"next": "/orgs/39ddc762-b1b9-41ce-ab42-defbe4575bd6/targets?limit=10&version=2024-06-21&excludeEmpty=true&starting_after=v1.eyJpZCI6IjI5MTk1NjgifQ%3D%3D",
"prev": "/orgs/39ddc762-b1b9-41ce-ab42-defbe4575bd6/targets?limit=10&version=2024-06-21&excludeEmpty=true&ending_before=v1.eyJpZCI6IjMyODE4NzkifQ%3D%3D"
}
}
2 changes: 1 addition & 1 deletion snyk/test_data/rest_targets_page3.json
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,6 @@
}
],
"links": {
"prev": "/orgs/39ddc762-b1b9-41ce-ab42-defbe4575bd6/targets?limit=10&version=2022-02-16~experimental&excludeEmpty=true&ending_before=v1.eyJpZCI6IjMyODE4ODAifQ%3D%3D"
"prev": "/orgs/39ddc762-b1b9-41ce-ab42-defbe4575bd6/targets?limit=10&version=2024-06-21&excludeEmpty=true&ending_before=v1.eyJpZCI6IjMyODE4ODAifQ%3D%3D"
}
}
Loading