Skip to content

Fix issue 3139 feat(hscan): add support for encoding.BinaryUnmarshaler#3768

Merged
ndyakov merged 3 commits intoredis:masterfrom
Aaditya-dubey1:fix-issue-3139
Apr 21, 2026
Merged

Fix issue 3139 feat(hscan): add support for encoding.BinaryUnmarshaler#3768
ndyakov merged 3 commits intoredis:masterfrom
Aaditya-dubey1:fix-issue-3139

Conversation

@Aaditya-dubey1
Copy link
Copy Markdown
Contributor

@Aaditya-dubey1 Aaditya-dubey1 commented Apr 8, 2026

Fixes #3139.
Added support for encoding.BinaryUnmarshaler in hscan.Scan to allow scanning into types like UUID.
Included unit test in hscan_test.go to verify the functionality.


Note

Medium Risk
Touches the core hscan struct field scanning path, which is widely used and could subtly change decoding precedence for custom types. Change is small and covered by a focused unit test.

Overview
Adds support for scanning Redis hash values into struct fields whose types implement encoding.BinaryUnmarshaler, calling UnmarshalBinary([]byte) during hscan decoding.

Extends the hscan test suite with a new case that verifies a BinaryUnmarshaler implementation is invoked and receives the raw value bytes.

Reviewed by Cursor Bugbot for commit 4a2d65b. Bugbot is set up for automated code reviews on this repo. Configure here.

@jit-ci
Copy link
Copy Markdown

jit-ci Bot commented Apr 8, 2026

Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset.

In case there are security findings, they will be communicated to you as a comment inside the PR.

Hope you’ll enjoy using Jit.

Questions? Comments? Want to learn more? Get in touch with us.

Copy link
Copy Markdown
Member

@ndyakov ndyakov left a comment

Choose a reason for hiding this comment

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

Hello @Aaditya-dubey1. I do like this, but not sure at this state if we should incorporate it withing v9, let me think about a possible breaking change this can introduce for users. If I can't produce one, I will merge it, if I can - I will hold it for v10.

@Aaditya-dubey1
Copy link
Copy Markdown
Contributor Author

@ndyakov Thanks for the review!

I completely understand the concern around v9 stability. I’ve verified that the implementation preserves existing precedence by prioritizing encoding.TextUnmarshaler before encoding.BinaryUnmarshaler. Since standard types like time.Time and net.IP implement both, their current behavior remains unchanged.

In effect, this only adds support for custom types that previously fell through as unsupported in hscan, so it acts as a backward-compatible feature expansion.

I’ve also added a local test to verify precedence (ensuring UnmarshalText is always preferred when both are implemented), and I’m happy to include it in the PR if that helps increase confidence for a v9 merge.

@ndyakov
Copy link
Copy Markdown
Member

ndyakov commented Apr 20, 2026

Hello @Aaditya-dubey1 I do agree it is backwards compatible in terms of it won't break the current support and the case where this would be a problem does not refer to directly using Scan so it should be fine, thank you!

@Aaditya-dubey1
Copy link
Copy Markdown
Contributor Author

@ndyakov Thank you for reviewing it again. Glad we could clarify the compatibility concerns. Appreciate your time and feedback!

@ndyakov ndyakov merged commit 928f27a into redis:master Apr 21, 2026
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Can't get UUID with Scan()

2 participants