Skip to content

Add redis.Scan() to scan results from redis maps into structs. #1631

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

Merged
merged 11 commits into from
Feb 4, 2021
Merged

Add redis.Scan() to scan results from redis maps into structs. #1631

merged 11 commits into from
Feb 4, 2021

Conversation

knadh
Copy link
Contributor

@knadh knadh commented Jan 28, 2021

The package uses reflection to decode default types (int, string etc.) from Redis map results (key-value pair sequences) into
struct fields where the fields are matched to Redis keys by tags.

Similar to how encoding/json allows custom decoders usingUnmarshalJSON(), the package supports decoding of arbitrary
types into struct fields by defining a Decode(string) errorfunction on types.

The field/type spec of every struct that's passed to Scan() is cached in the package so that subsequent scans avoid iteration
and reflection of the struct's fields.

Issue: #1603

@vmihailenco
Copy link
Collaborator

vmihailenco commented Jan 28, 2021

👍

I've left some comments - hopefully it is not too much :) Also please set "Allow edits from maintainers" on the PR so I can add commits...

Let's move the code to internal/hscan package for now so all types are private. And the public API could look like err := rdb.HGetAll(ctx, "key").Scan(&strct).

The package uses reflection to decode default types (int, string
etc.) from Redis map results (key-value pair sequences) into
struct fields where the fields are matched to Redis keys by tags.

Similar to how `encoding/json` allows custom decoders using
`UnmarshalJSON()`, the package supports decoding of arbitrary
types into struct fields by defining a `Decode(string) error`
function on types.

The field/type spec of every struct that's passed to Scan() is
cached in the package so that subsequent scans avoid iteration
and reflection of the struct's fields.
@knadh
Copy link
Contributor Author

knadh commented Jan 28, 2021

Incorporated all the changes you suggested and moved the package to internal/hscan.

PS: Did a force push because it was mostly hygiene fixes.

@vmihailenco
Copy link
Collaborator

@knadh I've pushed few small changes, but this looks good already.

Could you add SliceCmd.Scan and a test (or two :)?

@knadh
Copy link
Contributor Author

knadh commented Jan 29, 2021

Could you add SliceCmd.Scan and a test (or two :)?

Yep, will push this.

@knadh
Copy link
Contributor Author

knadh commented Feb 2, 2021

Could you add SliceCmd.Scan and a test (or two :)?

Added here: a4144ea

tag := f.Tag.Get(fieldTag)
if tag == "" || tag == "-" {
continue
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks we are ignoring fields that don't have redis:"something" tag. Is that intentional?

Usually we are supposed to generate default name from Go field name and only ignore the field when redis:"-". What do you think?

https://pkg.go.dev/github.com/codemodus/kace could be used to generate default name.

Copy link
Collaborator

Choose a reason for hiding this comment

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

this parser is too simple....can only do the most basic work, it needs to continue to improve its functions,
i can continue to improve it this week,,,if needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks we are ignoring fields that don't have redis:"something" tag. Is that intentional?

Yeah. Explicitly defining what's to be scanned is better (fewer gotchas) in my opinion. Opting in rather than opting out with "-".

this parser is too simple....can only do the most basic work, it needs to continue to improve its functions,

It only handles primitive types. The right way to extend it with the Decode() interface which was in the original PR. I hope @vmihailenco has plans to introduce that functionality soon!

@vmihailenco
Copy link
Collaborator

Could you add one end-to-end test or maybe even an example? That will help people to get started

rdb.HSet(..)
rdb.HSet(...)

rdb.HGetAll(ctx, "key").Scan(&dst)

@knadh
Copy link
Contributor Author

knadh commented Feb 2, 2021

I just realised, StringStringMapCmd will also need to support Scan() for HGetAll() to work. Okay to add?

@vmihailenco
Copy link
Collaborator

@knadh I've pushed a change with StructValue so StringStringMapCmd.Scan can be slightly more efficient. I think this PR is ready to be merged if you are happy with that change. It will be part of v8 but marked unstable so we can change it based on the feedback.

Float float32 `redis:"float"`
Bool bool `redis:"bool"`
}
var d2 data2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just FYI this can be written as

var d2 struct {
			String string  `redis:"string"`
			Bytes  []byte  `redis:"byte"`
			Int    int     `redis:"int"`
			Uint   uint    `redis:"uint"`
			Float  float32 `redis:"float"`
			Bool   bool    `redis:"bool"`
}

@monkey92t
Copy link
Collaborator

monkey92t commented Feb 3, 2021

missing test for mget command, we should complete it.
and...if it is 386 platform, there may be ERROR: "strconv.ParseInt: parsing "1234567890123456789": value out of range".
reflect.Int64、reflect.Uint64、reflect.Float64,
but I’m not sure if we still need to be compatible with the 386 platform

redis 127.0.0.1:6379>> set key 1234567890123456789

type Temp struct {
	Key int64 `redis:"key"`
}

GOARCH=386 go test it should run normally.

@knadh
Copy link
Contributor Author

knadh commented Feb 3, 2021

missing test for mget command, we should complete it.

Added test https://github.com/knadh/redis/commit/f8a546b4820de9e52fe0d244e24dca155688abef. @monkey92t I guess you could add platform specific parsing.

@knadh I've pushed a change with StructValue so StringStringMapCmd.Scan can be slightly more efficient. I think this PR is ready to be merged if you are happy with that change. It will be part of v8 but marked unstable so we can change it based on the feedback.

@vmihailenco looks fine. Please go ahead.

@vmihailenco
Copy link
Collaborator

🥳

@vmihailenco vmihailenco merged commit 35f6ccd into redis:master Feb 4, 2021
@mdelclaro
Copy link

mdelclaro commented Feb 25, 2021

Hello @vmihailenco @knadh

What if in this case I have a slice like []interface{} instead of an interface only? I'm getting this redis.Scan(non-struct *[]models.Price)

I see that in the code you check for if v.Kind() != reflect.Struct

I need it to be a slice since the result I'm getting from MGet is a slice.

Any ideias on what I'm doing wrong?

@monkey92t
Copy link
Collaborator

monkey92t commented Feb 25, 2021

Hello,

What if in this case I have a slice like []interface{} instead of an interface only? I'm getting this redis.Scan(non-struct *[]models.Price)

I see that in the code you check for if v.Kind() != reflect.Struct

I need it to be a slice since the result I'm getting from MGet is a slice.

Any ideias on what I'm doing wrong?

under normal circumstances, the data format of redis is as follows:

127.0.0.1:6379>>set id 100
127.0.0.1:6379>>set name monkey
127.0.0.1:6379>>set age 20

127.0.0.1:6379>>mget id name age
//{"id": 100, "name": "monkey", "age": 20}

//golang
type Per struct {
	ID   int    `redis:"id"`
	Name string `redis:"name"`
	Age  int    `redis:"age"`
}
p := &Per{}
err := redis.Mget(ctx, "id", "name", "age").Scan(p)

// p.ID = 100
// p.Name = monkey
// p.Age = 20

i don’t know what you mean by []interface, if i don’t understand correctly, you can add an example to describe your problem in detail.

@mdelclaro
Copy link

Hello,
What if in this case I have a slice like []interface{} instead of an interface only? I'm getting this redis.Scan(non-struct *[]models.Price)
I see that in the code you check for if v.Kind() != reflect.Struct
I need it to be a slice since the result I'm getting from MGet is a slice.
Any ideias on what I'm doing wrong?

under normal circumstances, the data format of redis is as follows:

127.0.0.1:6379>>set id 100
127.0.0.1:6379>>set name monkey
127.0.0.1:6379>>set age 20

127.0.0.1:6379>>mget id name age
//{"id": 100, "name": "monkey", "age": 20}

//golang
type Per struct {
	ID   int    `redis:"id"`
	Name string `redis:"name"`
	Age  int    `redis:"age"`
}
p := &Per{}
err := redis.Mget(ctx, "id", "name", "age").Scan(p)

// p.ID = 100
// p.Name = monkey
// p.Age = 20

i don’t know what you mean by []interface, if i don’t understand correctly, you can add an example to describe your problem in detail.

I have hundreds of keys like price/tokenX-tokenY, so for what I see I should create a struct with all the keys, which would not be possible. And each value is a JSON structure.

The response from MGet is something like this:

[ {"id":1,"base":"X","quote":"Y", "price": 1} ]

I'm a little lost here on what I have to do :')

@mdelclaro
Copy link

Hello,
What if in this case I have a slice like []interface{} instead of an interface only? I'm getting this redis.Scan(non-struct *[]models.Price)
I see that in the code you check for if v.Kind() != reflect.Struct
I need it to be a slice since the result I'm getting from MGet is a slice.
Any ideias on what I'm doing wrong?

under normal circumstances, the data format of redis is as follows:

127.0.0.1:6379>>set id 100
127.0.0.1:6379>>set name monkey
127.0.0.1:6379>>set age 20

127.0.0.1:6379>>mget id name age
//{"id": 100, "name": "monkey", "age": 20}

//golang
type Per struct {
	ID   int    `redis:"id"`
	Name string `redis:"name"`
	Age  int    `redis:"age"`
}
p := &Per{}
err := redis.Mget(ctx, "id", "name", "age").Scan(p)

// p.ID = 100
// p.Name = monkey
// p.Age = 20

i don’t know what you mean by []interface, if i don’t understand correctly, you can add an example to describe your problem in detail.

I have hundreds of keys like price/tokenX-tokenY, so for what I see I should create a struct with all the keys, which would not be possible. And each value is a JSON structure.

The response from MGet is something like this:

[ {"id":1,"base":"X","quote":"Y", "price": 1} ]

I'm a little lost here on what I have to do :')

I resolved it by doing the following:

res := Cli.MGet(ctx, keys...)

var prices []models.Price

str := fmt.Sprintf("%+v", res.Val())
rawIn := json.RawMessage(str)

bytes, err := rawIn.MarshalJSON()

json.Unmarshal(bytes, &prices)

Thanks!

@knadh
Copy link
Contributor Author

knadh commented Feb 26, 2021

@mdelclaro The Scan() function is specifically for scanning Redis values by their keys into structs. This is useful for MGET, HGETALL etc. In your example, you are pulling JSON strings as values from Redis and scanning keys of JSON strings, not keys from Redis. So, you indeed have to use json.Unmarshal.

@csdenboer
Copy link

csdenboer commented Jul 1, 2021

Scan doesn’t seem to work on nested structures: #1806. Based on the comments in this PR, it seems that “the package supports decoding of arbitrary
types into struct fields by defining a Decode(string) error function on types” was not actually implemented. Is this correct? If yes, will it be implemented anytime soon? Is there any other way to make this work?

@ysweid
Copy link

ysweid commented Sep 11, 2021

Decoding time.Time could be a big win.

@haijianyang
Copy link

cannot scan redis.result 2021-12-28T11:31:45.611441+08:00 into struct field UpdatedAt of type time.Time, error-redis.Scan(unsupported time.Time)

Hi guys, when scan will support time.Time?

@knadh
Copy link
Contributor Author

knadh commented Dec 28, 2021

The issue with time.Time is the format to parse. Since 2021-12-28T11:31:45.611441+08:00 is the default format that's stringified, maybe we can hardcode scanning for just this one format and document that. @vmihailenco what do you think?

@errpunk
Copy link

errpunk commented Dec 12, 2022

ping,
really need this

@monkey92t
Copy link
Collaborator

monkey92t commented Dec 24, 2022

Add Scanner interface, See #2317

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.

8 participants