search

gorilla / schema

Package gorilla/schema fills a struct with form values.
https://gorilla.github.io
BSD 3-Clause "New" or "Revised" License
1.39k stars 231 forks source link

fixed panic: reflect: indirection through nil pointer to embedded struct #211

Closed morus12 closed 5 months ago

morus12 commented 7 months ago

What type of PR is this? (check all applicable)

Description

The provided test TestUnmarshallToEmbeddedNoData shows that the decoder panics when setDefaults encounters a nil pointer to an embedded struct. The fix replaces FieldByName with FieldByIndexErr to catch this kind of situation and continue with the next field.

Related Tickets & Documents

Added/updated tests?

Run verifications and test

zak905 commented 7 months ago

Hi @morus12, introducing myself: I have contributed to the default functionality recently

I was able to reproduce the issue, and the solution you proposed fixes it (fixes #213). I think that ignoring the embedded struct if it is nil is an option, but there are also two other alternatives that we can explore:

  1. failing fast and returning an error if we find a nil embedded struct. The embedded struct comes always before its fields in the for loop, so if an error is returned, the panic will be avoided. 
        vCurrent := v.FieldByName(f.name)
    if f.typ.Kind() == reflect.Pointer && f.typ.Elem().Kind() == reflect.Struct && vCurrent.IsNil() {
        return MultiError{f.name:errors.New("unitialized pointer to embedded struct")}
    }
  2. initializing the embedded struct if it is nil - this is the same approach used when doing the decoding as you can see here: https://github.com/gorilla/schema/blob/main/decoder.go#L258. Because the embedded struct comes before its fields in the for loop, we can simply set the value if it's nil here https://github.com/gorilla/schema/blob/main/decoder.go#L108: 
        vCurrent := v.FieldByName(f.name)
    if f.typ.Kind() == reflect.Pointer && f.typ.Elem().Kind() == reflect.Struct && vCurrent.IsNil() {
        vCurrent.Set(reflect.New(vCurrent.Type().Elem()))
    }

    This not only avoids the panic, but also makes sure that any embedded struct fields with the default option tag are handled as well.

From a pure logic perspective, I think your solution makes most sens, but in order to keep in line with what the library does when decoding, I am leaning slightly towards option 2. Let's try to get a second opinion. @AlexVulaj, @jaitaiwan any thoughts ?

davidnewhall commented 7 months ago

Confirmed this fixes the bug in #213.

replace github.com/gorilla/schema => github.com/morus12/schema v0.0.0-20240403005049-3d48d14f37e5

(3d48d14f37e59b5d6645be74d06e425f392d0fde)

morus12 commented 7 months ago

Hi @zak905, nice to meet you!

I agree that the proposed solution to initialize nil pointers to the embedded structs is a better way to fix this case. I did it by iterating over struct fields and allocating nil pointers because this line panics:

vCurrent := v.FieldByName(f.name)

It calls FieldByIndex under the hood, checks for nil pointers there, and panics https://github.com/golang/go/blob/15cec430d75741960829e7e227c1b7c3e1f79114/src/reflect/value.go#L1315C6-L1315C10

zak905 commented 7 months ago

thanks @morus12, and nice to meet you too! It looks good to me. ps: I don't have merge rights, so we'd have to wait for the maintainers.

AlexVulaj commented 6 months ago

This looks good to me - thanks @davidnewhall @zak905 @morus12 for your contributions, testing, and reviewing of this change!

AlexVulaj commented 6 months ago

@morus12 I do see there's a merge conflict here - do you mind resolving that and pushing this back up so I can merge it? Thanks!

morus12 commented 6 months ago

@morus12 I do see there's a merge conflict here - do you mind resolving that and pushing this back up so I can merge it? Thanks!

@AlexVulaj It's ready.

davidnewhall commented 5 months ago

Are we waiting for anything before merging this and cutting a release? Happy to help further if needed.

zak905 commented 5 months ago

It does not seem the case, I think it's good to merge. @AlexVulaj any concerns here ? the branch have been updated as requested.

zak905 commented 5 months ago

@jaitaiwan it seems like Alex is unresponsive nowdays, do you mind taking a look please ?

jaitaiwan commented 5 months ago

Taking a look

jaitaiwan commented 5 months ago

This has been merged, I have no current plans to perform a version release for this repository yet. If this becomes an issue for anyone, I'll be happy to look at doing another release. For now many other gorilla repos require the maintainers attention. Thanks!

hidetatz commented 5 months ago

@jaitaiwan Hi, I want this change to be released as our application is experiencing unit test failure after updating gorilla/schema to 1.3.0 with the same error message. I'd appreciate if you consider creating a new version up.

davidnewhall commented 5 months ago

Hi @jaitaiwan,

If you want to maintain the trust of the community you'd make a new release and remove the bug (that causes panics) from the latest release. Leaving the bug in the latest release for over 2 months (when we had a fix within a week) was already unacceptable to most consumers. Please reconsider.

Thanks!

AlexVulaj commented 4 months ago

Sorry for taking a while to get back to this - other repositories have been having issues as well that required more of our time.

@davidnewhall it looks like this went out as part of v1.4.0