Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Additional CBOR types #160

Merged
merged 6 commits into from
Oct 22, 2024
Merged

Additional CBOR types #160

merged 6 commits into from
Oct 22, 2024

Conversation

remade
Copy link
Collaborator

@remade remade commented Oct 17, 2024

  • Add Custom types with CBOR marshaling/unmarshaling
  • Test and fix CBOR inconsistencies
  • Add test

@remade remade added the wip label Oct 17, 2024
@agufagit
Copy link

Hi,

the returning result is &[{Status:OK Time:28.641008ms Result:map[id:{Table:user ID:ls6bchgtj8l721a8dyeu}]}],
can you make returning result &[{Status:OK Time:28.641008ms Result:map[id:user:ls6bchgtj8l721a8dyeu]}]? Which is compatible with old driver, and I don't see the use to make an ID an object, it's supposed to be a string.

Also I'm getting another error "returnRawError":"json: unsupported type: map[interface {}]interface {}".

For Query function, the return type doesn't need to be a pointer, it can be []QueryResult[TResult], golang is different from rust, slices already contain a pointer, and are designed to be passed "as is" for data locality

@kearfy
Copy link
Member

kearfy commented Oct 18, 2024

Hey @agufagit, for official SurrealDB drivers reaching compatibility with SurrealDB 2.0.0, it is actually expected that values such as Record IDs, datetimes, durations, etc, are no longer represented in a string-like format, but rather in a native manner.

Taking record ids as an example, the id parts such as objects and arrays can contain information which the developer might use in it's implementation, which would be difficult to retrieve when all these values are represented in a string-like format. To decode these, you would need to implement the entire surrealql parser, as array and object record ids can contain any surrealql value 😅

All SDKs should implement a stringification method however, and have a way to send record ids as a string. (a StringRecordId class in the JS SDK as an example). I'm not up to speed with the Golang SDK, but if this is currently not the case, feel free to open an issue for that.

Thank you!

@remade
Copy link
Collaborator Author

remade commented Oct 18, 2024

Hi @agufagit

With CBOR implemented, all responses are CBOR encoded including the record ID. This is to our advantage. As @kearfy mentioned, there is a String() method on the RecordID type you can use to retrieve the familiar representation. Also, there are helper methods to create/parse record IDs and other custom types. As part of this PR, I will make sure all types have String() and SurrealString() methods

@agufagit
Copy link

Hi,

I hope this doesn't mean I have to loop every record to call id.String(), dateTime.toTime()?

Suppose I have a struct

type Data struct {
  ID string `json:"id"`
  Date time.Time `json:"date"`
}

will driver be smart enough to automatically convert id to string, date to time.Time? Or we have to do it ourselves?

@remade
Copy link
Collaborator Author

remade commented Oct 19, 2024

@agufagit

Whenever you are retrieving the ID, when you retrieve the ID, you can simply call the string method

d.ID // for RecordID
d.ID.String() // for string representation

@agufagit
Copy link

that's what I want to avoid to do, my app is fairly complex, that would mean too much work. I'm using auto gen graphql, where ID is string, I'll have to redefine every struct and map it to graphql struct instead of just using auto generated graphql struct

but no biggie, once you are done, I'll just fork the repo and make changes, and use that forked repo instead

@remade
Copy link
Collaborator Author

remade commented Oct 21, 2024

@agufagit
Have you tried it out? When a string value is expected of any type, as far as it implements the Stringer interface, all should be well. You don't need to do anything extra. If you experience otherwise, it definitely worth resolving

@agufagit
Copy link

I didn't try the new commit yet, but that sounds great, I'll try it after you're done and new release is out for v2.0.0, thank you.

@remade remade removed the wip label Oct 22, 2024
@remade remade merged commit f98f67e into main Oct 22, 2024
2 checks passed
@remade remade deleted the additional-cbor-types branch October 25, 2024 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants