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

Added basic http client #7

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

MrShutCo
Copy link

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

This comment was marked as outdated.

Copy link
Member

@tobiemh tobiemh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

httpclient/client.go Outdated Show resolved Hide resolved
httpclient/client.go Outdated Show resolved Hide resolved
httpclient/client.go Outdated Show resolved Hide resolved
httpclient/client.go Outdated Show resolved Hide resolved
httpclient/client.go Outdated Show resolved Hide resolved
@MrShutCo
Copy link
Author

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?

@MrShutCo MrShutCo requested a review from tobiemh September 21, 2022 02:53
@tobiemh
Copy link
Member

tobiemh commented Sep 22, 2022

@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
Copy link
Contributor

intabulas commented Sep 22, 2022

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
Copy link
Author

@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
Copy link
Contributor

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

@MrShutCo
Copy link
Author

@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 :)

go.mod Outdated Show resolved Hide resolved
client.go Show resolved Hide resolved
client.go Show resolved Hide resolved
client.go Show resolved Hide resolved
client.go Outdated Show resolved Hide resolved
@MrShutCo MrShutCo requested a review from tobiemh September 25, 2022 14:29
@tobiemh
Copy link
Member

tobiemh commented Oct 18, 2022

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
Copy link
Author

@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
Copy link
Member

tobiemh commented Oct 18, 2022

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
Copy link
Contributor

phughk commented Mar 7, 2023

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 phughk added the enhancement New feature or request label Mar 9, 2023
@phughk
Copy link
Contributor

phughk commented Apr 20, 2023

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
Copy link
Author

@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
Copy link
Contributor

phughk commented Apr 26, 2023

Landing this, thank you for persistence @MrShutCo !

@phughk phughk assigned phughk and unassigned tobiemh Apr 26, 2023
Copy link
Contributor

@phughk phughk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall approve, 1 comment for many lines, tagging @tobiemh to approve or remove request for change as I can't override.


resp, err := client.CreateOne("personreplace", "surreal", `{child: null, name: "FooBar"}`)
IsEqual(t, nil, err)
fmt.Println(resp.Result)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we change these println to testify asserts please?

@tobiemh tobiemh dismissed their stale review April 26, 2023 14:16

Ignore

@phughk
Copy link
Contributor

phughk commented Apr 30, 2023

Waiting for asserts to replace println then will approve @MrShutCo

@MrShutCo
Copy link
Author

MrShutCo commented Apr 30, 2023

@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
Copy link
Contributor

phughk commented May 12, 2023

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

@timpratim
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants