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: Record does not accept GlideString as a key #2152

Closed
adarovadya opened this issue Aug 19, 2024 · 3 comments · Fixed by #2207
Closed

Node: Record does not accept GlideString as a key #2152

adarovadya opened this issue Aug 19, 2024 · 3 comments · Fixed by #2207
Assignees
Labels
bug Something isn't working node Node.js wrapper Release blocker Can't release without.
Milestone

Comments

@adarovadya
Copy link
Collaborator

Describe the bug

there’s an issue with where we use Record and need to provide the option to use GlideString as the key.
Record is an object and does not accept Buffer as a key.
Usually when keys are non-primitive and needed to be store as key-val Map is the solution, we don’t think asking the user to create a map when providing input is a good UX.

Expected Behavior

Records based commands can get GlideString as a key. the expected structure should be:

[
  { key: "field1", value: "value1" },
  { key: "field2", value: "value2" },
  ...
]

Current Behavior

HSET and others Records command does not accept Buffer as a key.

Reproduction Steps

change the hset command: 
public async hset(
        key: GlideString,
        fieldValueMap: Record<GlideString, GlideString>,
        decoder?: Decoder,
---------------------------------------------------------
                const field1 = uuidv4();
                const field2 = uuidv4();
                const value = uuidv4();
                const fieldValueMap = {
                    [Buffer.from(field1)]: Buffer.from(value),
                    [Buffer.from(field2)]: Buffer.from(value),
                };
                expect(await client.hset(key, fieldValueMap)).toEqual(2);

Possible Solution

No response

Additional Information/Context

No response

Client version used

Engine type and version

OS

Language

TypeScript

Language Version

Cluster information

No response

Logs

No response

Other information

No response

@adarovadya adarovadya added bug Something isn't working node Node.js wrapper labels Aug 19, 2024
@adarovadya adarovadya modified the milestones: GA, node-GA Aug 19, 2024
@adarovadya adarovadya moved this to TODO bug fixes in Valkey-GLIDE - internal Aug 19, 2024
@Yury-Fridlyand
Copy link
Collaborator

export type GlideRecord<T> = [key: GlideString, value:T][]

@adarovadya adarovadya self-assigned this Aug 25, 2024
@Yury-Fridlyand
Copy link
Collaborator

Please see PoC in Bit-Quill#418

@Yury-Fridlyand
Copy link
Collaborator

#2207 contains fix for that issue. It is still in WIP state, because changing the approach of Record use causes a lot of changes and test fixes.
Currently it is done ~ 50% and Bit-Quill#421 adds ~30% more.
Most significant parts not done at the moment are - update docs, fixing flaky tests and adding new test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working node Node.js wrapper Release blocker Can't release without.
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants