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

feat: Domains API operations #208

Merged
merged 1 commit into from
Feb 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion clerk.go
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ func (b *defaultBackend) do(req *http.Request, params Queryable, setter Response
func setRequestBody(req *http.Request, params Queryable) error {
// GET requests don't have a body, but we will pass the params
// in the query string.
if req.Method == http.MethodGet {
if req.Method == http.MethodGet && params != nil {
q := req.URL.Query()
params.Add(q)
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ I just noticed this and I'm not sure I understand it 100%. This also allows for some room for someone to erroneously tamper with the q object, if they implement their own custom struct, isn't that right? I would probably expect it to be the other way round, but let me know what you think (and this goes up to the APIRequest struct I believe):

func setRequestBody(req *http.Request, params url.Values) error {
  if req.Method == http.MethodGet && params != nil {
    q := req.URL.Query()
    for key, values := range params {
       for _, v := range values {
         q.Add(key, v)
       }
    }

The Queryable interface could be then:

type Queryable interface {
  ToQueryParameters() url.Values
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Great point! I'll update the interface and the logic. I'd rather do it on a separate PR. Do you think that would be a problem?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not a problem at all, by all means proceed with merging 🚀

req.URL.RawQuery = q.Encode()
Expand Down
12 changes: 12 additions & 0 deletions deleted_resource.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package clerk

// DeletedResource describes an API resource that is no longer
// available.
// It's usually encountered as a result of delete API operations.
type DeletedResource struct {
APIResource
ID string `json:"id,omitempty"`
Slug string `json:"slug,omitempty"`
Object string `json:"object"`
Deleted bool `json:"deleted"`
}
25 changes: 25 additions & 0 deletions domain.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package clerk
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ Couldn't we use the domain package here instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could, but I'm afraid that at some point we might need this type from another package. Imagine an Instance which has a nested Domain.

I believe it's safer to have these types for "models" in the clerk package so we don't run into any circular dependecy problem in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, I thought that we might want to reuse structs. Do you think that if we introduced a models/<service> kind of structure, we might run into circular dependency problems? Just thinking out loud here:

import (
  domainmodels "github.com/clerk/clerk-sdk-go/v2/models/domains"
)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! Your solution would certainly avoid circular dependency, but isn't it a bit too much packaging?

Is there any reason why using the clerk package might be problematic?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not problematic, it's only that packages provide a native namespace and the import aliases provide some more naming flexibility for someone that uses them. Names also become more uniform.

package domain

// Debatable name :)
type Object struct{}
type List struct {
  Data []Object `json:"data"`
}
import (
  domainmodels "github.com/clerk/clerk-sdk-go/v2/models/domains"
) 

func HandleDomains() {
  var domainList domainmodels.List
  //...
}

Just an idea here, not insisting on this, it might be overly verbose/complicated for some reason. Noting now, since it would be a breaking change afterwards!


type Domain struct {
APIResource
ID string `json:"id"`
Object string `json:"object"`
Name string `json:"name"`
IsSatellite bool `json:"is_satellite"`
FrontendAPIURL string `json:"frontend_api_url"`
AccountPortalURL *string `json:"accounts_portal_url,omitempty"`
ProxyURL *string `json:"proxy_url,omitempty"`
CNAMETargets []CNAMETarget `json:"cname_targets,omitempty"`
DevelopmentOrigin string `json:"development_origin"`
}

type CNAMETarget struct {
Host string `json:"host"`
Value string `json:"value"`
}

type DomainList struct {
APIResource
Domains []*Domain `json:"data"`
TotalCount int64 `json:"total_count"`
}
73 changes: 73 additions & 0 deletions domain/domain.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
// Package domain provides the Domains API.
package domain

import (
"context"
"net/http"
"net/url"

"github.com/clerk/clerk-sdk-go/v2"
)

const path = "/domains"

type CreateParams struct {
clerk.APIParams
Name *string `json:"name,omitempty"`
ProxyURL *string `json:"proxy_url,omitempty"`
IsSatellite *bool `json:"is_satellite,omitempty"`
}

// Create creates a new domain.
func Create(ctx context.Context, params *CreateParams) (*clerk.Domain, error) {
req := clerk.NewAPIRequest(http.MethodPost, path)
req.SetParams(params)

domain := &clerk.Domain{}
err := clerk.GetBackend().Call(ctx, req, domain)
return domain, err
}

type UpdateParams struct {
clerk.APIParams
Name *string `json:"name,omitempty"`
ProxyURL *string `json:"proxy_url,omitempty"`
}

// Update updates a domain's properties.
func Update(ctx context.Context, id string, params *UpdateParams) (*clerk.Domain, error) {
path, err := url.JoinPath(path, id)
if err != nil {
return nil, err
}
req := clerk.NewAPIRequest(http.MethodPatch, path)
req.SetParams(params)

domain := &clerk.Domain{}
err = clerk.GetBackend().Call(ctx, req, domain)
return domain, err
}

// Delete removes a domain.
func Delete(ctx context.Context, id string) (*clerk.DeletedResource, error) {
path, err := url.JoinPath(path, id)
if err != nil {
return nil, err
}
req := clerk.NewAPIRequest(http.MethodDelete, path)
domain := &clerk.DeletedResource{}
err = clerk.GetBackend().Call(ctx, req, domain)
return domain, err
}

type ListParams struct {
clerk.APIParams
}

// List returns a list of domains
func List(ctx context.Context, params *ListParams) (*clerk.DomainList, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

🔧 If there's not a strong reason to use pointers, I think we should avoid them where possible, as they make garbage collection more difficult and the risk of accidental mutations increases.

req := clerk.NewAPIRequest(http.MethodGet, path)
list := &clerk.DomainList{}
err := clerk.GetBackend().Call(ctx, req, list)
return list, err
}
194 changes: 194 additions & 0 deletions domain/domain_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,194 @@
package domain

import (
"bytes"
"context"
"encoding/json"
"fmt"
"io"
"net/http"
"testing"

"github.com/clerk/clerk-sdk-go/v2"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestDomainCreate(t *testing.T) {
name := "clerk.com"
id := "dmn_123"
clerk.SetBackend(clerk.NewBackend(&clerk.BackendConfig{
HTTPClient: &http.Client{
Transport: &mockRoundTripper{
T: t,
in: json.RawMessage(fmt.Sprintf(`{"name":"%s"}`, name)),
out: json.RawMessage(fmt.Sprintf(`{"id":"%s","name":"%s"}`, id, name)),
path: "/v1/domains",
method: http.MethodPost,
},
},
}))

dmn, err := Create(context.Background(), &CreateParams{
Name: clerk.String(name),
})
require.NoError(t, err)
assert.Equal(t, id, dmn.ID)
assert.Equal(t, name, dmn.Name)
}

func TestDomainCreate_Error(t *testing.T) {
clerk.SetBackend(clerk.NewBackend(&clerk.BackendConfig{
HTTPClient: &http.Client{
Transport: &mockRoundTripper{
T: t,
status: http.StatusBadRequest,
out: json.RawMessage(`{
"errors":[{
"code":"create-error-code"
}],
"clerk_trace_id":"create-trace-id"
}`),
},
},
}))

_, err := Create(context.Background(), &CreateParams{})
require.Error(t, err)
apiErr, ok := err.(*clerk.APIErrorResponse)
require.True(t, ok)
assert.Equal(t, "create-trace-id", apiErr.TraceID)
require.Equal(t, 1, len(apiErr.Errors))
assert.Equal(t, "create-error-code", apiErr.Errors[0].Code)
}

func TestDomainUpdate(t *testing.T) {
id := "dmn_456"
name := "clerk.dev"
clerk.SetBackend(clerk.NewBackend(&clerk.BackendConfig{
HTTPClient: &http.Client{
Transport: &mockRoundTripper{
T: t,
in: json.RawMessage(fmt.Sprintf(`{"name":"%s"}`, name)),
out: json.RawMessage(fmt.Sprintf(`{"id":"%s","name":"%s"}`, id, name)),
path: fmt.Sprintf("/v1/domains/%s", id),
method: http.MethodPatch,
},
},
}))

dmn, err := Update(context.Background(), id, &UpdateParams{
Name: clerk.String(name),
})
require.NoError(t, err)
assert.Equal(t, id, dmn.ID)
assert.Equal(t, name, dmn.Name)
}

func TestDomainUpdate_Error(t *testing.T) {
clerk.SetBackend(clerk.NewBackend(&clerk.BackendConfig{
HTTPClient: &http.Client{
Transport: &mockRoundTripper{
T: t,
status: http.StatusBadRequest,
out: json.RawMessage(`{
"errors":[{
"code":"update-error-code"
}],
"clerk_trace_id":"update-trace-id"
}`),
},
},
}))

_, err := Update(context.Background(), "dmn_123", &UpdateParams{})
require.Error(t, err)
apiErr, ok := err.(*clerk.APIErrorResponse)
require.True(t, ok)
assert.Equal(t, "update-trace-id", apiErr.TraceID)
require.Equal(t, 1, len(apiErr.Errors))
assert.Equal(t, "update-error-code", apiErr.Errors[0].Code)
}

func TestDomainDelete(t *testing.T) {
id := "dmn_789"
clerk.SetBackend(clerk.NewBackend(&clerk.BackendConfig{
HTTPClient: &http.Client{
Transport: &mockRoundTripper{
T: t,
out: json.RawMessage(fmt.Sprintf(`{"id":"%s","deleted":true}`, id)),
path: fmt.Sprintf("/v1/domains/%s", id),
method: http.MethodDelete,
},
},
}))

dmn, err := Delete(context.Background(), id)
require.NoError(t, err)
assert.Equal(t, id, dmn.ID)
assert.True(t, dmn.Deleted)
}

func TestDomainList(t *testing.T) {
clerk.SetBackend(clerk.NewBackend(&clerk.BackendConfig{
HTTPClient: &http.Client{
Transport: &mockRoundTripper{
T: t,
Copy link
Contributor

Choose a reason for hiding this comment

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

🔧 Don't you want to assert that the GET method is used here?

out: json.RawMessage(`{
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 You can also introduce a helper here:

out: MustJSONMarshal(clerk.DomainList{}),

Since JSON serialization/deserialization is generally symmetric, I think this doesn't break the expectation validity here.

"data": [{"id":"dmn_123","name":"clerk.com"}],
"total_count": 1
}`),
path: "/v1/domains",
method: http.MethodGet,
},
},
}))

list, err := List(context.Background(), &ListParams{})
require.NoError(t, err)
assert.Equal(t, int64(1), list.TotalCount)
assert.Equal(t, 1, len(list.Domains))
assert.Equal(t, "dmn_123", list.Domains[0].ID)
assert.Equal(t, "clerk.com", list.Domains[0].Name)
}

type mockRoundTripper struct {
T *testing.T
// Response status.
status int
// Response body.
out json.RawMessage
// If set, we'll assert that the request body
// matches.
in json.RawMessage
// If set, we'll assert the request path matches.
path string
// If set, we'll assert that the request method matches.
method string
}

func (rt *mockRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) {
georgepsarakis marked this conversation as resolved.
Show resolved Hide resolved
if rt.status == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Mostly a suggestion, it might be easier to use a constructor for initializations: newMockRoundTripper

rt.status = http.StatusOK
}

if rt.method != "" {
georgepsarakis marked this conversation as resolved.
Show resolved Hide resolved
require.Equal(rt.T, rt.method, r.Method)
}
if rt.path != "" {
georgepsarakis marked this conversation as resolved.
Show resolved Hide resolved
require.Equal(rt.T, rt.path, r.URL.Path)
}
if rt.in != nil {
georgepsarakis marked this conversation as resolved.
Show resolved Hide resolved
body, err := io.ReadAll(r.Body)
if err != nil {
return nil, err
}
defer r.Body.Close()
require.JSONEq(rt.T, string(rt.in), string(body))
georgepsarakis marked this conversation as resolved.
Show resolved Hide resolved
}

return &http.Response{
StatusCode: rt.status,
Body: io.NopCloser(bytes.NewReader(rt.out)),
}, nil
}
Loading