ohler55 / ojg

Optimized JSON for Go
MIT License
857 stars 49 forks source link

support for "json" tag in struct elements #133

Closed thiagodpf closed 1 year ago

thiagodpf commented 1 year ago

Hello Mr Ohler! I would like to suggest an adjustment in the jp/get.go file, in the reflectGetChild() function.

This function does not take into account the name of the element (alias) defined in the "json" tag of the struct that is passed to it.

We have structs that are populated from an json.Unmarshal(), but the structs have attributes whose name is a little different from the original names contained in the json. Then we would like to rely on the json attr names instead of the internal name defined in the struct. To do this, the lib would need to look for the "correct" name in the struct tags when necessary...

I would like to suggest the following improvement:

func (x Expr) reflectGetChild(data any, key string) (v any, has bool) {
    if !isNil(data) {
        rd := reflect.ValueOf(data)
        rt := rd.Type()
        if rt.Kind() == reflect.Ptr {
            rt = rt.Elem()
            rd = rd.Elem()
        }
        switch rt.Kind() {
        case reflect.Struct:

            // modified code START
            rv := rd.FieldByNameFunc(func(k string) bool { 
                if strings.EqualFold(k, key) {
                    return true
                }

                f, _ := rt.FieldByName(k)
                tagValue := f.Tag.Get("json")
                jsonKey, _, _ := strings.Cut(tagValue, ",")
                jsonKey = strings.Trim(jsonKey, " ")

                return strings.EqualFold(jsonKey, key)
            })
            // modified code END

            if rv.IsValid() && rv.CanInterface() {
                v = rv.Interface()
                has = true
            }
        case reflect.Map:
            rv := rd.MapIndex(reflect.ValueOf(key))
            if rv.IsValid() && rv.CanInterface() {
                v = rv.Interface()
                has = true
            }
        }
    }
    return
}

So that the above change passes the following unit test:

func TestGetChildReflectByJsonTag(t *testing.T) {
    type Element struct {
        Value string
    }
    type Root struct {
        Elements    []Element
        AltElements []Element `json:"anyOtherAttributeName"`
    }
    data := Root{
        Elements: []Element{
            {Value: "e1"},
            {Value: "e2"},
            {Value: "e3"},
        },
        AltElements: []Element{
            {Value: "e4"},
            {Value: "e5"},
            {Value: "e6"},
        },
    }
    path := jp.MustParseString("$.elements[*]")
    tt.Equal(t, "[{value: e1} {value: e2} {value: e3}]", pretty.SEN(path.Get(data)))
    tt.Equal(t, "{value: e1}", pretty.SEN(path.First(data)))

    path = jp.MustParseString("$.elements[*].value")
    tt.Equal(t, "[e1 e2 e3]", pretty.SEN(path.Get(data)))
    tt.Equal(t, "e1", pretty.SEN(path.First(data)))

    path = jp.MustParseString("$.altElements[*]")
    tt.Equal(t, "[{value: e4} {value: e5} {value: e6}]", pretty.SEN(path.Get(data)))
    tt.Equal(t, "{value: e4}", pretty.SEN(path.First(data)))

    path = jp.MustParseString("$.altElements[*].value")
    tt.Equal(t, "[e4 e5 e6]", pretty.SEN(path.Get(data)))
    tt.Equal(t, "e4", pretty.SEN(path.First(data)))

    // Get by "json" tag
    path = jp.MustParseString("$.anyOtherAttributeName[*]")
    tt.Equal(t, "[{value: e4} {value: e5} {value: e6}]", pretty.SEN(path.Get(data)))
    tt.Equal(t, "{value: e4}", pretty.SEN(path.First(data)))

    path = jp.MustParseString("$.anyOtherAttributeName[*].value")
    tt.Equal(t, "[e4 e5 e6]", pretty.SEN(path.Get(data)))
    tt.Equal(t, "e4", pretty.SEN(path.First(data)))

    // a non-existent attribute in the struct and also in the json (which populated the struct) 
    // would still be non-existent as expected
    path = jp.MustParseString("$.nonExistentAttributeName[*].value")
    tt.Equal(t, "[]", pretty.SEN(path.Get(data)))
}

I can't say if there is a more performant way to code this adjustment, but I kept the change simple to better exemplify the problem.

does it make sense to you?

ohler55 commented 1 year ago

I had not anticipated a need to look up struct members by json tag names as the path lookup has nothing to do with json at that stage but it does seem like a nice addition. Let me take a look at what is most optimum. If what you have is performant enough would you like to create an MR for the addition or do you feel more comfortable if I make the enhancement?

thiagodpf commented 1 year ago

I created the MR above with an adaptation of the original function from the reflect package. Basically what I did was open the match function to receive a reflect.StructField instead of just the field name as string.

I think this adaptation performs better than the one I suggested at the beginning. If you find it interesting and want to make adjustments to better fit your rationale, feel free. I don't care about having my name as a contributor, I just want to help you not have to waste a lot of time on it. You can cancel my MR if you prefer. :satisfied: :thumbsup:

ohler55 commented 1 year ago

Your MR was merged and a new release has been made, v1.18.6.

thiagodpf commented 1 year ago

I am very grateful for accepting this request, it will help me a lot and I hope it will help others too! Thank you very much!