Fix panic caused by incorrect type assert. #377
Conversation
Integralist
requested review from
a team,
phamann and
kailan
and removed request for
a team
| for _, elem := range set.List() { | ||
| s = append(s, elem.(string)) | ||
| } | ||
| opts.ContentTypes = gofastly.String(strings.Join(s, " ")) |
There was a problem hiding this comment.
Noting here for reviewers, this API accepts the types as a space-seperated list, so this is correct 👍
https://developer.fastly.com/reference/api/vcl-services/gzip/
| name = "amazon docs" | ||
| } | ||
|
|
||
| gzip { |
There was a problem hiding this comment.
I want to clarify why this new test validates the actual 'update' mechanics...
The original test thought it was validating a 'modification' to the gzip resource, but because they changed the name field (along with other fields) it meant that the gzip resource was actually being 'deleted' and then 'recreated' so the 'modification' logic (which is where the panic happens) was never actually run.
To ensure the modification/update logic is executed I created a new test that updated the gzip resource but left the name the same and just modified some of the other fields.
Problem: the gzip resource was recently migrated to using the
DiffSetalgorithm for determining changes, which introduced a bug where we incorrectly were type asserting a Set into a String which would cause a panic at runtime.Notes: the tests didn't catch this because the pre-existing gzip tests were written in such a way that the 'update' test was actually causing the gzip resource to be deleted and recreated anew (because it was modifying the 'name' field) and so I've modified the tests to address this.