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

Fix: "all_teams" data resource causes 429 status code requests #988

Merged
merged 4 commits into from
Dec 8, 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
7 changes: 6 additions & 1 deletion client/api_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ type ApiClient struct {
http http.HttpClientInterface
cachedOrganizationId string
defaultOrganizationId string
memoizedGetTeams func(string) ([]Team, error)
}

type ApiClientInterface interface {
Expand Down Expand Up @@ -172,9 +173,13 @@ type ApiClientInterface interface {
}

func NewApiClient(client http.HttpClientInterface, defaultOrganizationId string) ApiClientInterface {
return &ApiClient{
apiClient := &ApiClient{
http: client,
cachedOrganizationId: "",
defaultOrganizationId: defaultOrganizationId,
}

apiClient.memoizedGetTeams = memoize(apiClient.GetTeams)

return apiClient
}
5 changes: 2 additions & 3 deletions client/configuration_variable_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package client_test

import (
"encoding/json"
"strings"
"testing"

. "github.com/env0/terraform-provider-env0/client"
Expand Down Expand Up @@ -309,7 +308,7 @@ func TestConfigurationVariableMarshelling(t *testing.T) {

b, err := json.Marshal(&variable)
if assert.NoError(t, err) {
assert.False(t, strings.Contains(string(b), str))
assert.NotContains(t, string(b), str)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

linter complaints...

}

type ConfigurationVariableDummy ConfigurationVariable
Expand All @@ -318,7 +317,7 @@ func TestConfigurationVariableMarshelling(t *testing.T) {

b, err = json.Marshal(&dummy)
if assert.NoError(t, err) {
assert.True(t, strings.Contains(string(b), str))
assert.Contains(t, string(b), str)
}

var variable2 ConfigurationVariable
Expand Down
27 changes: 27 additions & 0 deletions client/memoize.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package client
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

generic memoization function.


type memoizedResult[V any] struct {
value V
err error
}

func memoize[K comparable, V any](f func(K) (V, error)) func(K) (V, error) {
cache := make(map[K]memoizedResult[V])

return func(key K) (V, error) {
if res, ok := cache[key]; ok {
return res.value, res.err
}

value, err := f(key)

cache[key] = memoizedResult[V]{value: value, err: err}

return value, err
}
}

// MemoizeExported exports the memoize function for testing
func MemoizeExported[K comparable, V any](f func(K) (V, error)) func(K) (V, error) {
return memoize(f)
}
68 changes: 68 additions & 0 deletions client/memoize_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
package client_test

import (
"errors"
"testing"

"github.com/env0/terraform-provider-env0/client"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestMemoize(t *testing.T) {
t.Run("should cache successful results", func(t *testing.T) {
callCount := 0
f := func(s string) ([]client.Team, error) {
callCount++

return []client.Team{{Name: s}}, nil
}

memoized := client.MemoizeExported(f)

// First call
result1, err1 := memoized("")
require.NoError(t, err1)
assert.Len(t, result1, 1)
assert.Equal(t, 1, callCount)

// Second call with same input - should use cache
result2, err2 := memoized("")
require.NoError(t, err2)
assert.Len(t, result2, 1)
assert.Equal(t, 1, callCount) // Count shouldn't increase

// Different input - should call function again
result3, err3 := memoized("test")
require.NoError(t, err3)
assert.Len(t, result3, 1)
assert.Equal(t, "test", result3[0].Name)
assert.Equal(t, 2, callCount)
})

t.Run("should cache errors", func(t *testing.T) {
callCount := 0
expectedError := errors.New("test error")
f := func(s string) ([]client.Team, error) {
callCount++

return nil, expectedError
}

memoized := client.MemoizeExported(f)

// First call
result1, err1 := memoized("")
require.Error(t, err1)
assert.Equal(t, expectedError, err1)
assert.Nil(t, result1)
assert.Equal(t, 1, callCount)

// Second call - should return cached error
result2, err2 := memoized("")
require.Error(t, err2)
assert.Equal(t, expectedError, err2)
assert.Nil(t, result2)
assert.Equal(t, 1, callCount) // Count shouldn't increase
})
}
11 changes: 8 additions & 3 deletions client/team.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,12 @@ func (client *ApiClient) TeamUpdate(id string, payload TeamUpdatePayload) (Team,
return result, nil
}

func (client *ApiClient) GetTeams(params map[string]string) ([]Team, error) {
func (client *ApiClient) GetTeams(name string) ([]Team, error) {
params := map[string]string{"limit": "100"}
if name != "" {
params["name"] = name
}

organizationId, err := client.OrganizationId()
if err != nil {
return nil, err
Expand Down Expand Up @@ -112,9 +117,9 @@ func (client *ApiClient) GetTeams(params map[string]string) ([]Team, error) {
}

func (client *ApiClient) Teams() ([]Team, error) {
return client.GetTeams(map[string]string{"limit": "100"})
return client.memoizedGetTeams("")
}

func (client *ApiClient) TeamsByName(name string) ([]Team, error) {
return client.GetTeams(map[string]string{"name": name, "limit": "100"})
return client.memoizedGetTeams(name)
}
1 change: 1 addition & 0 deletions env0/data_teams.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
func dataTeams() *schema.Resource {
return &schema.Resource{
ReadContext: dataTeamsRead,
Description: "Note: this data source is cached, once fetched it will not be updated until the next plan/apply",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added this note so users understand limitations.


Schema: map[string]*schema.Schema{
"names": {
Expand Down
Loading