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

[morus12]

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

• Refactor
• Feature
• Bug Fix
• Optimization
• Documentation Update
• Go Version Update
• Dependency Update

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

• Related Issue #
• Closes #

Added/updated tests?

• Yes
• No, and this is why: please replace this line with details on why tests
  have not been included
• I need help with writing tests

Run verifications and test

• make verify is passing
• make test is passing

[zak905]

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]

Confirmed this fixes the bug in #213.

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

(3d48d14)

[morus12]

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]

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]

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

[AlexVulaj]

@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]

@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]

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

[zak905]

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]

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

[jaitaiwan]

Taking a look

[jaitaiwan]

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]

@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]

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!