Waf resource import #17

oscarDovao · 2020-01-03T15:05:45Z

This PR contains a new waf version struct method that checks for emptiness. Empty, in this case, means that non of the configuration fields are populated. This method can be used to prevent sending a back-end server request which will result on an error

phamann

Whilst this solution is simple and elegant and is suitable for 98% of use-cases it solves, the underlying problem it is trying to solve is a much wider issue across the go-fastly client (not just the WAF related structs) and API clients in general.

This HasChanges() implementation doesn't account for the fact that a user may genuinely be wanting to set all of the values to their zero values (0, "", false etc).

The problem is the combination of Go's type zero values, JSON marshalling with the omitempty struct field tag, and a users intent to actually set a zero value. This blog post does a much better job at explaining the problem and well worth a read: https://willnorris.com/2014/05/go-rest-apis-and-pointers/

As the post suggests, the more robust solution would be to change the struct field values to all be pointers instead of the underlying type value. However - whilst I think this is the correct approach - this would be different to how all other structures in the go-fastly client are modelled and places some pain on the library users to convert their inputs to pointers and do nil pointer checks when consuming.

Another option would be for us to create an optional type structure to model values, such as this pattern i've used in other projects:

type Optional struct {
	Valid bool
}

func (o *Optional) Set() error {
	o.Valid = true
	return nil
}

// OptionalString models an optional string flag value.
type OptionalString struct {
	Optional
	Value string
}

// OptionalBool models an optional boolean flag value.
type OptionalBool struct {
	Optional
	Value bool
}

// etc...

However I think such patterns would be an even larger change for the client and consumers, as they have to know to use the inner .Value field to access the underlying data. As an aside, I'm a big van of real Optional types from languages like Scala and Rust, but thats a discussion for another day.

TL;DR is i'm ok with this implementation as is, however I'm writing this to open the wider discussion of how we solve this problem for the client going forward and would like yours and other thoughts on it.

oscarDovao · 2020-01-15T17:24:59Z

Thanks @phamann for the detailed explanation.

I see how using pointers would make the update more accurate and flexible in general terms.
Looking again at the resource's schema, i see a number of TypeInt fields which could technically (regardless the practicality, but for consistency) be set as 0 value and that would fail to be reflected on the server if the default is other than 0.

I like the Optional pattern and the semantics it brings to the implementation, but I agree it would be a bigger change for go-fastly SDK users to adopt.

Given that the current implementation of the WAF configuration doesn't work for 100% of the cases, we'd like to change this PATCH endpoint's to use pointers and document the reason why this is different from the others endpoints. We can have further discussions on making a bigger change so this issue is addressed across the entire SDK.

Please let us know if you agree with the change
Thanks

phamann

Agreed. Therefore, as per the discussion in the PR and in-person, can we please change the struct field values to use pointers and document why 👍

pegerto

LGTM

pegerto · 2020-01-21T14:41:24Z

fastly/waf_version.go

@@ -226,60 +226,72 @@ func (c *Client) GetWAFVersion(i *GetWAFVersionInput) (*WAFVersion, error) {
 	return &wafVer, nil
 }

-// UpdateWAFVersionInput is used as input to the UpdateWAFVersion function.
+// UpdateWAFVersionInput is used as input to the UpdateWAFVersion function. This struct uses pointers due to the problem
+// detailed on this issue https://trello.com/c/dbF8uqKn.


happy to have this in trello, as this is public probably make sense to reported as a GitHub issue (enhancement)

Changed to the PR link

pegerto · 2020-01-21T14:41:40Z

fastly/waf_version_test.go

@@ -160,92 +160,92 @@ func TestClient_WAF_Versions(t *testing.T) {

 func verifyWAFVersionUpdate(t *testing.T, i *UpdateWAFVersionInput, o *WAFVersion) {

-	if i.WAFVersionID != o.ID {
-		t.Errorf("expected %s waf: got %s", i.WAFVersionID, o.ID)
+	if *i.WAFVersionID != o.ID {


[IDEA] use testify to manage assertions,

I agree, but probably a bigger change so that it is used it consistently across all tests. @phamann any thoughts?

I would prefer not to introduce that change in this pr.

pegerto

LGTM

phamann

👍 LGTM other than minor comment.

fastly/waf_version.go

pegerto

LGTM

phamann

👍

* Introducing waf version struct method to check for custom emptiness * Using pointers on the WAf version update endpoint

* Service waf component (#5) * service waf component new API implemented * Waf configuration resource (#6) * waf version (configuration) API implemented * Waf active rules (#9) * WAF active rules implementation * Waf rules (#10) * WAF rules related endpoints implementation * Waf config resource deletion (#12) * create empty waf version endpoint implemented * Waf disable enable (#14) * making WAF enable and disable functions consistent with rest * introducing a maximum batch size exclusive for WAF requests * Waf resource import (#17) * Introducing waf version struct method to check for custom emptiness * Using pointers on the WAf version update endpoint * Cleaning up the old API remaining code (#18) * Update enable/disable WAF API * Update WAF active rules and WAF fixtures * Remove enable/disable WAF API * Fix type assertion for linter * Fix error message capitalisation for linter * Update waf_versions lock API * Update waf_versions activate API * Add LastDeploymentStatus so clients can deal with async deploys * Add testing for LastDeploymentStatus * Apply suggestions from code review * Fix fixtures for WAF tests * Setting go.mod package to v2 * Adding WAF Exclusion endpoints to go-fastly client Co-authored-by: Zsolt Balvanyos <5632209+ZsoltBalvanyos@users.noreply.github.com> * Changing WAF Exclusion terminology to WAF Rule Exclusion * Changing Include field to be a Slice instead of a comma-separated string * field consistency: ServiceID, ServiceVersion, DictionaryID, PoolID * use pointers as field values for optional basic types reference: https://willnorris.com/2014/05/go-rest-apis-and-pointers/ * Implement /stats/field API Fixes fastly#214 * README consistency with updated field identifiers * modernize the markdown syntax for defining headers * add migration notice for v1 to v2 * Update README with v1 and v2 tagged releases * Consistency in response struct fields * remove pointer references for non write/update structs... NOTE: this does not include 'create' structs that already had pointer references on them as I'm unaware of the context related to why they are pointers. * remove pointer references for existing non write/update structs * Compatibool should be non-pointer for 'create' structs * Use pointer reference for Header* constants on write/update structs. * Use pointer reference for PoolType constants on write/update structs. Co-authored-by: oscarDovao <31310209+oscarDovao@users.noreply.github.com> Co-authored-by: Oscar <dovaooj@gmail.com> Co-authored-by: Stuart Wallace <stuart@wapbot.co.uk> Co-authored-by: Patrick Hamann <patrick@fastly.com> Co-authored-by: Mateus Pimenta <1920261+matpimenta@users.noreply.github.com> Co-authored-by: Zsolt Balvanyos <5632209+ZsoltBalvanyos@users.noreply.github.com>

oscarDovao added 2 commits January 3, 2020 12:59

Introducing waf version struct method to check for custom emptiness

68b6e36

improved docs - renamed IsEmpty method

6230bc2

oscarDovao requested review from fgsch and phamann January 3, 2020 15:06

Merge branch 'service-waf-implementation' into waf-resource-import

e145318

phamann reviewed Jan 15, 2020

View reviewed changes

phamann suggested changes Jan 20, 2020

View reviewed changes

Using pointers on the WAf version update endpoint

850288a

pegerto approved these changes Jan 21, 2020

View reviewed changes

phamann approved these changes Jan 21, 2020

View reviewed changes

fastly/waf_version.go Outdated Show resolved Hide resolved

Pointing documentation to github PR

e237cc7

oscarDovao requested review from pegerto and phamann January 22, 2020 16:16

pegerto approved these changes Jan 22, 2020

View reviewed changes

phamann approved these changes Jan 22, 2020

View reviewed changes

oscarDovao merged commit 761f466 into service-waf-implementation Jan 22, 2020

oscarDovao deleted the waf-resource-import branch January 22, 2020 17:04

yoink00 pushed a commit that referenced this pull request Jul 13, 2020

Waf resource import (#17)

47b07ed

* Introducing waf version struct method to check for custom emptiness * Using pointers on the WAf version update endpoint

phamann pushed a commit that referenced this pull request Jul 22, 2020

Waf resource import (#17)

39049b8

* Introducing waf version struct method to check for custom emptiness * Using pointers on the WAf version update endpoint

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Waf resource import #17

Waf resource import #17

oscarDovao commented Jan 3, 2020

phamann left a comment

oscarDovao commented Jan 15, 2020

phamann left a comment

pegerto left a comment

pegerto Jan 21, 2020

oscarDovao Jan 22, 2020 •

edited

Loading

pegerto Jan 21, 2020

oscarDovao Jan 22, 2020

phamann Jan 22, 2020

pegerto left a comment

phamann left a comment

pegerto left a comment

phamann left a comment

Waf resource import #17

Waf resource import #17

Conversation

oscarDovao commented Jan 3, 2020

phamann left a comment

Choose a reason for hiding this comment

oscarDovao commented Jan 15, 2020

phamann left a comment

Choose a reason for hiding this comment

pegerto left a comment

Choose a reason for hiding this comment

pegerto Jan 21, 2020

Choose a reason for hiding this comment

oscarDovao Jan 22, 2020 • edited Loading

Choose a reason for hiding this comment

pegerto Jan 21, 2020

Choose a reason for hiding this comment

oscarDovao Jan 22, 2020

Choose a reason for hiding this comment

phamann Jan 22, 2020

Choose a reason for hiding this comment

pegerto left a comment

Choose a reason for hiding this comment

phamann left a comment

Choose a reason for hiding this comment

pegerto left a comment

Choose a reason for hiding this comment

phamann left a comment

Choose a reason for hiding this comment

oscarDovao Jan 22, 2020 •

edited

Loading