surrealdb / surrealdb.go

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

Added basic http client #7

Open MrShutCo opened 2 years ago

MrShutCo commented 2 years ago

Added a basic wrapper struct that allows http requests to the POST /sql endpoint, returning the response a string of the json. Also added the testify library for writing unit tests

MrShutCo commented 2 years ago

Still a WIP, need to fix up unit tests / make more robust as well as have some kind of unmarshaling of responses

MrShutCo commented 1 year ago

Could we use http instead of httpclient for the folder name?

@tobiemh I think this might be a bit confusing, as it conflicts with the standard golang http package, and having a http.Client might cause further confusion. What if I copy this code out to the root surrealdb package and change the struct to HttpClient or something, so that it becomes surrealdb.HttpClient or surrealdb.Client?

tobiemh commented 1 year ago

@tobiemh I think this might be a bit confusing, as it conflicts with the standard golang http package, and having a http.Client might cause further confusion. What if I copy this code out to the root surrealdb package and change the struct to HttpClient or something, so that it becomes surrealdb.HttpClient or surrealdb.Client?

Ahhh yes, of course - good point!

intabulas commented 1 year ago

A general comment if I may. Two things concern me, the language of xxxAll and the use of strings. My mental model is something along the lines of (using create for an example) Create() and CreateOne().

My second concern is the use of string as the value param. I really would love to see this as interface{} that is marshaled inside Request so we can pass structs vs marshaling before calling.

MrShutCo commented 1 year ago

@intabulas The first one is sorta outta my control, I was just using the functions from https://github.com/surrealdb/surrealdb.py/pull/5/files which has been merged already, as well as when going to the official docs for things like https://surrealdb.com/docs/integration/http#select-all it'll bring you to the query... Though I do admit its a bit strange.

I agree on the second point! String was just the easiest, and now that the PRs for marshalling are in, ill revisit this and see what I can do

intabulas commented 1 year ago

@MrShutCo gotcha, totally makes sense to follow the pattern then

MrShutCo commented 1 year ago

@intabulas @tobiemh The PR is ready for a proper review. I've put a bunch of TODO's as well for other devs to possibly improve on this code. It isn't perfect, but hopefully its enough of a base for people to improve on :)

tobiemh commented 1 year ago

Hi @MrShutCo just looking at this again, and thinking about the upcoming Rust library, what is the need to have an HTTP/REST specific functionality available to the end user?

With regards to the Rust library (and the latest changes to this Golang library), the HTTP/WebSocket implementations are hidden from the user.

What are your thoughts/reasoning?

MrShutCo commented 1 year ago

@tobiemh I was just following what was being done on other repos like the Python one. If we're not exposing the endpoints and Websockets directly then this PR has no real use, and I must have missed some of the discussion on how it would be used

tobiemh commented 1 year ago

Hey @MrShutCo ah no don't worry you haven't missed the discussion. I'm going to keep this PR open for the moment, but if we follow the Rust library (coming soon), then the HTTP endpoints will be used behind the scenes for import and export functionality. All other querying will happen over the WebSocket connection.

phughk commented 1 year ago

Heya! Checking out open changes - I think there is still some work to be done in this area, so can't quite approve yet, but we will get back to it. Thanks for the work and patience!

phughk commented 1 year ago

Hi @MrShutCo ! Would you be able to merge the latest changes so we can get this in? It would be really cool to have in included!

MrShutCo commented 1 year ago

@phughk Pulled in the latest changes. It still says requested changes from @tobiemh but I cant re-request a review. Has anything changed in the http endpoints since i first made this pr?

phughk commented 1 year ago

Landing this, thank you for persistence @MrShutCo !

phughk commented 1 year ago

Waiting for asserts to replace println then will approve @MrShutCo

MrShutCo commented 1 year ago

@phughk What should be done about the response object? Ive tried to replace the fmt statements but its kinda hard since the objects look like interface{}([]interface{}{map[string]interface{}{"child": interface{}(nil), "id": "", "name": "foobar"}} which looks horrifying. I recall there was some discussion about this when the PR first went up, but whats the consensus now? I cant imagine anyone wanting to use that to try and unmarshal

I also tried SmartUnmarshal but its built just for websockets stuff and fails on my end

phughk commented 1 year ago

We can use the complex types for now and address in the future but agree

timpratim commented 1 year ago

Hey @MrShutCo , could you please add in a new PR to skip all the new code updates? Or else merge this PR?