surrealdb / surrealdb.go

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

Handle slice input with `SmartUnmarshal` #78

Open rytswd opened 1 year ago

rytswd commented 1 year ago

While SmartUnmarshal is useful with its generics support, it needs an explicit handling for handling a slice input. As we can check further into the unmarshalled data in RawQuery, it would be great to add some handling for complex data structure. This comes with a bit of cost with potential recurses, and thus it is probably preferable to keep the existing logic, and add a new function instead.

phughk commented 1 year ago

Hey @rytswd , could you perhaps provide an example of how you think this could work? Do you have example data structures? Is this about ordering of keys? Does slice work for Unmarshal instead of SmartUnmarshal?

rytswd commented 1 year ago

Hi @phughk! Sure, for the data structure, do you mean what return type we should use? In my code in the PR, I simply defined a new func SmartUnmarshalAll[T] to return []T instead of T. (I also added a note how one could call with slice type such as SmartUnmarshal[[]User]( ... ), but this looks quite foreign to me...)

For the example, I actually shared my finding in a comment in the above PR:

    data, err = db.Query(`
        CREATE user:j SET name = "J";
        CREATE user:j SET name = "J"; // conflict
        CREATE user SET name = "John";
        `, nil)
    if err != nil {
        t.Fatal(err) // Not hit
    }
    y, err := surrealdb.SmartUnmarshal[[]User](data, nil)
    t.Logf("old data: %+v", y)   // old data: [{Username: Password: ID:user:j Name:J}]
    t.Logf("old err:  %+v", err) // old err:  <nil>

    x, err := surrealdb.SmartUnmarshalAll[User](data)
    t.Logf("new data: %+v", x)   // new data: [{Username: Password: ID:user:j Name:J} {Username: Password: ID:user:3owq0tv8kn0aiz1rc6od Name:John}]
    t.Logf("new err:  %+v", err) // new err: ERR: Database record `user:j` already exists

Because the RawQuery can be nested, the generic handling needs to handle RawQuery as any type, and recursively unmarshal. This could potentially cause degraded performance, and Unmarshal may be a better fit. But as there is already SmartUnmarshal in place, I'd think it should at least treat the errors and data write path fully.

It is arguable what the idiomatic Go implementation should look like with generics, but having to specify []User rather than User for the type input seems unnatural to me.

This is potentially an area where godoc clarification may be sufficient, as long as the SmartUnmarshal can handle bulk input with full details of all items written and any errors hit.

Hope this makes sense, and if there is any more clarifications needed, I'd be happy to share!

phughk commented 9 months ago

It's definitely a good idea, but I am wondering if we would rather use Row and Column from the upcoming changes? It might result in a simpler solution to use, and reduce the number of options for deserialising