Feature: better error return

[agufagit]

Is your feature request related to a problem?

most of errors are embedded in the returned json, not actually returned as error type

there isn't clear documentation on how to parse errors from returned json

if a transaction fails, I'd like to rollback something, but not going to rollback for other errors (like parse errors)

Describe the solution

parse returned json in driver, return error type if there is actually an error

Alternative methods

n/a

SurrealDB version

1.0.0-beta.9+20230402.5eafebd for linux on x86_64

Contact Details

No response

Is there an existing issue for this?

• [X] I have searched the existing issues

Code of Conduct

• [X] I agree to follow this project's Code of Conduct

[ElecTwix]

there isn't clear documentation on how to parse errors from returned json

Current methods check for errors. If we are talking about RawQuery we had a function to parse. It also improves with #90 but has not been released yet. for now, you can use unmarshalraw

// a simple user struct
type testUser struct {
    Username string `json:"username,omitempty"`
    Password string `json:"password,omitempty"`
    ID       string `json:"id,omitempty"`
}

username := "johnny"
password := "123"

// create test user with raw SurrealQL and unmarshal
userData, err := db.Query("create users:johnny set Username = $user, Password = $pass", map[string]interface{}{
    "user": username,
    "pass": password,
})

var userSlice []testUser
ok, err := surrealdb.UnmarshalRaw(userData, &userSlice)

[agufagit]

UnmarshalRaw is the parsing step, which means parsing error will also trigger it, with this code, I need to call rollback twice

cloudFiles := uploadFilesToCloud(files)

userData, err := db.Query("update $id set files=$files", map[string]interface{}{
    "id":  id,
    "files": cloudFiles,
})
if (err != nil) {
  deleteFilesFromCloud(cloudFiles);
}

var userSlice []testUser
ok, err := surrealdb.UnmarshalRaw(userData, &userSlice)
if (err != nil) {
  deleteFilesFromCloud(cloudFiles);
}

If database transaction is success, and a parse error or any error is throwed at UnmarshalRaw step, it gives inconsistent state, with record committed in database and files deleted in cloud. So I just want to call rollback once if transaction failed

userData, err := db.Query("update $id set files=$files", map[string]interface{}{
    "id":  id,
    "files": files,
})
if (err != nil) {
  deleteFilesFromCloud();
}

[ElecTwix]

is the parsing step, which means parsing error will also trigger it, with this code, I need to call rollback twice

the first error that db.Query return for websocket read status for network errors.

parsing will return rpc.respond's error and that error is returned by the database

Source: https://github.com/surrealdb/surrealdb.go/blob/main/db.go#L119

---

it gives inconsistent state, with record committed in database and files deleted in cloud.

if the Query failed it will return an error when parsing the response.

---

I will also recommend checking transactions doc

[agufagit]

I'm using SmartUnmarshal, but the code from UnmarshalRaw, all those are parse errors, which I don't want to rollback

// UnmarshalRaw loads a raw SurrealQL response returned by Query into a struct. Queries that return with results will
// return ok = true, and queries that return with no results will return ok = false.
func UnmarshalRaw(rawData, v interface{}) (ok bool, err error) {
    var data []interface{}
    if data, ok = rawData.([]interface{}); !ok {
        return false, fmt.Errorf("failed raw unmarshaling to interface slice: %w", InvalidResponse)
    }

    var responseObj map[string]interface{}
    if responseObj, ok = data[0].(map[string]interface{}); !ok {
        return false, fmt.Errorf("failed mapping to response object: %w", InvalidResponse)
    }

    var status string
    if status, ok = responseObj["status"].(string); !ok {
        return false, fmt.Errorf("failed retrieving status: %w", InvalidResponse)
    }
    if status != statusOK {
        return false, fmt.Errorf("status was not ok: %w", ErrQuery)
    }

    result := responseObj["result"]
    if len(result.([]interface{})) == 0 {
        return false, nil
    }
    err = Unmarshal(result, v)
    if err != nil {
        return false, fmt.Errorf("failed to unmarshal: %w", err)
    }

    return true, nil
}

and the final error returned is from err = json.Unmarshal(bytes, &raw);, which can also be parse errors. If you are sure that all these errors are only caused by a failed database transaction, then I'm fine with it. But I can think of one case now that throw these errors, an updated database with return data format that's incompatible with current version of driver, but transaction is committed.

[ElecTwix]

I'm using SmartUnmarshal, but the code from UnmarshalRaw, all those are parse errors, which I don't want to rollback

This is for checking the rpc response integrity for this parse error. It is checking if that received can parse if it cannot it already cannot tell if it is committed or not.

also, I don't think it will happen if your connection didn't down for X>2 minutes more it will be fine websocket is built on top of TCP that grantees send data and check the length.

RFC 6455

Still understand your concerns I will create PR for UnmarshalRaw for returning some error types for checking the what is error

and the final error returned is from err = json.Unmarshal(bytes, &raw);, which can also be parse errors. If you are sure that all these errors are only caused by a failed database transaction, then I'm fine with it. But I can think of one case now that throw these errors, an updated database with return data format that's incompatible with current version of driver, but transaction is committed.

for now, you can parse with RawQuery but I recommend waiting for an update I will create PR in 1-2 days and I think it will merge around 1-2 weeks.

Thanks for bringing this up.

[agufagit]

awesome👍

[agufagit]

It actually would be very nice if you guys would consider to return database/network errors in the first step only. That would mean 1 more deserialization step json.Unmarshal, you could consider to use high performance jsoniter, it's not maintained anymore, but encoding/json itself hasn't been updated for years.

It not only makes rollback problem go away, but also makes code a lot shorter, so instead

returnRaw, err := dbCommon.Query(c, `UPDATE $contactId SET display=$display RETURN true`, map[string]any{
  "contactId": id,
  "display":   display,
})
if err != nil {
  return nil, errors.Wrap(err, "[UpdateContactDisplay] database error")
}

type ReturnData struct {
  True bool `json:"true"`
}

returnData, err := surrealdb.SmartUnmarshal[[]ReturnData](returnRaw, nil)
if err != nil {
  return returnUpdated, errors.Wrap(err, "[UpdateContactDisplay] can't unmarshal query return data")
}

I can just do

_, err := dbCommon.Query(c, `UPDATE $contactId SET display=$display`, map[string]any{
  "contactId": id,
  "display":   display,
})
if err != nil {
  return nil, errors.Wrap(err, "[UpdateContactDisplay] database error")
}

[ElecTwix]

It actually would be very nice if you guys would consider to return database/network errors in the first step only. That would mean 1 more deserialization step json.Unmarshal, you could consider to use high performance jsoniter, it's not maintained anymore, but encoding/json itself hasn't been updated for years.

I don't think we can eliminate the error because that can happen but we can use json/marshaling dependency injection for use other libs, I have 1-2 PR active right now and when they finish I will create the PR with the marshal in mind too.

I will link the issue when I create PR so you will know when it will be created.

[phughk]

We intend to move to errors that have better classification. Since the protocol uses strings instead of error codes at the moment, we will require regex in the implementation, and translation of error strings to get better matching in error handling.