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

Node: Handle commands with binary output in transaction #2133

Closed

Conversation

Yury-Fridlyand
Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand commented Aug 14, 2024

  1. Added option for a command in a transaction to require binary decoder
  2. Added check for this in exec calls
  3. Added IT for that feature
  4. Fixed docs for exec
  5. Added client.close() in IT where it was missing
  6. Reworked shared tests (pair of runBaseTests and runCommonTests) to accept client config and redefine params
  7. Removed unused context from shared tests

Should be merged before #2126 and #2129

@Yury-Fridlyand Yury-Fridlyand added the node Node.js wrapper label Aug 14, 2024
@Yury-Fridlyand Yury-Fridlyand marked this pull request as ready for review August 14, 2024 21:58
@Yury-Fridlyand Yury-Fridlyand requested a review from a team as a code owner August 14, 2024 21:58
@yipin-chen
Copy link
Collaborator

There is a CI failure related to your changes.

@Yury-Fridlyand
Copy link
Collaborator Author

Can't reproduce it locally, but I fixed that.

@@ -1,4 +1,5 @@
#### Changes
* Node: Added handlind commands with binary output in transactions ([#2133](https://github.com/valkey-io/valkey-glide/pull/2133))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* Node: Added handlind commands with binary output in transactions ([#2133](https://github.com/valkey-io/valkey-glide/pull/2133))
* Node: Added handling for commands with binary output in transactions ([#2133](https://github.com/valkey-io/valkey-glide/pull/2133))

Copy link
Contributor

@acarbonetto acarbonetto left a comment

Choose a reason for hiding this comment

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

I think we should align more closely with the Java implementation. I'd suggest that we move the Decoder from the exec() call to the Transaction object. The user can set the decoder (optionally) when creating the Transaction object, or by calling something like .withBinarySafeOutput() like we do in Java.
In this case, exec() would only take a transaction object.
Reference: https://github.com/valkey-io/valkey-glide/pull/1814/files#diff-a9af498f347847324483b3ee806c7a98babbfba5ff1313c7998edecdc652aa5a

transaction.requiresBinaryDecorer
) {
throw new RequestError(
"Transaction has a command which requres `Decoder.Bytes`.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Transaction has a command which requres `Decoder.Bytes`.",
"Transaction has a command which requires `Decoder.Bytes`.",

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe:

"Pass decoder=Decoder.Bytes as part of the exec() command to process transactions that return Bytes."

@Yury-Fridlyand Yury-Fridlyand marked this pull request as draft August 19, 2024 21:56
@Yury-Fridlyand Yury-Fridlyand deleted the node/yuryf-transaction-bin branch September 16, 2024 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
node Node.js wrapper
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants