sas1024 / gorm-loggable

Loggable plugin for GORM
MIT License
28 stars 22 forks source link

[ISSUE] reflect: NumField of non-struct type #18

Open vzool opened 5 years ago

vzool commented 5 years ago

Hi,

I have to use your package with the package hide.ID and I have an error with Update and Delete. Here is the full error report: I use SQLite driver

2019/09/24 14:39:23 [Recovery] 2019/09/24 - 14:39:23 panic recovered:
PUT /api/v1/country/lk4KbkPpG HTTP/1.1
Host: localhost:8080
Accept: application/json, text/plain, */*
Accept-Encoding: gzip, deflate, br
Accept-Language: en-US,en;q=0.9,ar;q=0.8
Authorization: *
Connection: keep-alive
Content-Length: 29
Content-Type: application/json;charset=UTF-8
Origin: http://localhost:3000
Referer: http://localhost:3000/
Sec-Fetch-Mode: cors
Sec-Fetch-Site: same-site
User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/77.0.3865.90 Safari/537.36

reflect: NumField of non-struct type
/usr/lib64/go/1.11/src/runtime/panic.go:513 (0x4333d8)
    gopanic: reflectcall(nil, unsafe.Pointer(d.fn), deferArgs(d), uint32(d.siz), uint32(d.siz))
/home/vzool/go/pkg/mod/github.com/jinzhu/gorm@v1.9.10/scope.go:884 (0x989542)
    (*Scope).callCallbacks.func1: panic(err)
/usr/lib64/go/1.11/src/runtime/asm_amd64.s:522 (0x45ebda)
    call32: CALLFN(·call32, 32)
/usr/lib64/go/1.11/src/runtime/panic.go:513 (0x4333d8)
    gopanic: reflectcall(nil, unsafe.Pointer(d.fn), deferArgs(d), uint32(d.siz), uint32(d.siz))
/usr/lib64/go/1.11/src/reflect/type.go:984 (0x4b4b4d)
    (*rtype).NumField: panic("reflect: NumField of non-struct type")
/home/vzool/go/pkg/mod/github.com/sas1024/gorm-loggable@v0.0.0-20190307192712-3ae87a295625/util.go:99 (0x997b32)
    getLoggableFieldNames: for i := 0; i < t.NumField(); i++ {
/home/vzool/go/pkg/mod/github.com/sas1024/gorm-loggable@v0.0.0-20190307192712-3ae87a295625/callbacks.go:141 (0x9954cb)
    computeUpdateDiff: names := getLoggableFieldNames(old)
/home/vzool/go/pkg/mod/github.com/sas1024/gorm-loggable@v0.0.0-20190307192712-3ae87a295625/callbacks.go:88 (0x994d81)
    addUpdateRecord: diff := computeUpdateDiff(scope)
/home/vzool/go/pkg/mod/github.com/sas1024/gorm-loggable@v0.0.0-20190307192712-3ae87a295625/callbacks.go:70 (0x994a27)
    (*Plugin).addUpdated: addUpdateRecord(scope, p.opts)
/home/vzool/go/pkg/mod/github.com/sas1024/gorm-loggable@v0.0.0-20190307192712-3ae87a295625/plugin.go:25 (0x998543)
    (*Plugin).addUpdated-fm: callback.Update().After("gorm:after_update").Register("loggable:update", p.addUpdated)
/home/vzool/go/pkg/mod/github.com/jinzhu/gorm@v1.9.10/scope.go:888 (0x974b47)
    (*Scope).callCallbacks: (*f)(scope)
/home/vzool/go/pkg/mod/github.com/jinzhu/gorm@v1.9.10/main.go:467 (0x9648bf)
    (*DB).Save: newDB := scope.callCallbacks(s.parent.callbacks.updates).db
/home/vzool/Workspace/seren/webapp/app/http/controllers/country.go:210 (0xbd0b81)
    CountryUpdate: if err := db.Save(&data).Error; err != nil {
/home/vzool/go/pkg/mod/github.com/gin-gonic/gin@v1.4.0/context.go:124 (0x8d4a09)
    (*Context).Next: c.handlers[c.index](c)
/home/vzool/Workspace/seren/webapp/app/http/middleware/auth.go:55 (0xbf5492)
    Auth.func1: ctx.Next()
/home/vzool/go/pkg/mod/github.com/gin-gonic/gin@v1.4.0/context.go:124 (0x8d4a09)
    (*Context).Next: c.handlers[c.index](c)
/home/vzool/Workspace/seren/webapp/app/http/middleware/hideID.go:32 (0xbf5ef1)
    HideID.func1: ctx.Next()
/home/vzool/go/pkg/mod/github.com/gin-gonic/gin@v1.4.0/context.go:124 (0x8d4a09)
    (*Context).Next: c.handlers[c.index](c)
/home/vzool/Workspace/seren/webapp/app/http/middleware/cors.go:26 (0xbf5b72)
    CORS.func1: c.Next()
/home/vzool/go/pkg/mod/github.com/gin-gonic/gin@v1.4.0/context.go:124 (0x8d4a09)
    (*Context).Next: c.handlers[c.index](c)
/home/vzool/go/pkg/mod/github.com/gin-contrib/gzip@v0.0.1/gzip.go:47 (0xbf155e)
    Gzip.func2: c.Next()
/home/vzool/go/pkg/mod/github.com/gin-gonic/gin@v1.4.0/context.go:124 (0x8d4a09)
    (*Context).Next: c.handlers[c.index](c)
/home/vzool/Workspace/seren/webapp/app/http/middleware/secure.go:42 (0xbf61bf)
    Secure.func1: ctx.Next()
/home/vzool/go/pkg/mod/github.com/gin-gonic/gin@v1.4.0/context.go:124 (0x8d4a09)
    (*Context).Next: c.handlers[c.index](c)
/home/vzool/go/pkg/mod/github.com/gin-gonic/gin@v1.4.0/recovery.go:83 (0x8e7c19)
    RecoveryWithWriter.func1: c.Next()
/home/vzool/go/pkg/mod/github.com/gin-gonic/gin@v1.4.0/context.go:124 (0x8d4a09)
    (*Context).Next: c.handlers[c.index](c)
/home/vzool/go/pkg/mod/github.com/gin-gonic/gin@v1.4.0/logger.go:240 (0x8e6d80)
    LoggerWithConfig.func1: c.Next()
/home/vzool/go/pkg/mod/github.com/gin-gonic/gin@v1.4.0/context.go:124 (0x8d4a09)
    (*Context).Next: c.handlers[c.index](c)
/home/vzool/go/pkg/mod/github.com/gin-gonic/gin@v1.4.0/gin.go:389 (0x8de0c4)
    (*Engine).handleHTTPRequest: c.Next()
/home/vzool/go/pkg/mod/github.com/gin-gonic/gin@v1.4.0/gin.go:351 (0x8dd8f1)
    (*Engine).ServeHTTP: engine.handleHTTPRequest(c)
/usr/lib64/go/1.11/src/net/http/server.go:2741 (0x70760a)
    serverHandler.ServeHTTP: handler.ServeHTTP(rw, req)
/usr/lib64/go/1.11/src/net/http/server.go:1847 (0x703855)
    (*conn).serve: serverHandler{c.server}.ServeHTTP(w, w.req)
/usr/lib64/go/1.11/src/runtime/asm_amd64.s:1333 (0x4608f0)
    goexit: BYTE    $0x90   // NOP

Any advice could help me out? Thanks

vzool commented 5 years ago

After some investigation I found that when I enabled loggable.ComputeDiff() in loggable.Register() the issue occurred and when I removed the error will go. So the main issue should be within loggable.ComputeDiff() function.

sas1024 commented 5 years ago

Hello @vzool. Can you show me the structure of data variable somewhere in /home/vzool/Workspace/seren/webapp/app/http/controllers/country.go:210 ?

vzool commented 5 years ago

Hi @sas1024, data is an instance of Country struct so here it is:


// BaseModel model
type BaseModel struct {
    ID hide.ID `gorm:"primary_key" json:"id,omitempty"`
}

// TrackingModel model
type TrackingModel struct {
    CreatedAt *time.Time `json:"created_at,omitempty" gorm:"NOT NULL"`
    UpdatedAt *time.Time `json:"updated_at,omitempty" gorm:"DEFAULT NULL"`
    DeletedAt *time.Time `sql:"index" json:"deleted_at,omitempty" gorm:"DEFAULT NULL"`

    CreatedBy hide.ID `json:"created_by,omitempty" gorm:"NOT NULL"`
    UpdatedBy hide.ID `json:"updated_by,omitempty" gorm:"DEFAULT NULL"`
    DeletedBy hide.ID `json:"deleted_by,omitempty" gorm:"DEFAULT NULL"`

    loggable.LoggableModel
}

// Country model
type Country struct {
    BaseModel

    Name string `json:"name,omitempty" form:"name" valid:"required" gorm:"unique_index;NOT NULL" gorm-loggable:"true"`
    Code int    `json:"code,omitempty" form:"code" gorm:"unique_index;NOT NULL" gorm-loggable:"true"`

    TrackingModel
}
vzool commented 5 years ago

After extra investigation I think that this line below inside callbacks/computeUpdateDiff():

old := im.get(scope.Value, scope.PrimaryKeyValue())

Does not return the old record to make Diff operation with new one.

sas1024 commented 5 years ago

I think you need to move loggable.LoggableModel from TrackingModel to Country.

vzool commented 5 years ago

This let me pass the error and fall into a new one:

reflect: call of reflect.Value.Field on ptr Value

And I can't share the complete logs because I changed a lot of things which become messy.

In fact, I did managed to pass first error with the following update:

func getLoggableFieldNames(value interface{}) []string {
    var names []string

    t := pointerType(reflect.TypeOf(value))

    for i := 0; i < t.NumField(); i++ {
        field := t.Field(i)
        value, ok := field.Tag.Lookup(loggableTag)
        if !ok || value != "true" {
            continue
        }

        names = append(names, field.Name)
    }

    fmt.Println("(Names): ", names)

    return names
}

// pointerType is made to chase for value through all the way following,
// leading pointers until reach the deep final value which is not a pointer
func pointerType(t reflect.Type) reflect.Type {
    for {
        if t.Kind() != reflect.Ptr {
            return t
        }
        t = t.Elem()
    }
}

So, I think it doesn't matter any more if I shift loggable.LoggableModel into upper level or leave it in the TrackingModel. Because the pointerType function will affects over all code and it behaves now more dynamic.

vzool commented 5 years ago

Okey I did reset the code and just add pointerType function with its call inside callbacks/getLoggableFieldNames() and produce the issue which gives me this:

2019/09/25 17:01:39 [Recovery] 2019/09/25 - 17:01:39 panic recovered:
PUT /api/v1/country/e0YPEGdBM HTTP/1.1
Host: localhost:8080
Accept: */*
Accept-Encoding: gzip, deflate, br
Accept-Language: en-US,en;q=0.9,ar;q=0.8
Authorization: *
Cache-Control: no-cache
Connection: keep-alive
Content-Length: 45
Content-Type: text/plain;charset=UTF-8
Origin: chrome-extension://fhbjgbiflinjbdggehcddcbncdddomop
Postman-Token: 8a460257-d870-b7f8-5ac6-ae1159a0c9ff
Sec-Fetch-Mode: cors
Sec-Fetch-Site: cross-site
User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/77.0.3865.90 Safari/537.36

reflect: call of reflect.Value.FieldByName on ptr Value
/usr/lib64/go/1.11/src/runtime/panic.go:513 (0x433358)
    gopanic: reflectcall(nil, unsafe.Pointer(d.fn), deferArgs(d), uint32(d.siz), uint32(d.siz))
/home/vzool/go/pkg/mod/github.com/jinzhu/gorm@v1.9.10/scope.go:884 (0x985422)
    (*Scope).callCallbacks.func1: panic(err)
/usr/lib64/go/1.11/src/runtime/asm_amd64.s:522 (0x45eb0a)
    call32: CALLFN(·call32, 32)
/usr/lib64/go/1.11/src/runtime/panic.go:513 (0x433358)
    gopanic: reflectcall(nil, unsafe.Pointer(d.fn), deferArgs(d), uint32(d.siz), uint32(d.siz))
/usr/lib64/go/1.11/src/reflect/value.go:207 (0x4bd2a3)
    flag.mustBe: panic(&ValueError{methodName(), f.kind()})
/usr/lib64/go/1.11/src/reflect/value.go:865 (0x4c017b)
    Value.FieldByName: v.mustBe(Struct)
/home/vzool/go/pkg/mod/github.com/sas1024/gorm-loggable@v0.0.0-20190307192712-3ae87a295625/callbacks.go:146 (0x99144f)
    computeUpdateDiff: ofv := ov.FieldByName(name).Interface()
/home/vzool/go/pkg/mod/github.com/sas1024/gorm-loggable@v0.0.0-20190307192712-3ae87a295625/callbacks.go:88 (0x990c61)
    addUpdateRecord: diff := computeUpdateDiff(scope)
/home/vzool/go/pkg/mod/github.com/sas1024/gorm-loggable@v0.0.0-20190307192712-3ae87a295625/callbacks.go:70 (0x990907)
    (*Plugin).addUpdated: addUpdateRecord(scope, p.opts)
/home/vzool/go/pkg/mod/github.com/sas1024/gorm-loggable@v0.0.0-20190307192712-3ae87a295625/plugin.go:25 (0x9944c3)
    (*Plugin).addUpdated-fm: callback.Update().After("gorm:after_update").Register("loggable:update", p.addUpdated)
/home/vzool/go/pkg/mod/github.com/jinzhu/gorm@v1.9.10/scope.go:888 (0x970a27)
    (*Scope).callCallbacks: (*f)(scope)
/home/vzool/go/pkg/mod/github.com/jinzhu/gorm@v1.9.10/main.go:467 (0x96079f)
    (*DB).Save: newDB := scope.callCallbacks(s.parent.callbacks.updates).db
/home/vzool/Workspace/seren/webapp/app/http/controllers/country.go:210 (0xb31f01)
    CountryUpdate: if err := db.Save(&data).Error; err != nil {
/home/vzool/go/pkg/mod/github.com/gin-gonic/gin@v1.4.0/context.go:124 (0x8d08e9)
    (*Context).Next: c.handlers[c.index](c)
/home/vzool/Workspace/seren/webapp/app/http/middleware/auth.go:55 (0xb562d2)
    Auth.func1: ctx.Next()
/home/vzool/go/pkg/mod/github.com/gin-gonic/gin@v1.4.0/context.go:124 (0x8d08e9)
    (*Context).Next: c.handlers[c.index](c)
/home/vzool/Workspace/seren/webapp/app/http/middleware/hideID.go:32 (0xb56d31)
    HideID.func1: ctx.Next()
/home/vzool/go/pkg/mod/github.com/gin-gonic/gin@v1.4.0/context.go:124 (0x8d08e9)
    (*Context).Next: c.handlers[c.index](c)
/home/vzool/Workspace/seren/webapp/app/http/middleware/cors.go:26 (0xb569b2)
    CORS.func1: c.Next()
/home/vzool/go/pkg/mod/github.com/gin-gonic/gin@v1.4.0/context.go:124 (0x8d08e9)
    (*Context).Next: c.handlers[c.index](c)
/home/vzool/go/pkg/mod/github.com/gin-contrib/gzip@v0.0.1/gzip.go:47 (0xb5239e)
    Gzip.func2: c.Next()
/home/vzool/go/pkg/mod/github.com/gin-gonic/gin@v1.4.0/context.go:124 (0x8d08e9)
    (*Context).Next: c.handlers[c.index](c)
/home/vzool/Workspace/seren/webapp/app/http/middleware/secure.go:42 (0xb56fff)
    Secure.func1: ctx.Next()
/home/vzool/go/pkg/mod/github.com/gin-gonic/gin@v1.4.0/context.go:124 (0x8d08e9)
    (*Context).Next: c.handlers[c.index](c)
/home/vzool/go/pkg/mod/github.com/gin-gonic/gin@v1.4.0/recovery.go:83 (0x8e3af9)
    RecoveryWithWriter.func1: c.Next()
/home/vzool/go/pkg/mod/github.com/gin-gonic/gin@v1.4.0/context.go:124 (0x8d08e9)
    (*Context).Next: c.handlers[c.index](c)
/home/vzool/go/pkg/mod/github.com/gin-gonic/gin@v1.4.0/logger.go:240 (0x8e2c60)
    LoggerWithConfig.func1: c.Next()
/home/vzool/go/pkg/mod/github.com/gin-gonic/gin@v1.4.0/context.go:124 (0x8d08e9)
    (*Context).Next: c.handlers[c.index](c)
/home/vzool/go/pkg/mod/github.com/gin-gonic/gin@v1.4.0/gin.go:389 (0x8d9fa4)
    (*Engine).handleHTTPRequest: c.Next()
/home/vzool/go/pkg/mod/github.com/gin-gonic/gin@v1.4.0/gin.go:351 (0x8d97d1)
    (*Engine).ServeHTTP: engine.handleHTTPRequest(c)
/usr/lib64/go/1.11/src/net/http/server.go:2741 (0x7034ea)
    serverHandler.ServeHTTP: handler.ServeHTTP(rw, req)
/usr/lib64/go/1.11/src/net/http/server.go:1847 (0x6ff735)
    (*conn).serve: serverHandler{c.server}.ServeHTTP(w, w.req)
/usr/lib64/go/1.11/src/runtime/asm_amd64.s:1333 (0x460820)
    goexit: BYTE    $0x90   // NOP
sas1024 commented 5 years ago

For now i'm not able to resolve this issue, because i'm not using this addon anymore.

@vcraescu can you help with this issue?

vetcher commented 5 years ago

@vzool Hi. I think, the main reason, why it panics, because you use pointers in Update method. Could you post minimal example, that panics on https://play.golang.org ? In current implementation, diff expects from model to be structure.

Also, as quick fix, I think, you may change code of identity_manager.go: Before:

func (im *identityManager) save(value, pk interface{}) {
    t := reflect.TypeOf(value)
    newValue := reflect.New(t).Interface()
    err := copier.Copy(&newValue, value)
    if err != nil {
        panic(err)
    }

    im.m[genIdentityKey(t, pk)] = newValue
}

After:

func (im *identityManager) save(value, pk interface{}) {
    t := reflect.Indirect(reflect.ValueOf(value)).Type()
    newValue := reflect.New(t).Interface()
    err := copier.Copy(&newValue, value)
    if err != nil {
        panic(err)
    }

    im.m[genIdentityKey(t, pk)] = newValue
}
vzool commented 5 years ago

@vetcher I'm not sure now, because I forked it locally and I did changed a lot of things for a degree that I can get rid of identityManager itself.