-
-
Notifications
You must be signed in to change notification settings - Fork 345
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: Use make() to initialise array of Policy and Role instead of leaving it uninitalised #250
fix: Use make() to initialise array of Policy and Role instead of leaving it uninitalised #250
Conversation
…fix nil error in encoding/json. golang/go#27589 (comment)
*[]storage.Policy/Role{}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thank you for your contribution! This looks pretty good but I have some ideas how to improve it further :)
storage/filter_helper_test.go
Outdated
@@ -1,7 +1,7 @@ | |||
package storage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert the changes in this file :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from reverting this, I have also changed the expected nil values to match the empty policies/roles that is returned now.
Ah, I didn't know that! It's late here now, I will push the changes that you've suggested tomorrow |
@aeneasr , The requested changes have been made! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome 🎉
Thank you for your contribution!
Fixes #217
This is an ongoing issue in
encoding/json
, whereby if an uninitialised array (such as policies, which is declared asvar p Policies
, or roles, which is declared asvar p Roles
) is attempted to be encoded usingjson.Marshal
, it results in anil
value being returned instead of[]
.Given that json doesn't appear to be merging the currently open PR to add an option to return nil as [] or {} in when json.Marshal is used anytime soon, this patch should fix that.
I've updated the tests in filter_test.go and filter_helper_test.go to replace the expected nil values with the relevant empty Role/Policy object.
Related issue
#217
golang/go#27589
Proposed changes
Checklist
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
security@ory.sh) from the maintainers to push
the changes.
works.
Further comments
Below is the tracing process that I posted on #217
I've been tracing this bug because it also affects us at work, and I've traced it to this
func (h *Handler) List(factory func(context.Context, *http.Request, httprouter.Params) (*ListRequest, error)) httprouter.Handle
instorage/handler.go
returns ah.h.Write(w, r, l.Filter(m).Value)
, and in this case,l.Filter(m).Value
returns*storage.Policies
, which is declared as a type of []Policy .h
, in this case, which is a type ofherodot.Writer
, an interface, in this particular function, is aherodot.JSONWriter
, andWrite
in this case callsWriteCode
here. This tells me that there's an issue withencoding/json
'sjson.Marshal(e)
, wheree
refers tol.Filter(m).Value
.I believe this is due to golang/go#27589, and a proposal has been made to modify
encoding/json
that is still under going review here and awaiting a PR at golang/go#37711.At this point, I'm wondering if I should submit a PR to
ory/herodot
, to do the checking infunc WriteCode
. If it is a shared library, do you think that this will affect other projects that ory has?There's a quick fix, as can be seen from CosmWasm/wasmvm#75, to quickly check the value, and if it's nil, do not even proceed with
JSON.Marshal
. This might actually be the fastest way to fix this, but I'm unsure about the potential impact, given that herodot is a shared repository.Ultimately, the issue lies in
func ListByQuery
instorage/handler.go
, which declares var res Roles and var res Policies without explicitly initialising them, as well asfunc rolesList
andfunc policiesList
inengine/ladon/handler.go
, which does the same.