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

Initial commit with basic request/response. #1

Closed
wants to merge 3 commits into from
Closed

Initial commit with basic request/response. #1

wants to merge 3 commits into from

Conversation

tysont
Copy link
Contributor

@tysont tysont commented Dec 19, 2022

No description provided.

client.go Outdated
"time"
)

var authorizationHeader = "Authorization"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
var authorizationHeader = "Authorization"
const authorizationHeader = "Authorization"

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a constant?

client.go Outdated
func DefaultClient() *Client {
secret := os.Getenv(secretKey)
if secret == "" {
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

I think an error here is probably more expressive than nil.

client.go Outdated
return client
}

func Query[T any](c *Client, request *Request) *Response[T] {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be more ergonomic if this was a method of the Client struct returning T or an error, IMO. Further more, dealing with request/response types is probably going to require more code for the simple use-cases than we want to. I'd personally go for an API similar to:

coll, err = cli.Query("Collection.create({ name: 'User' })")
if err != nil { .. }

The request / response based API can still exist, I think, but for more advanced use-cases. Maybe something like:

res := cli.Execute(request)

The handing of query arguments is quite challenging to provide an ergonomic API for due to the lack of less verbose data types in Go. There are 2 options that pop to my head:

  1. Use anonymous structs:
res, err := cli.Query("Collection.create({ name: Name })", struct { Name string } { "Users" })
  1. Use a helper function and build the map arguments internally:
res, err := cli.Query("Collection.create({ name: Name })", Var("Name", "Users"))

Option 2. is a bit more "good looking" but, 1. is more flexible as it integrates better with the language.

Comment on lines +71 to +74
request.raw.Header.Add(authorizationHeader, c.token())
for k, v := range c.headers {
request.raw.Header.Add(k, v)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: we must keep track and send back the last seen txn time in order to guarantee txn consistency across nodes behind the load balancer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this functionality in the updated PR, mostly c/p from the existing driver.

raw *http.Request
Query string `json:"query"`
Arguments map[string]string `json:"arguments"`
Typecheck bool `json:"typecheck"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Is nice that we can flip this on and off at a per request basis but, from a user's perspective, I'd appreciate being able to set this a client level so I only need to care about it once.


type Response[T any] struct {
raw *http.Response
err error
Copy link
Contributor

Choose a reason for hiding this comment

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

there are different error categories for different failure modes.

Some discussion on implications for the drivers here: https://faunadb.atlassian.net/wiki/spaces/PROD/pages/3330408449/FQL-X+Exception+Model+Design

An open debate is making the error model's more tuned to the driver langauge - e.g. in a dynamically typed language like JS juset have a single type with fields that can be inspected.

However, since Go is statically types defining different types for each error seems like a usability win with little to no downside.

See the linked doc and the latest wire model: https://github.com/fauna/fauna-js/blob/main/src/wire-protocol.ts

for what those shapes are.


type Request struct {
raw *http.Request
Query string `json:"query"`
Copy link
Contributor

@cleve-fauna cleve-fauna Dec 21, 2022

Choose a reason for hiding this comment

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

Something to jam on next:

  • how do you enable queries to be composed in Go?

In the JS prototype that gets handled here: https://github.com/fauna/fauna-js/blob/main/src/query-builder.ts leveraging some native JS features.

The value for the customer is the following:

  • built in defense against "FQL Injection"
  • ability to define FQL snippets; or methods that build completed FQL

Example:

async function getTopNScores(n: number, game: string) {
  return client.query(fql`Scores.where(.game = ${game}).orderBy(.score).limit(${n}`);
}

I'm no go-pro; but poking around some possible paths to getting equivalent behavior:

https://pkg.go.dev/text/template
golang/go#34174 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that the output of the fql method is a request like:

{
  query: "Scores.where(.game = arg1).orderBy(.score).limit(arg2)"
  arguments: {
    arg1: "Tetris",
   arg2: 10,
  }
}

So it automates the building of queries away from a customer's concern.

Acheiving the same in Go and other drivers would be a nice usability win, I suspect.

@tysont tysont closed this Dec 21, 2022
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