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

Add SCRIPT SHOW command #2171

Merged
merged 8 commits into from
Sep 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 glide-core/src/protobuf/command_request.proto
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,7 @@ enum RequestType {
ScriptExists = 215;
ScriptFlush = 216;
ScriptKill = 217;
ScriptShow = 218;
}

message Command {
Expand Down
3 changes: 3 additions & 0 deletions glide-core/src/request_type.rs
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,7 @@ pub enum RequestType {
ScriptExists = 215,
ScriptFlush = 216,
ScriptKill = 217,
ScriptShow = 218,
}

fn get_two_word_command(first: &str, second: &str) -> Cmd {
Expand Down Expand Up @@ -455,6 +456,7 @@ impl From<::protobuf::EnumOrUnknown<ProtobufRequestType>> for RequestType {
ProtobufRequestType::ScriptExists => RequestType::ScriptExists,
ProtobufRequestType::ScriptFlush => RequestType::ScriptFlush,
ProtobufRequestType::ScriptKill => RequestType::ScriptKill,
ProtobufRequestType::ScriptShow => RequestType::ScriptShow,
}
}
}
Expand Down Expand Up @@ -679,6 +681,7 @@ impl RequestType {
RequestType::PubSubNumPat => Some(get_two_word_command("PUBSUB", "NUMPAT")),
RequestType::PubSubSChannels => Some(get_two_word_command("PUBSUB", "SHARDCHANNELS")),
RequestType::PubSubSNumSub => Some(get_two_word_command("PUBSUB", "SHARDNUMSUB")),
RequestType::ScriptShow => Some(get_two_word_command("SCRIPT", "SHOW")),
RequestType::ScriptExists => Some(get_two_word_command("SCRIPT", "EXISTS")),
RequestType::ScriptFlush => Some(get_two_word_command("SCRIPT", "FLUSH")),
RequestType::ScriptKill => Some(get_two_word_command("SCRIPT", "KILL")),
Expand Down
13 changes: 13 additions & 0 deletions java/client/src/main/java/glide/api/BaseClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@
import static command_request.CommandRequestOuterClass.RequestType.SScan;
import static command_request.CommandRequestOuterClass.RequestType.SUnion;
import static command_request.CommandRequestOuterClass.RequestType.SUnionStore;
import static command_request.CommandRequestOuterClass.RequestType.ScriptShow;
import static command_request.CommandRequestOuterClass.RequestType.Set;
import static command_request.CommandRequestOuterClass.RequestType.SetBit;
import static command_request.CommandRequestOuterClass.RequestType.SetRange;
Expand Down Expand Up @@ -1871,6 +1872,18 @@ public CompletableFuture<Object> invokeScript(
}
}

@Override
public CompletableFuture<String> scriptShow(@NonNull String sha1) {
return commandManager.submitNewCommand(
ScriptShow, new String[] {sha1}, this::handleStringResponse);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it return null if no script found? If yes, use another handler

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no it panics

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh right valkey-io/valkey#617
Should be added to the doc in all clients

}

@Override
public CompletableFuture<GlideString> scriptShow(@NonNull GlideString sha1) {
return commandManager.submitNewCommand(
ScriptShow, new GlideString[] {sha1}, this::handleGlideStringResponse);
}

@Override
public CompletableFuture<Long> zadd(
@NonNull String key,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,4 +128,36 @@ CompletableFuture<Object> fcall(
*/
CompletableFuture<Object> fcallReadOnly(
GlideString function, GlideString[] keys, GlideString[] arguments);

/**
* Returns the original source code of a script in the script cache.
*
* @see <a href="https://valkey.io/commands/script-show">valkey.io</a> for details.
* @since Valkey 8.0.0 and above.
* @param sha1 The SHA1 digest of the script.
* @return The original source code of the script, if present in the cache. If the script is not
* found in the cache, an error is thrown.
* @example
* <pre>{@code
* String scriptSource = client.scriptShow(luaScript.getHash()).get();
* assert scriptSource.equals("return { KEYS[1], ARGV[1] }");
* }</pre>
*/
CompletableFuture<String> scriptShow(String sha1);

/**
* Returns the original source code of a script in the script cache.
*
* @see <a href="https://valkey.io/commands/script-show">valkey.io</a> for details.
* @since Valkey 8.0.0 and above.
* @param sha1 The SHA1 digest of the script.
* @return The original source code of the script, if present in the cache. If the script is not
* found in the cache, an error is thrown.
* @example
* <pre>{@code
* String scriptSource = client.scriptShow(gs(luaScript.getHash())).get();
* assert scriptSource.equals(gs("return { KEYS[1], ARGV[1] }"));
* }</pre>
*/
CompletableFuture<GlideString> scriptShow(GlideString sha1);
}
23 changes: 23 additions & 0 deletions java/client/src/test/java/glide/api/GlideClientTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@
import static command_request.CommandRequestOuterClass.RequestType.SUnion;
import static command_request.CommandRequestOuterClass.RequestType.SUnionStore;
import static command_request.CommandRequestOuterClass.RequestType.Scan;
import static command_request.CommandRequestOuterClass.RequestType.ScriptShow;
import static command_request.CommandRequestOuterClass.RequestType.Select;
import static command_request.CommandRequestOuterClass.RequestType.SetBit;
import static command_request.CommandRequestOuterClass.RequestType.SetRange;
Expand Down Expand Up @@ -1581,6 +1582,28 @@ public void invokeScript_with_ScriptOptionsGlideString_returns_success() {
assertEquals(payload, response.get());
}

@SneakyThrows
@Test
public void scriptShow_returns_script_source_glidestring() {
// setup
GlideString scriptSource = gs("return { KEYS[1], ARGV[1] }");
GlideString hash = gs(UUID.randomUUID().toString());

CompletableFuture<GlideString> testResponse = new CompletableFuture<>();
testResponse.complete(scriptSource);

when(commandManager.<GlideString>submitNewCommand(
eq(ScriptShow), eq(new GlideString[] {hash}), any()))
.thenReturn(testResponse);

// exercise
CompletableFuture<GlideString> response = service.scriptShow(hash);

// verify
assertEquals(testResponse, response);
assertEquals(scriptSource, response.get());
}

@SneakyThrows
@Test
public void pttl_returns_success() {
Expand Down
27 changes: 27 additions & 0 deletions java/integTest/src/test/java/glide/SharedCommandTests.java
Original file line number Diff line number Diff line change
Expand Up @@ -3244,6 +3244,33 @@ public void invokeScript_gs_test(BaseClient client) {
}
}

@SneakyThrows
@ParameterizedTest(autoCloseArguments = false)
@MethodSource("getClients")
public void scriptShow_test(BaseClient client) {
assumeTrue(SERVER_VERSION.isGreaterThanOrEqualTo("7.9.0"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why 7.9? Use 8.0

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

its release candidate so it would be changed when they will actually release it #2168

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can put version check 8.0 and tests start run on this version once everything implemented

Copy link
Contributor

Choose a reason for hiding this comment

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

or make it a constant we just need to update the constant when it's released.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have already added this before so it wont make any difference now 🙁


String code = "return '" + UUID.randomUUID().toString().substring(0, 5) + "'";
Script script = new Script(code, false);

// Load the script
client.invokeScript(script).get();

// Get the SHA1 digest of the script
String sha1 = script.getHash();

// Test with String
assertEquals(code, client.scriptShow(sha1).get());

// Test with GlideString
assertEquals(gs(code), client.scriptShow(gs(sha1)).get());

// Test with non-existing SHA1
String nonExistingSha1 = UUID.randomUUID().toString();
assertThrows(ExecutionException.class, () -> client.scriptShow(nonExistingSha1).get());
assertThrows(ExecutionException.class, () -> client.scriptShow(gs(nonExistingSha1)).get());
}

@SneakyThrows
@ParameterizedTest(autoCloseArguments = false)
@MethodSource("getClients")
Expand Down
28 changes: 27 additions & 1 deletion node/src/BaseClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ import {
createSScan,
createSUnion,
createSUnionStore,
createScriptShow,
createSet,
createSetBit,
createSetRange,
Expand Down Expand Up @@ -231,9 +232,9 @@ import {
ConfigurationError,
ConnectionError,
ExecAbortError,
ValkeyError,
RequestError,
TimeoutError,
ValkeyError,
} from "./Errors";
import { GlideClientConfiguration } from "./GlideClient";
import {
Expand Down Expand Up @@ -3695,6 +3696,31 @@ export class BaseClient {
return this.createWritePromise(scriptInvocation, options);
}

/**
* Returns the original source code of a script in the script cache.
*
* @see {@link https://valkey.io/commands/script-show|valkey.io} for more details.
* @remarks Since Valkey version 8.0.0.
*
* @param sha1 - The SHA1 digest of the script.
* @param options - (Optional) See {@link DecoderOption}.
* @return The original source code of the script, if present in the cache.
* If the script is not found in the cache, an error is thrown.
*
* @example
* ```typescript
* const scriptHash = script.getHash();
* const scriptSource = await client.scriptShow(scriptHash);
* console.log(scriptSource); // Output: "return { KEYS[1], ARGV[1] }"
* ```
*/
public async scriptShow(
sha1: GlideString,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: is there a reason why we don't pass the Script object and internally we can call script.getHash()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

same as script exists, kill ect...

Copy link
Contributor

Choose a reason for hiding this comment

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

well... the comment could apply to any of these commands.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have no idea 🙉

options?: DecoderOption,
): Promise<GlideString> {
return this.createWritePromise(createScriptShow(sha1), options);
}

/**
* Returns stream entries matching a given range of entry IDs.
*
Expand Down
7 changes: 7 additions & 0 deletions node/src/Commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3998,6 +3998,13 @@ export function createBZPopMin(
return createCommand(RequestType.BZPopMin, [...keys, timeout.toString()]);
}

/**
* @internal
*/
export function createScriptShow(sha1: GlideString): command_request.Command {
return createCommand(RequestType.ScriptShow, [sha1]);
}

/**
* Time unit representation which is used in optional arguments for {@link BaseClient.getex|getex} and {@link BaseClient.set|set} command.
*/
Expand Down
2 changes: 1 addition & 1 deletion node/src/GlideClusterClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
BaseClientConfiguration,
Decoder,
DecoderOption,
GlideRecord, // eslint-disable-line @typescript-eslint/no-unused-vars
GlideRecord,
GlideReturnType,
GlideString,
PubSubMsg,
Expand Down
27 changes: 27 additions & 0 deletions node/tests/SharedTests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4192,6 +4192,33 @@ export function runBaseTests(config: {
config.timeout,
);

it.each([ProtocolVersion.RESP2, ProtocolVersion.RESP3])(
`script show test_%p`,
async (protocol) => {
await runTest(async (client: BaseClient, cluster) => {
if (cluster.checkIfServerVersionLessThan("7.9.0")) {
shohamazon marked this conversation as resolved.
Show resolved Hide resolved
return;
}

const value = uuidv4();
const code = `return '${value}'`;
const script = new Script(Buffer.from(code));

expect(await client.invokeScript(script)).toEqual(value);

// Get the SHA1 digests of the script
const sha1 = script.getHash();

expect(await client.scriptShow(sha1)).toEqual(code);

await expect(
client.scriptShow("non existing sha1"),
).rejects.toThrow(RequestError);
}, protocol);
},
config.timeout,
);

it.each([ProtocolVersion.RESP2, ProtocolVersion.RESP3])(
`zadd and zaddIncr test_%p`,
async (protocol) => {
Expand Down
21 changes: 21 additions & 0 deletions python/python/glide/async_commands/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -5393,6 +5393,27 @@ async def zintercard(
await self._execute_command(RequestType.ZInterCard, args),
)

async def script_show(self, sha1: TEncodable) -> bytes:
"""
Returns the original source code of a script in the script cache.

See https://valkey.io/commands/script-show for more details.

Args:
sha1 (TEncodable): The SHA1 digest of the script.

Returns:
bytes: The original source code of the script, if present in the cache.
If the script is not found in the cache, an error is thrown.

Example:
>>> await client.script_show(script.get_hash())
b"return { KEYS[1], ARGV[1] }"

Since: Valkey version 8.0.0.
"""
return cast(bytes, await self._execute_command(RequestType.ScriptShow, [sha1]))

async def pfadd(self, key: TEncodable, elements: List[TEncodable]) -> int:
"""
Adds all elements to the HyperLogLog data structure stored at the specified `key`.
Expand Down
21 changes: 21 additions & 0 deletions python/python/tests/test_async_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -10477,3 +10477,24 @@ async def attempt_kill_writing_script():

await test_client.close()
await test_client2.close()

@pytest.mark.parametrize("cluster_mode", [True, False])
@pytest.mark.parametrize("protocol", [ProtocolVersion.RESP2, ProtocolVersion.RESP3])
async def test_script_show(self, glide_client: TGlideClient):
min_version = "7.9.0"
shohamazon marked this conversation as resolved.
Show resolved Hide resolved
if await check_if_server_version_lt(glide_client, min_version):
return pytest.mark.skip(reason=f"Valkey version required >= {min_version}")

code = f"return '{get_random_string(5)}'"
script = Script(code)

# Load the scripts
await glide_client.invoke_script(script)

# Get the SHA1 digests of the script
sha1 = script.get_hash()

assert await glide_client.script_show(sha1) == code.encode()

with pytest.raises(RequestError):
await glide_client.script_show("non existing sha1")
Loading