Returning interface{}/any on most calls is not very flexible

[Keitio]

My idea would be to return some sort of raw value that can be unmarshalled to anything (thus removing the need for the strange Unmarshal top-level function)

type RawValue struct { ... }
func (rv RawValue) Unmarshal(target any) { /* use the raw value internals to unmarshal */ }
func Select(what string) (RawValue, error) {...}

I don't know if this idea is good, as most libraries use a method param to unmarshal into, à la

func Select(what string, target any) (RawValue, error) { /* directly unmarshal into target */ }

The advantage of the first option would be that we can add other methods, for example

func (rv RawValue) Subvalue(target any, keys ...string) // Subvalue(v, "user", "name") would unmarshall user.name to v

I can do the implementation of both, but am very open to any alternative as those do no strike me as very good

[MrShutCo]

If we want to use generics (locking us into go1.18+) we could have a cleaner solution something like this

type Response[T any] struct {
    Time   string      `json:"time"`
    Status string      `json:"status"`
    Result []T          `json:"result"`
}

and then the functions like Select just directly unmarshal it using that data type

[Keitio]

This is a good option, but sadly as of go1.19 we can't have generic methods (only on the types over which the receiver is generic) so the function returning the Result would need to be top-level like

surrealdb.Select[User](ctx, db, "* from user")

and I find that less useful and organic than a simple db.Select(ctx, "* from user") Sadly i don't know any library using generics like that today so i don't know how viable this is as an API

Maybe this could exist as a replacement for the top-level Unmarshal ? We return some kind of raw result (stored as []byte ?) that has some basic methods, and a top-level UnmarshalAs[T any](raw) This is a bit more cumbersome though

[garrison-henkle]

My original branch I forked from #3 converted the Unmarshal (and RawUnmarshal that I added) to use generics so I could have syntax similar to the code in @Keitio's comment above, but I switched to any for compatibility reasons. I think compatibility should probably be prioritized for a project like this, as it would help with adoption of Surreal.

I am currently working on the issue of multiple marshals / unmarshals mentioned in #17 and may be able to add a way to do this with any while I'm changing everything around. I am currently returning []byte from everything, so I just need to modify each of the functions to add an additional any parameter and call the updated unmarshal functions that I am working on. I can make an additional comment with the branch when I finish in a few hours.

[MrShutCo]

If we want compatability then we should do interface{} i think. any is a go1.18 generic thing and is just an alias

[garrison-henkle]

This isn't really a great implementation, but I got something that works: https://github.com/garrison-henkle/surrealdb.go/tree/fix_unmarshaling_performance

The functions now work like this:

type testUser struct {
    ID string `json:"id"`
    Name string `json:"name"`
}

user := testUser{
    Name: "garrison",
}

var response testUser
ok, err := db.Create("users", &user, &response)
if err != nil{
    panic(err)
}
if !ok{
    panic("empty result from database")
}

fmt.Println(response.Name) //prints "garrison"

I added an ok boolean return to all of the database methods to address no result responses, so it is pretty clunky to use due to double checks after every call. It also uses really simple means of parsing the json strings, such as hard coded indices and looking for certain characters. It's better than marshalling then unmarshalling, but would likely need to be changed to something more robust for something like this to be merged in. I also didn't add any safe guards, so the users would need to know to use a slice if more than one result is possible. Generics provides an easy fix for the first two issues honestly, so it might be nice to have two separate forms of the driver: one with clunky interface{} functions and one with generics.

If we want compatability then we should do interface{} i think. any is a go1.18 generic thing and is just an alias

Sorry, I meant interface{} instead of any. I probably should not have used them interchangably in a comment explicitly talking about backwards compatibility 😆. I've been using interface{} in my changes to this repo since one of the early PRs that swapped out all the anys.

[Keitio]

for the (ok, err) problem, you should just return an error, with some ErrNoResult we could check against, like we do for io.EOF But we need to have better error handling in most places anyway

Using another parameter to unmarshal into is nice, but locks us into non-generics sadly, and generics migration would be breaking or clunky For now, i still think that the raw result return, as strange as it is, is the more flexible option as it would allow both generic and non generic behaviors MongoDB's driver does something similar for some calls so it's not entirely unheard of.

[iDevelopThings]

So, since this ties into performance partially, I'm going to make a new issue and show an idea i had, which also ties into a way to avoid the interface{}/any responses and such, we can all discuss the performance/json unmarshalling side there

[garrison-henkle]

@Keitio mentioned Mongo, so I tried to clean up my changes and mimic a bit of its driver code to see what it would look like. I ended up with something like this:

var users []testUser

//normal method
ok, err = db.Select("testUser").Unmarshal(&users)

//query
ok, err = db.Query("select * from testUser", nil).UnmarshalRaw(&users)

I added two structs that return from the send function in db.go to encapsulate the data better and allow for the unmarshal functions to be methods:

//returned by normal methods
type SurrealWSResult struct {
    Result []byte
    Single bool
    Error  error
}

//returned by query
type SurrealWSRawResult struct {
    Result []byte
    Error  error
}

I actually like this a lot better than the previous attempts I've made because it inlines the unmarshaling. It also prevents use of the wrong unmarshal function (ie using the raw function for Select or the normal one for Query).

The branch with all my changes is here if anyone wants to browse through them or suggest changes.

for the (ok, err) problem, you should just return an error, with some ErrNoResult we could check against, like we do for io.EOF

Maybe this is because I don't have much experience in Go (mostly JVM background), but should a no result response really be an error? If I send a raw query using Query that returns no results, the status attached to that no result response will be "status"="OK". It just feels wrong to me to treat a successful response as an error, but, again, I don't know enough about Go style to know any better.

[Keitio]

Basically for empty result, it depends. For a SelectAll kind of method, we should just return an empty error For a SelectOne that is empty, we should return an error

But either way, returning a bool and an error is bad practice, because it should be an error (on select 1) or a non-event (on select all). If you want to check for empty result, you can just directly do if len(result) == 0 {...}

I'm all for this API though, I would just rename UnmarshalRaw to just Unmarshal, to be more consistent with the other Result type. Maybe the internal implementation could be a bit different bu i like the API

[garrison-henkle]

Yeah that's fair. I can switch the bool out for an error and make that signature change when I'm cleaning up the code later.