omitempty tag does not work for empty time.Time struct field when using HSet

[gh73962]

Issue tracker is used for reporting bugs and discussing new features. Please use
stackoverflow for supporting issues.

Expected Behavior

type MyHash struct { 
    Key1 string `redis:"key1"`;
    Key2 time.Time `redis:"key2,omitempty"` 
}

should be

hset myhash key1 value1

Current Behavior

hset myhash key1 value1 key2 0001-01-01T00:00:00Z

Possible Solution

https://github.com/redis/go-redis/blob/master/commands.go#L142

func isEmptyValue(v reflect.Value) bool {
	switch v.Kind() {
	case reflect.Array, reflect.Map, reflect.Slice, reflect.String:
		return v.Len() == 0
	case reflect.Bool:
		return !v.Bool()
	case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
		return v.Int() == 0
	case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr:
		return v.Uint() == 0
	case reflect.Float32, reflect.Float64:
		return v.Float() == 0
	case reflect.Interface, reflect.Pointer:
		return v.IsNil()
++++++++++++++++++++++++++++++++++++++++++++++
        case reflect.Struct:
                return v.IsZero()
++++++++++++++++++++++++++++++++++++++++++++++
	}
	return false
}

Steps to Reproduce

type MyHash struct { 
    Key1 string `redis:"key1"`;
    Key2 time.Time `redis:"key2,omitempty"` 
}

data := MyHash{
     Key1 : "value1",
     Key2 : time.Time{},
}

if err := rdb.HSet(ctx, "key", model1).Err(); err != nil {
	panic(err)
}

Context (Environment)

Detailed Description

omitempty tag does not work for empty time.Time struct field when using HSet

Possible Implementation

If you agree with the proposal I made in the title "Possible Solution," I can proceed to complete this PR.

[SoulPancake]

This solution makes sense to me, have you tried it? @gh73962

[aliforever]

Also if you use *time.Time without omitempty tag, it would panic with nil pointer dereference. Is that the intended behavior?

[SoulPancake]

This is the expected behaviour right now K guess but with go 1.24
We have omitzero which solves this directly

https://www.bytesizego.com/blog/go-124-omitzero#:~:text=What%20Does%20omitzero,nil%20(nothing)

Take a look at this

[aliforever]

This is the expected behaviour right now K guess but with go 1.24 We have omitzero which solves this directly

https://www.bytesizego.com/blog/go-124-omitzero#:~:text=What%20Does%20omitzero,nil%20(nothing)

Take a look at this

I was checking the code, it seems like it doesn't respect when a struct field is nil or not, it just goes on and dereference it. This seems to be a bug for me, check following function for instance.

First of all unlike other types (bool, string, ...) it doesn't consider a *time.Time, second, it doesn't check whether a pointer is nil or not.

[gh73962]

This solution makes sense to me, have you tried it?

In my use case, this solution is work. If you think your approach works, I need collect more test cases. However, directly checking case reflect.Struct feels a bit rough and not very elegant. I don’t have a better solution on my end either. @SoulPancake

[ndyakov]

@aliforever would you like to open a PR for the potential nil dereference that can occur in writeArgs ?

@gh73962 the proposed solution makes sense to me.

[gh73962]

Upon a thorough examination, the function isEmptyValue(v reflect.Value) bool bears a resemblance to the functionality of standard library functions and can be utilized directly.But the default case of the func (v Value) IsZero() bool will panic.

https://github.com/golang/go/blob/go1.18/src/reflect/value.go#L1520

// IsZero reports whether v is the zero value for its type.
// It panics if the argument is invalid.
func (v Value) IsZero() bool {

The appropriate solution is. if omitEmpty(opt) && isEmptyValue(field) { -> if omitEmpty(opt) && field.IsZero() {
https://github.com/redis/go-redis/blob/master/commands.go#L104

// appendStructField appends the field and value held by the structure v to dst, and returns the appended dst.
func appendStructField(dst []interface{}, v reflect.Value) []interface{} {
	typ := v.Type()
	for i := 0; i < typ.NumField(); i++ {
		tag := typ.Field(i).Tag.Get("redis")
		if tag == "" || tag == "-" {
			continue
		}
		name, opt, _ := strings.Cut(tag, ",")
		if name == "" {
			continue
		}

		field := v.Field(i)

		// miss field
+               if omitEmpty(opt) && field.IsZero() {
			continue
		}

		if field.CanInterface() {
			dst = append(dst, name, field.Interface())
		}
	}

	return dst
}

I will first submit a PR based on the solution initially proposed.

[aliforever]

@aliforever would you like to open a PR for the potential nil dereference that can occur in writeArgs ?

@gh73962 the proposed solution makes sense to me.

Sure, though @gh73962 has also said will open a PR

@gh73962 Will your solution fix the issue with nil pointers?

Upon a thorough examination, the function isEmptyValue(v reflect.Value) bool bears a resemblance to the functionality of standard library functions and can be utilized directly.But the default case of the func (v Value) IsZero() bool will panic.

https://github.com/golang/go/blob/go1.18/src/reflect/value.go#L1520

// IsZero reports whether v is the zero value for its type.
// It panics if the argument is invalid.
func (v Value) IsZero() bool {

The appropriate solution is. if omitEmpty(opt) && isEmptyValue(field) { -> if omitEmpty(opt) && field.IsZero() { https://github.com/redis/go-redis/blob/master/commands.go#L104

// appendStructField appends the field and value held by the structure v to dst, and returns the appended dst.
func appendStructField(dst []interface{}, v reflect.Value) []interface{} {
	typ := v.Type()
	for i := 0; i < typ.NumField(); i++ {
		tag := typ.Field(i).Tag.Get("redis")
		if tag == "" || tag == "-" {
			continue
		}
		name, opt, _ := strings.Cut(tag, ",")
		if name == "" {
			continue
		}

		field := v.Field(i)

		// miss field
+               if omitEmpty(opt) && field.IsZero() {
			continue
		}

		if field.CanInterface() {
			dst = append(dst, name, field.Interface())
		}
	}

	return dst
}

I will first submit a PR based on the solution initially proposed.

[gh73962]

I haven't tried to reproduce the issue I mentioned yet. I need to reproduce it first to identify the problem before handling it @aliforever

[aliforever]

I created a PR for nil point dereferencing issue @ndyakov:
#3271

[ndyakov]

@gh73962 there is an ongoing discussion in the PR #3273 . Once we add tests, I assume we can merge it. we can merge it.