ugorji / go

idiomatic codec and rpc lib for msgpack, cbor, json, etc. msgpack.org[Go]
MIT License
1.86k stars 295 forks source link

No possible to register encoder extensions for time.Time #368

Closed HEskandari closed 2 years ago

HEskandari commented 2 years ago

When registering an extension to encoder time, as advertised the encoder won't get picked up, regardless of the TimeNotBuiltin flag.

    writer := bytes.Buffer{}
    var json codec.JsonHandle
    json.TimeNotBuiltin = true //won't run for both true and false
    err := json.SetInterfaceExt(timeType, 1, NewTimeMarshaller()) //implements InterfaceExt

Unlike advertised, there is a hidden check that silently ignores extensions registered for time.Time. See the below code (line 947 in helper.go). I think a check for json.TimeNotBuiltin flag is missing.

func (x *basicHandleRuntimeState) setExt(rt reflect.Type, tag uint64, ext Ext) (err error) {
    rk := rt.Kind()
    for rk == reflect.Ptr {
        rt = rt.Elem()
        rk = rt.Kind()
    }

    if rt.PkgPath() == "" || rk == reflect.Interface { // || rk == reflect.Ptr {
        return fmt.Errorf("codec.Handle.SetExt: Takes named type, not a pointer or interface: %v", rt)
    }

    rtid := rt2id(rt)
    switch rtid {
    case timeTypId, rawTypId, rawExtTypId:
        // these are all natively supported type, so they cannot have an extension.
        // However, we do not return an error for these, as we do not document that.
        // Instead, we silently treat as a no-op, and return.
        return
    }

This should be changed to:

func (x *basicHandleRuntimeState) setExt(rt reflect.Type, tag uint64, ext Ext) (err error) {
         ...
    switch rtid {
    case rawTypId, rawExtTypId:
        // these are all natively supported type, so they cannot have an extension.
        // However, we do not return an error for these, as we do not document that.
        // Instead, we silently treat as a no-op, and return.
        return
    case timeTypId:
               if x.timeBuiltin {
                  //ignore time as it is supported internally
                  return
               }
    }