-
Notifications
You must be signed in to change notification settings - Fork 48
feat: sign record's public url #358
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
Changes from 7 commits
17241fe
3b40e57
13cdc63
b01c22a
7a74be9
a404dbf
23505f1
af0f70b
d2a11f3
1bf60b2
d830ed1
9477a14
cf7144e
00a96f7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,15 +58,15 @@ | |
"@apify/input_secrets": "^1.1.40", | ||
"@apify/log": "^2.4.3", | ||
"@apify/timeout": "^0.3.0", | ||
"@apify/utilities": "^2.9.3", | ||
"@crawlee/core": "^3.9.0", | ||
"@crawlee/types": "^3.9.0", | ||
"@crawlee/utils": "^3.9.0", | ||
"apify-client": "^2.12.0", | ||
"@apify/utilities": "^2.13.0", | ||
"@crawlee/core": "^3.12.3-beta.12", | ||
"@crawlee/types": "^3.12.3-beta.12", | ||
"@crawlee/utils": "^3.12.3-beta.12", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and don't forget to bump this |
||
"apify-client": "^2.11.1", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why the downgrade here? 👀 |
||
"fs-extra": "^11.2.0", | ||
"ow": "^0.28.2", | ||
"semver": "^7.5.4", | ||
"tslib": "^2.6.2", | ||
"ws": "^8.18.0" | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ export interface ConfigurationOptions extends CoreConfigurationOptions { | |
actorRunId?: string; | ||
actorTaskId?: string; | ||
apiBaseUrl?: string; | ||
apiPublicBaseUrl?: string; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lets add a comment here about the difference between |
||
containerPort?: number; | ||
containerUrl?: string; | ||
proxyHostname?: string; | ||
|
@@ -139,6 +140,7 @@ export class Configuration extends CoreConfiguration { | |
APIFY_ACTOR_EVENTS_WS_URL: 'actorEventsWsUrl', | ||
APIFY_ACTOR_ID: 'actorId', | ||
APIFY_API_BASE_URL: 'apiBaseUrl', | ||
APIFY_API_PUBLIC_BASE_URL: 'apiPublicBaseUrl', | ||
APIFY_IS_AT_HOME: 'isAtHome', | ||
APIFY_ACTOR_RUN_ID: 'actorRunId', | ||
APIFY_ACTOR_TASK_ID: 'actorTaskId', | ||
|
@@ -183,6 +185,7 @@ export class Configuration extends CoreConfiguration { | |
defaultRequestQueueId: LOCAL_ACTOR_ENV_VARS[ACTOR_ENV_VARS.DEFAULT_REQUEST_QUEUE_ID], | ||
inputKey: 'INPUT', | ||
apiBaseUrl: 'https://api.apify.com', | ||
apiPublicBaseUrl: 'https://api.apify.com', | ||
B4nan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
proxyStatusUrl: 'http://proxy.apify.com', | ||
proxyHostname: LOCAL_APIFY_ENV_VARS[APIFY_ENV_VARS.PROXY_HOSTNAME], | ||
proxyPort: +LOCAL_APIFY_ENV_VARS[APIFY_ENV_VARS.PROXY_PORT], | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
import { createHmacSignature } from '@apify/utilities'; | ||
import type { StorageManagerOptions } from '@crawlee/core'; | ||
import { KeyValueStore as CoreKeyValueStore } from '@crawlee/core'; | ||
|
||
|
@@ -15,11 +16,18 @@ export class KeyValueStore extends CoreKeyValueStore { | |
* access the value in the remote key-value store. | ||
*/ | ||
override getPublicUrl(key: string): string { | ||
if (!(this.config as Configuration).get('isAtHome') && getPublicUrl) { | ||
const config = this.config as Configuration; | ||
if (!config.get('isAtHome') && getPublicUrl) { | ||
return getPublicUrl.call(this, key); | ||
} | ||
|
||
return `https://api.apify.com/v2/key-value-stores/${this.id}/records/${key}`; | ||
const publicUrl = new URL(`${config.get('apiPublicBaseUrl', 'https://api.apify.com')}/v2/key-value-stores/${this.id}/records/${key}`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no need for the explicit default here, its the exact same value as you define on the config level |
||
|
||
if (this.storageObject?.urlSigningSecretKey) { | ||
publicUrl.searchParams.append('signature', createHmacSignature(this.storageObject.urlSigningSecretKey as string, key)); | ||
} | ||
|
||
return publicUrl.toString(); | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
import { Actor, log } from 'apify'; | ||
|
||
// Also needs to be changed in test.mjs | ||
const PUBLIC_RECORD_KEY = 'public-record-key'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (opt) How about you pass this as input to the Actor? All three values...
Then you don't need to define anything in two places and the test will be more readable (not that it is super complicated or anything 😄 ) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good point, thank you 🙏 Refactored it |
||
const PUBLIC_DATA = { exposedData: 'test' }; | ||
|
||
await Actor.init(); | ||
|
||
await Actor.setValue(PUBLIC_RECORD_KEY, JSON.stringify(PUBLIC_DATA), { contentType: `application/json` }); | ||
|
||
const defaultKeyValueStore = await Actor.openKeyValueStore(); | ||
const publicUrl = defaultKeyValueStore.getPublicUrl(PUBLIC_RECORD_KEY); | ||
|
||
// Here we store the url itself | ||
await Actor.setValue('urlToPublicData', publicUrl); | ||
|
||
log.info('Generated public url', { publicUrl }); | ||
|
||
await Actor.pushData({ publicUrl }); | ||
|
||
await Actor.exit(); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
import assert from 'node:assert/strict'; | ||
|
||
import { ApifyClient } from 'apify'; | ||
|
||
// Also needs to be changed in main.mjs | ||
const PUBLIC_DATA = { exposedData: 'test' }; | ||
|
||
const client = new ApifyClient({ | ||
token: process.env.APIFY_TOKEN, | ||
}); | ||
|
||
const actor = client.actor(process.argv[2]); | ||
|
||
const run = await actor.call({}, { waitSecs: 15 }); | ||
assert.equal(run.exitCode, 0); | ||
|
||
const publicUrl = await client.keyValueStore(run.defaultKeyValueStoreId).getRecord('urlToPublicData'); | ||
const data = await fetch(publicUrl.value).then((res) => res.json()); | ||
|
||
assert.deepEqual(data, PUBLIC_DATA); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please keep the full version definition here