stretchr / objx

Go package for dealing with maps, slices, JSON and other data.
MIT License
687 stars 75 forks source link

Slices not being handled correctly... #125

Closed water-a closed 1 year ago

water-a commented 1 year ago
package main

import (
    "github.com/stretchr/objx"
    "log"
    "reflect"
)

func main() {
    m := objx.MustFromJSON(`{"a":{"b":["hello"]}}`)
    v := m.Get("a.b")

    log.Println(reflect.TypeOf(v.Data())) // []interface{}
    log.Println(v.IsStrSlice()) // false
    log.Println(v.StringSlice()) 
    // got: empty string slice: []string{}
    // expected: []string{"hello"}
}

Go version: go version go1.19 darwin/arm64

Simple example where I'm trying to retrieve the array "a.b" and retrieve it as a string slice, but it returns an empty string array

geseq commented 1 year ago

You're better off doing

log.Println(v.IsInterSlice())
log.Println(v.InterSlice())

for _, v := range v.InterSlice() {
    log.Println(v.(string))
}
water-a commented 1 year ago

When should you use .StringSlice() then?

water-a commented 1 year ago

Feels like maybe .StringSlice() could maybe add a check to see if it's an InterSlice and then iterating through it to do that type casting

e.g.

case v.IsInterSlice():
        slice := v.MustInterSlice()
        vals := make([]string, len(slice))
        for i, iv := range slice {
            if val, ok := iv.(string); ok {
                 vals[i] = val
            }
        }
        return vals

This should be added to the other types of slices as well...

water-a commented 1 year ago

If you like this idea, I can create a PR for it!

geseq commented 1 year ago

when you're using objx.Map directly. JSON unmarshal will be an []interface{}

geseq commented 1 year ago

happy to have a PR. please make sure to add tests with both string and non string type slices

geseq commented 1 year ago

the one problem I can see is that IsStrSlice might get expensive for large slices. Also there shouldn't be any loss of values, so you should be converting the values to a string rather than type casting and ignoring non-strings.

water-a commented 1 year ago

Well I wasn't going to modify .IsStrSlice, just .StringSlice. My thinking .StringSlice already coerces int slices into strings, why not an interface slice into strings. .IsStrSlice should still strictly be checking for []string type.

water-a commented 1 year ago

Though I question the behavior regarding defaults...like if it's an interface slice, do we want to try to coerce it or return the default...? Should it return the default if it's empty?

geseq commented 1 year ago

Even with just StringSlice, a JSON array can contain objects which might not be string convertible. StringSlice is designed for objects that are fully convertible to a []string without loss of values

water-a commented 1 year ago

Ah, ok. That makes sense. I'll close this issue then!