surrealdb / surrealdb.go

SurrealDB SDK for Golang
https://surrealdb.com
Apache License 2.0
231 stars 62 forks source link

SmartMarshal Added. #48

Closed ElecTwix closed 1 year ago

ElecTwix commented 1 year ago

Basemodel struct added. Unit Tests added for SmartMarshal.

ElecTwix commented 1 year ago

Like #42 ,Its add

but the thing mostly sees until they use the library you need to give func to a table name for everything. like most ORMs, they have BaseModel that has a struct tag for a table of names it will allow something easier to use generics such as.

Ex.

// Now

type User struct {
    Username            string
    Password            string
    ID                  string
}

type Ship struct {
    Speed               int
    Cargo               int
    ID                  string
}

// Needs multiple methods or create an interface for get ply.ID
func (ply *User) Insert() (err error) {
    ply, err = surrealdb.SmartUnmarshal[*User](db.Globaldb.Create(PlayerTable+ply.ID, ply))
    return
}

func (thing *Test[W, I]) Insert() (err error) {
    thing, err = surrealdb.SmartUnmarshal[*testUser](db.Globaldb.Create(TestTable+thing.ID, ply))
    return
}
// With PR

type User struct {
    surrealdb.Basemodel `table:"users"`
    Username            string
    Password            string
    ID                  string
}

type Ship struct {
    surrealdb.Basemodel `table:"ships"`
    Speed               int
    Cargo               int
    ID                  string
}

// This Can be Used with Everything like Both IPlayer & Test
func Insert[I any](thing I) (err error) {
    thing, err = surrealdb.SmartUnmarshal[I](surrealdb.SmartMarshal(db.Create, thing))
    return
}
ElecTwix commented 1 year ago

Added ErrNoRow. Needed for Checking if Row Exists or Not.

phughk commented 1 year ago

Had a brief look and mostly approve except for this issue https://github.com/surrealdb/surrealdb.go/pull/48/files#r1143356765 This is a note to self or others reviewing, not quite yet an approval (for example, do we want error handling done that way, do we want to have such an interface at all etc)

ElecTwix commented 1 year ago

Had a brief look and mostly approved except for this issue https://github.com/surrealdb/surrealdb.go/pull/48/files#r1143356765 This is a note to self or others reviewing, not quite yet an approval (for example, do we want error handling done that way, do we want to have such an interface at all etc)

Yeah, I think we need to talk about this too. Firstly I would like to explain why I write this in this way.

I had a mind to creating ORM like a system that can define table names with struct tag and if there is an ID it will override the table name. I wanted to create a func for works with all db methods but there was no way to achieve that with existing methods and without passing func as a parameter and executing func inside the SmartMarshal.

For Summary, what I wanted to achieve

I share my personal experience with lib about this topic,

I had ~15 Struct that has Get Update Insert and Delete Methods Some of them use ID as their Record some of them just use table names to generate the unique IDs. Like SmartUnMarshal or UnMarshal on the lib, I wanted some struct support with ORM features to use easily and effectively.

Such as SmartUnMarshal and Unmarshal already give record id as id when you unmarshal so why we arent use some marshaling to do the same with struct tags or id?

For ErrNoRow, this was a huge bottleneck for a project, you don't know if a row exists or not, such as there is no error when you try to select the not existing row.

it just gives errors when you try to unmarshal, and even that gives a marshaling error, so we can not determine is really no row or if some data is corrupted, we need some shape of the error to determine if there is no row in DB.

right now, if we try to do this we could but DB is already checking whether is response is array or not, we can, we are doing the same thing for the second time for no reason, also it will affect when we have 1k+ requests every second we could just do it once in DB and pass the error to the end user to handle whatever they want.

phughk commented 1 year ago

My take is that we already have a lot of tech debt in various drivers. Long term we will want a coherent plan for this (API, functionality, scope etc). Until then, happy to grab whatever code helps users. Thanks @ElecTwix ! Merging

Empera0 commented 1 year ago

lgtm but why are we returning message for non existing row?

we can return false or another parameter. If we do as i said developer can do adding row or other process for that. it will help for the automation.

phughk commented 1 year ago

@Empera0 do you think you could add a test and change to demonstrate this please? If you have the time of course. Will merge that as well, as from what I understand I agree