Skip to content

Conversation

@Pijukatel
Copy link
Contributor

@Pijukatel Pijukatel commented Sep 29, 2025

Description

  • Add KeyValueStoreClient.get_record_public_url.
  • Add KeyValueStoreClientAsync.get_record_public_url.
  • Add tests.

Issues

Closes: #497

@github-actions github-actions bot added this to the 124th sprint - Tooling team milestone Sep 29, 2025
@github-actions github-actions bot added t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics. labels Sep 29, 2025
@Pijukatel Pijukatel marked this pull request as ready for review September 29, 2025 13:01
@Pijukatel Pijukatel requested review from barjin and vdusek September 29, 2025 13:01
Copy link
Contributor

@barjin barjin left a comment

Choose a reason for hiding this comment

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

The changes seem sound to me, thank you! 👍

I have one nit / idea regarding the tests:

Comment on lines 90 to 93
expected_signature = f'?signature={public_url.split("signature=")[1]}' if signing_key else ''
assert public_url == (
f'{(api_public_url or DEFAULT_API_URL).strip("/")}/v2/key-value-stores/someID/keys{expected_signature}'
)
Copy link
Contributor

Choose a reason for hiding this comment

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

In all the (new) tests, it seems we're parsing the signature query param from the public_url, only to test if public_url contains this query param.

something like

const { signature } = public_url;
assert public_url === `abc.def/${signature}`;

Can we check whether, e.g., the signature is the actual HMAC of the key instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added expected signature value to the tests

Copy link
Contributor

@vdusek vdusek left a comment

Choose a reason for hiding this comment

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

LGTM

@Pijukatel Pijukatel force-pushed the generating-presigned-url branch from a6f448a to e1bfec3 Compare September 30, 2025 07:10
@Pijukatel Pijukatel merged commit 6417d26 into master Sep 30, 2025
26 checks passed
@Pijukatel Pijukatel deleted the generating-presigned-url branch September 30, 2025 07:16
Pijukatel added a commit to apify/apify-sdk-python that referenced this pull request Oct 20, 2025
### Description
- Use the Apify client for creating a public URL. This was added in the
latest release of the client:
  - apify/apify-client-python#500
  - apify/apify-client-python#506

- Refactor Apify storage creation to reduce code duplication

### Issues

- Closes: #612
- Related: #635
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow generating presigned URLs for KVS records

4 participants