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

Go SDK Enhancement Opportunities #13

Open
jon-whit opened this issue Apr 12, 2023 · 0 comments
Open

Go SDK Enhancement Opportunities #13

jon-whit opened this issue Apr 12, 2023 · 0 comments

Comments

@jon-whit
Copy link
Member

  • Reduce the stutter of client invocations
    The underlying OpenFgaApi client interface should be embedded. For example,

    config, err := openfga.NewConfiguration(openfga.Configuration{...})
    if err != nil {
    // .. Handle error
    }
    
    apiClient := openfga.NewAPIClient(config)
    
    // current design
    stores, response, err := apiClient.OpenFgaApi.ListStores(context.Background()).Execute()
    
    // recommended design
    stores, response, err := apiClient.ListStores(context.Background()).Execute()

    It's possible to just store the underlying OpenFgaApi as a variable and use that, but that also requires a level of indirection that is unecessary. Point is - the APIClient should have the API methods attached to it directly, and this is best achieved by embedding the underlying interface.

    type APIClient struct {
        ...
    	OpenFgaApi OpenFgaApi
    }

    changes to

    type APIClient struct {
        ...
        OpenFgaApi
    }
  • Make the API surface more idiomatic, streamlined, and RPC style

    The Go SDK is currently very verbose and not idiomatic. Building a Go integration with the OpenFGA Go SDK involved non-standard Go idioms, and this makes consuming the libraries API surface more challenging than other Go SDKs for other APIs.

    Our API is an RPC style API, and most Go clients that are idiomatic define client interfaces with request inputs and response outputs like so:

    type APIClientInterface interface {
        Read(ctx context.Context, req *ReadRequest) (*ReadResponse, error)
        Write(ctx context.Context, reqw *WriteRequest) (*WriteResponse, error)
    }

    In fact, if you look at the Go gRPC generated client stubs this is what generated code like this gets produced as. I acknowledge that our HTTP API defines custom mappings to our RPC API, and so we'd need to generate this structure off of the OpenAPI definition instead of inheriting the default HTTP-->gRPC transcoding mapping, but it would be nice to stay within this idiom.

    For example

    var client openfga.OpenFgaApi
    
    // current design
    readResp, httpResp, err := client.
      Read(context.Background()).
      Body(auth0fga.ReadRequest{...}).
      Execute() 
      
    // recommended design
    readResp, httpResp, err := client.Read(context.Background(), &auth0fga.ReadRequest{...})
  • Drop the HTTP response param from the return list
    Our code currently returns the raw HTTP response as part of the method signatures. I suppose this isn't necessarily a bad thing, but it's definitely a Go idiom I have not particularly seen that used in the wild. I would also argue that exposing the raw HTTP response is counter productive to the Go SDK in the first place. The SDK is supposed to be abstracting the underlying HTTP API, and this makes it possible for client applications to build integrations they depend on with the raw HTTP API. That makes this part of the API surface another thing we have to maintain compatibility with even though the SDK is purposefully trying to abstract it.

    I recommend we drop the HTTP response object from the response altogether and depend upon the error usage pattern to appropriately capture the error semantics of the underlying HTTP response behavior. Lots of Go API client libs define an APIError structure and it implements the error interface, and they return this with the status and any underlying error message (if status is non 200). Also, if you need access to the Headers (for rate limiting for example), then it's also a common Go idiom to embed those in the response object.

    var client openfga.OpenFgaApi
    
    // current design
    readResp, httpResp, err := client.Read(context.Background()).Body(...).Execute()
    
    if httpResp.StatusCode != http.StatusOK {
      // handle error behavior
    }
    
    if remaining := httpResp.Header.Get("x-ratelimit-remaining"); remaining > 1 {
        // do next request, for example
    }
    
    // recommended design
    type APIErrorStatus int // enum set of codes for consummable API errors
    
    type APIError struct {
        ... // other metadata as needed
        Status APIErrorStatus
        Message string
    }
    
    type APIResponse[T any] interface {
        GetResponse() (T, error)
        GetHeaders http.Header
    }
    
    readResp, err := client.Read(context.Background(), &openfga.ReadRequest{...})
    if err != nil {
        if errors.Is(err, openfga.APIError) {
            // handle specific `APIErrorStatus`
        }
    }
    
    if remaining := readResp.GetHeaders()["x-ratelimit-remaining"]; remaining > 1 {
        // do next request, for example
    }

    A great example of a widely used Go SDK that follows some common Go idioms and is very easy to consume is the Stripe Go SDK (https://github.com/stripe/stripe-go). There are examples of this ^^ in that SDK.

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

No branches or pull requests

1 participant