open-feature / go-sdk-contrib

Community maintained OpenFeature Providers and Hooks for Go
https://openfeature.dev
Apache License 2.0
46 stars 45 forks source link

fix: Calling `err.Error()` on nil error causes panic #579

Closed dangra closed 1 month ago

dangra commented 2 months ago

This PR

fix a panic when /v1/flag/change returns an http status code other than 200 and 304.

Related Issues

Fixes #578 , a bug introduced in #547

Notes

In my case the GOFF endpoint returned 404 but the nil error is more subtle and worth fixing.

the 404 isn't logged by go-feature-flag daemon but I captured it by capturing packets:

IP 127.0.0.1.51966 > 127.0.0.1.1031: Flags [P.], seq 1:142, ack 1, win 512, options [nop,nop,TS val 1661138460 ecr 1661138460], length 141
E.....@.@.................c..W.............
c...c...GET /v1/flag/change HTTP/1.1
Host: localhost:1031
User-Agent: Go-http-client/1.1
Content-Type: application/json
Accept-Encoding: gzip

IP 127.0.0.1.1031 > 127.0.0.1.51966: Flags [P.], seq 1:169, ack 142, win 512, options [nop,nop,TS val 1661138461 ecr 1661138460], length 168
E...&.@.@..c.............W....dk...........
c...c...HTTP/1.1 404 Not Found
Content-Type: application/json; charset=UTF-8
Vary: Origin
Date: Fri, 06 Sep 2024 21:21:13 GMT
Content-Length: 24

{"message":"Not Found"}

How to test

Point the client to a webserver that responds with 404 on /v1/flag/change

I am using go-feature-flag v1.24.0 on my setup.

beeme1mr commented 1 month ago

Thanks for the PR @dangra! I've approved, but I'll defer to @thomaspoignant for a final approval.

thomaspoignant commented 1 month ago

Good catch @dangra and thanks for the fix. I will release a new version of the provider including this fix.

PS: thanks @beeme1mr I missed this PR before :(