Skip to content

fix: Presigned resource urls shouldn't follow base url #500

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Sep 12, 2025
Merged

fix: Presigned resource urls shouldn't follow base url #500

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bb5a260
Allow to define public api url different from api url
Pijukatel Sep 10, 2025
1db6008
Add option to define custom public url
Pijukatel Sep 10, 2025
6fb6f03
Apply suggestions from code review
Pijukatel Sep 10, 2025
d7163b8
Add non existent url test
Pijukatel Sep 10, 2025
77c5f20
Treat url and public url the same way with respect to the v2 part of …
Pijukatel Sep 10, 2025
a28026c
Refactor the tests to avoid real API calls
Pijukatel Sep 11, 2025
116621b
Update tests based on review
Pijukatel Sep 12, 2025
2d31a37
Move shared parametrized_api_urls to util file
Pijukatel Sep 12, 2025
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
1 change: 1 addition & 0 deletions tests/integration/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
('https://api.apify.com', 'https://custom-public-url.com'),
('https://api.apify.com', 'https://custom-public-url.com/with/custom/path'),
('https://api.apify.com', 'https://custom-public-url.com/with/custom/path/'),
('http://10.0.88.214:8010', 'https://api.apify.com'),
Copy link
Contributor

Choose a reason for hiding this comment

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

I hate to be a stickler for detail, but now we're testing this:

client = ApifyClient(base_url="10.0.0...", public_base_url="https://api.apify.com")
assert "api.apify.com" in client.kvs.public_url()

While the failing case from the original issue would be something like

client = ApifyClient(base_url="10.0.0...") # no public url passed, only base
assert "api.apify.com" in client.kvs.public_url()  # yet, this assert still passes

Not that it really matters, but I wanted to propose this with the previous commit and ultimately revoked my comment, because I noticed this difference :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

],
)

Expand Down
67 changes: 39 additions & 28 deletions tests/integration/test_dataset.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
from __future__ import annotations

from unittest import mock
from unittest.mock import Mock

import impit

from integration.conftest import parametrized_api_urls
Expand All @@ -8,6 +11,32 @@
from apify_client import ApifyClient, ApifyClientAsync
from apify_client.client import DEFAULT_API_URL

MOCKED_API_DATASET_RESPONSE = """{
"data": {
"id": "someID",
"name": "name",
"userId": "userId",
"createdAt": "2025-09-11T08:48:51.806Z",
"modifiedAt": "2025-09-11T08:48:51.806Z",
"accessedAt": "2025-09-11T08:48:51.806Z",
"actId": null,
"actRunId": null,
"schema": null,
"stats": {
"readCount": 0,
"writeCount": 0,
"deleteCount": 0,
"listCount": 0,
"storageBytes": 0
},
"fields": [],
"consoleUrl": "https://console.apify.com/storage/datasets/someID",
"itemsPublicUrl": "https://api.apify.com/v2/datasets/someID/items",
"generalAccess": "FOLLOW_USER_SETTING",
"urlSigningSecretKey": "urlSigningSecretKey"
}
}"""


class TestDatasetSync:
def test_dataset_should_create_public_items_expiring_url_with_params(self, apify_client: ApifyClient) -> None:
Expand Down Expand Up @@ -49,24 +78,15 @@ def test_dataset_should_create_public_items_non_expiring_url(self, apify_client:
@parametrized_api_urls
def test_public_url(self, api_token: str, api_url: str, api_public_url: str) -> None:
apify_client = ApifyClient(token=api_token, api_url=api_url, api_public_url=api_public_url)
created_store = apify_client.datasets().get_or_create(name=random_resource_name('key-value-store'))
dataset = apify_client.dataset(created_store['id'])
try:
dataset = apify_client.dataset('someID')

# Mock the API call to return predefined response
with mock.patch.object(apify_client.http_client, 'call', return_value=Mock(text=MOCKED_API_DATASET_RESPONSE)):
public_url = dataset.create_items_public_url()
assert public_url == (
f'{(api_public_url or DEFAULT_API_URL).strip("/")}/v2/datasets/'
f'{created_store["id"]}/items?signature={public_url.split("signature=")[1]}'
f'someID/items?signature={public_url.split("signature=")[1]}'
)
finally:
dataset.delete()

def test_public_url_nonexistent_host(self, api_token: str) -> None:
dataset_name = 'whatever'
non_existent_url = 'http://10.0.88.214:8010'
apify_client = ApifyClient(token=api_token, api_url=non_existent_url)
kvs_client = apify_client.dataset(dataset_id=dataset_name)
assert kvs_client._url() == f'{non_existent_url}/v2/datasets/{dataset_name}'
assert kvs_client._url(public=True) == f'{DEFAULT_API_URL}/v2/datasets/{dataset_name}'


class TestDatasetAsync:
Expand Down Expand Up @@ -113,21 +133,12 @@ async def test_dataset_should_create_public_items_non_expiring_url(
@parametrized_api_urls
async def test_public_url(self, api_token: str, api_url: str, api_public_url: str) -> None:
apify_client = ApifyClientAsync(token=api_token, api_url=api_url, api_public_url=api_public_url)
created_store = await apify_client.datasets().get_or_create(name=random_resource_name('key-value-store'))
dataset = apify_client.dataset(created_store['id'])
try:
dataset = apify_client.dataset('someID')

# Mock the API call to return predefined response
with mock.patch.object(apify_client.http_client, 'call', return_value=Mock(text=MOCKED_API_DATASET_RESPONSE)):
public_url = await dataset.create_items_public_url()
assert public_url == (
f'{(api_public_url or DEFAULT_API_URL).strip("/")}/v2/datasets/'
f'{created_store["id"]}/items?signature={public_url.split("signature=")[1]}'
f'someID/items?signature={public_url.split("signature=")[1]}'
)
finally:
await dataset.delete()

def test_public_url_nonexistent_host(self, api_token: str) -> None:
dataset_name = 'whatever'
non_existent_url = 'http://10.0.88.214:8010'
apify_client = ApifyClientAsync(token=api_token, api_url=non_existent_url)
kvs_client = apify_client.dataset(dataset_id=dataset_name)
assert kvs_client._url() == f'{non_existent_url}/v2/datasets/{dataset_name}'
assert kvs_client._url(public=True) == f'{DEFAULT_API_URL}/v2/datasets/{dataset_name}'
68 changes: 38 additions & 30 deletions tests/integration/test_key_value_store.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
from __future__ import annotations

from unittest import mock
from unittest.mock import Mock

import impit

from integration.conftest import parametrized_api_urls
Expand All @@ -8,6 +11,31 @@
from apify_client import ApifyClient, ApifyClientAsync
from apify_client.client import DEFAULT_API_URL

MOCKED_API_KVS_RESPONSE = """{
"data": {
"id": "someID",
"name": "name",
"userId": "userId",
"createdAt": "2025-09-11T08:48:51.806Z",
"modifiedAt": "2025-09-11T08:48:51.806Z",
"accessedAt": "2025-09-11T08:48:51.806Z",
"actId": null,
"actRunId": null,
"schema": null,
"stats": {
"readCount": 0,
"writeCount": 0,
"deleteCount": 0,
"listCount": 0,
"storageBytes": 0
},
"consoleUrl": "https://console.apify.com/storage/key-value-stores/someID",
"keysPublicUrl": "https://api.apify.com/v2/key-value-stores/someID/keys",
"generalAccess": "FOLLOW_USER_SETTING",
"urlSigningSecretKey": "urlSigningSecretKey"
}
}"""


class TestKeyValueStoreSync:
def test_key_value_store_should_create_expiring_keys_public_url_with_params(
Expand Down Expand Up @@ -49,24 +77,15 @@ def test_key_value_store_should_create_public_keys_non_expiring_url(self, apify_
@parametrized_api_urls
def test_public_url(self, api_token: str, api_url: str, api_public_url: str) -> None:
apify_client = ApifyClient(token=api_token, api_url=api_url, api_public_url=api_public_url)
created_store = apify_client.key_value_stores().get_or_create(name=random_resource_name('key-value-store'))
kvs = apify_client.key_value_store(created_store['id'])
try:
kvs = apify_client.key_value_store('someID')

# Mock the API call to return predefined response
with mock.patch.object(apify_client.http_client, 'call', return_value=Mock(text=MOCKED_API_KVS_RESPONSE)):
public_url = kvs.create_keys_public_url()
assert public_url == (
f'{(api_public_url or DEFAULT_API_URL).strip("/")}/v2/key-value-stores/'
f'{created_store["id"]}/keys?signature={public_url.split("signature=")[1]}'
f'someID/keys?signature={public_url.split("signature=")[1]}'
)
finally:
kvs.delete()

def test_public_url_nonexistent_host(self, api_token: str) -> None:
kvs_name = 'whatever'
non_existent_url = 'http://10.0.88.214:8010'
apify_client = ApifyClient(token=api_token, api_url=non_existent_url)
kvs_client = apify_client.key_value_store(key_value_store_id=kvs_name)
assert kvs_client._url() == f'{non_existent_url}/v2/key-value-stores/{kvs_name}'
assert kvs_client._url(public=True) == f'{DEFAULT_API_URL}/v2/key-value-stores/{kvs_name}'


class TestKeyValueStoreAsync:
Expand Down Expand Up @@ -115,23 +134,12 @@ async def test_key_value_store_should_create_public_keys_non_expiring_url(
@parametrized_api_urls
async def test_public_url(self, api_token: str, api_url: str, api_public_url: str) -> None:
apify_client = ApifyClientAsync(token=api_token, api_url=api_url, api_public_url=api_public_url)
created_store = await apify_client.key_value_stores().get_or_create(
name=random_resource_name('key-value-store')
)
kvs = apify_client.key_value_store(created_store['id'])
try:
kvs = apify_client.key_value_store('someID')

# Mock the API call to return predefined response
with mock.patch.object(apify_client.http_client, 'call', return_value=Mock(text=MOCKED_API_KVS_RESPONSE)):
public_url = await kvs.create_keys_public_url()
assert public_url == (
f'{(api_public_url or DEFAULT_API_URL).strip("/")}/v2/key-value-stores/'
f'{created_store["id"]}/keys?signature={public_url.split("signature=")[1]}'
f'someID/keys?signature={public_url.split("signature=")[1]}'
)
finally:
await kvs.delete()

async def test_public_url_nonexistent_host(self, api_token: str) -> None:
kvs_name = 'whatever'
non_existent_url = 'http://10.0.88.214:8010'
apify_client = ApifyClientAsync(token=api_token, api_url=non_existent_url)
kvs_client = apify_client.key_value_store(key_value_store_id=kvs_name)
assert kvs_client._url() == f'{non_existent_url}/v2/key-value-stores/{kvs_name}'
assert kvs_client._url(public=True) == f'{DEFAULT_API_URL}/v2/key-value-stores/{kvs_name}'
Loading