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 relevant fields to be pointers #493

Merged
merged 77 commits into from Dec 8, 2023
Merged

Conversation

Integralist
Copy link
Collaborator

@Integralist Integralist commented Nov 27, 2023

Problem

The mapstructure dependency will take a null from the API response and coerce it into the zero value for the associated field type. This has resulted in errors in the Fastly Terraform provider where certain fields return from the API as null but are coerced by go-fastly into a zero value such as 0 and then stored in Terraform state as 0, which is incorrect behaviour (e.g. keepalive_time should be stored in Terraform as null not 0).

Solution

We already use pointers for Create/Update methods, we should extend this so that the entire code base consistently uses pointers (for both input and output structs alike).

Exceptions to this rule are newer APIs like those for the 'stores' (Config, KV, Secret) and any API that implements JSONAPI as I'm not confident in how pointers will be handled by the JSONAPI package (also TLS uses it and so it feels extra risky).

Notes

I've discovered via the test suite a bunch of cases where null was being coerced into a type's zero value (e.g. empty string and zero mostly) and so I've updated the tests for those assertions to check for nil now as that's what we're expecting.

As discussed in a separate breaking PR (generic pagination), we've decided to no longer sort the APIs response data. So you'll see a bunch of struct types (across most files) that implemented the sort interface are now removed.

@Integralist Integralist changed the title fix(acl): fix all relevant fields to be pointers fix: all relevant fields to be pointers Nov 27, 2023
@Integralist Integralist marked this pull request as ready for review November 30, 2023 18:29
@Integralist
Copy link
Collaborator Author

Successfully integrated this version of go-fastly with an earlier generic pagination script I was using...

package main

import (
	"fmt"
	"log"
	"os"
	"sort"

	"github.com/fastly/go-fastly/v8/fastly" // uses `replace` directive in go.mod locally
)

const (
	serviceID = "<REDACTED>"
	aclID     = "<REDACTED>"
	dictID    = "<REDACTED>"
)

func main() {
	client, err := fastly.NewClient(os.Getenv("FASTLY_API_KEY"))
	if err != nil {
		log.Fatal(err)
	}

	getServicesInput := &fastly.GetServicesInput{
		Direction: fastly.ToPointer("ascend"),
		PerPage:   fastly.ToPointer(50),
		Sort:      fastly.ToPointer("type"),
	}
	p := client.GetServices(getServicesInput)

	var results []*fastly.Service
	for p.HasNext() {
		data, err := p.GetNext()
		if err != nil {
			fmt.Printf("failed to get next page (remaining: %d): %s", p.Remaining(), err)
			return
		}
		results = append(results, data...)
	}

	fmt.Printf("%#v\n", len(results))
	for _, service := range results {
		fmt.Printf("%#v (%s)\n", *service.Name, *service.Type)
	}

	sort.Stable(servicesByName(results))
	fmt.Printf("\nSorted:\n\n")
	for _, service := range results {
		fmt.Printf("%#v (%s)\n", *service.Name, *service.Type)
	}

	fmt.Printf("\nUsing ListServices:\n\n")
	listServiceResults, err := client.ListServices(&fastly.ListServicesInput{
		Direction: fastly.ToPointer("ascend"),
		Sort:      fastly.ToPointer("type"),
	})
	if err != nil {
		log.Fatal(err)
	}
	fmt.Printf("(%d) %#v\n", len(listServiceResults), listServiceResults)

	fmt.Println("---------------------------------------------------------------")

	getACLEntriesInput := &fastly.GetACLEntriesInput{
		ACLID:     aclID,
		Direction: fastly.ToPointer("ascend"),
		PerPage:   fastly.ToPointer(50),
		ServiceID: serviceID,
		Sort:      fastly.ToPointer("ip"),
	}
	p2 := client.GetACLEntries(getACLEntriesInput)

	var results2 []*fastly.ACLEntry
	for p2.HasNext() {
		data, err := p2.GetNext()
		if err != nil {
			fmt.Printf("failed to get next page (remaining: %d): %s", p2.Remaining(), err)
			return
		}
		results2 = append(results2, data...)
	}

	fmt.Printf("%#v\n", len(results2))
	for _, aclEntry := range results2 {
		fmt.Printf("%s | %s\n", *aclEntry.IP, *aclEntry.ID)
	}

	sort.Stable(entriesByID(results2))
	fmt.Printf("\nSorted:\n\n")
	for _, aclEntry := range results2 {
		fmt.Printf("%s | %s\n", *aclEntry.IP, *aclEntry.ID)
	}

	fmt.Printf("\nUsing ListACLEntries:\n\n")
	listACLResults, err := client.ListACLEntries(&fastly.ListACLEntriesInput{
		ACLID:     aclID,
		Direction: fastly.ToPointer("ascend"),
		ServiceID: serviceID,
		Sort:      fastly.ToPointer("ip"),
	})
	if err != nil {
		log.Fatal(err)
	}
	fmt.Printf("(%d) %#v\n", len(listACLResults), listACLResults)

	fmt.Println("---------------------------------------------------------------")

	getDictionaryItemsInput := &fastly.GetDictionaryItemsInput{
		DictionaryID: dictID,
		Direction:    fastly.ToPointer("ascend"),
		PerPage:      fastly.ToPointer(50),
		ServiceID:    serviceID,
		Sort:         fastly.ToPointer("item_value"),
	}
	p3 := client.GetDictionaryItems(getDictionaryItemsInput)

	var results3 []*fastly.DictionaryItem
	for p3.HasNext() {
		data, err := p3.GetNext()
		if err != nil {
			fmt.Printf("failed to get next page (remaining: %d): %s", p3.Remaining(), err)
			return
		}
		results3 = append(results3, data...)
	}

	fmt.Printf("%#v\n", len(results3))
	for _, dictItem := range results3 {
		fmt.Printf("%s | %s\n", *dictItem.ItemKey, *dictItem.ItemValue)
	}

	sort.Stable(dictionaryItemsByKey(results3))
	fmt.Printf("\nSorted:\n\n")
	for _, dictItem := range results3 {
		fmt.Printf("%s | %s\n", *dictItem.ItemKey, *dictItem.ItemValue)
	}

	fmt.Printf("\nUsing ListDictionaryItems:\n\n")
	listDictResults, err := client.ListDictionaryItems(&fastly.ListDictionaryItemsInput{
		DictionaryID: dictID,
		Direction:    fastly.ToPointer("ascend"),
		ServiceID:    serviceID,
		Sort:         fastly.ToPointer("item_value"),
	})
	if err != nil {
		log.Fatal(err)
	}
	fmt.Printf("(%d) %#v\n", len(listDictResults), listDictResults)

	fmt.Println("---------------------------------------------------------------")
}

// servicesByName is a list of services that are sortable by their name.
type servicesByName []*fastly.Service

// Len implement the sortable interface.
func (s servicesByName) Len() int {
	return len(s)
}

// Swap implement the sortable interface.
func (s servicesByName) Swap(i, j int) {
	s[i], s[j] = s[j], s[i]
}

// Less implement the sortable interface.
func (s servicesByName) Less(i, j int) bool {
	return *s[i].Name < *s[j].Name
}

// entriesByID is a list of ACL entries that are sortable by their ID.
type entriesByID []*fastly.ACLEntry

// Len implements the sortable interface.
func (s entriesByID) Len() int {
	return len(s)
}

// Swap implements the sortable interface.
func (s entriesByID) Swap(i, j int) {
	s[i], s[j] = s[j], s[i]
}

// Less implements the sortable interface.
func (s entriesByID) Less(i, j int) bool {
	return *s[i].ID < *s[j].ID
}

// dictionaryItemsByKey is a list of Dictionary items that are sortable by their key.
type dictionaryItemsByKey []*fastly.DictionaryItem

// Len implement the sortable interface.
func (s dictionaryItemsByKey) Len() int {
	return len(s)
}

// Swap implement the sortable interface.
func (s dictionaryItemsByKey) Swap(i, j int) {
	s[i], s[j] = s[j], s[i]
}

// Less implement the sortable interface.
func (s dictionaryItemsByKey) Less(i, j int) bool {
	return *s[i].ItemKey < *s[j].ItemKey
}

@Integralist
Copy link
Collaborator Author

I've integrated these changes into the CLI successfully.

I'm now working on integrating these changes into the Fastly Terraform Provider.

@Integralist
Copy link
Collaborator Author

I've successfully integrated these changes into the Fastly Terraform provider.

@Integralist Integralist merged commit edcae50 into main Dec 8, 2023
2 checks passed
@Integralist Integralist deleted the integralist/pointers branch December 8, 2023 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant