Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

馃┕ Fix: improper query/body parsing with embedded structs #2906

Merged
merged 4 commits into from
Mar 17, 2024
Merged

馃┕ Fix: improper query/body parsing with embedded structs #2906

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
13e92bc
added fix for embedded struct field query parsing
devhsoj Mar 9, 2024
d5fa3c8
Merge branch 'main' into fix-query-parser-embedded-struct-fields
gaby Mar 10, 2024
389f9a9
updated ctx parser tests to test embedded struct field parsing
devhsoj Mar 10, 2024
5228ca9
Merge branch 'main' into fix-query-parser-embedded-struct-fields
gaby Mar 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 18 additions & 0 deletions binder/mapping.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,24 @@
structFieldKind := structField.Kind()
// Does the field type equals input?
if structFieldKind != kind {
// Is the field an embedded struct?
if structFieldKind == reflect.Struct {
// Loop over embedded struct fields
for j := 0; j < structField.NumField(); j++ {
structFieldField := structField.Field(j)

Check warning on line 189 in binder/mapping.go

View check run for this annotation

Codecov / codecov/patch

binder/mapping.go#L188-L189 

Added lines #L188 - L189 were not covered by tests

// Can this embedded field be changed?
if !structFieldField.CanSet() {
continue

Check warning on line 193 in binder/mapping.go

View check run for this annotation

Codecov / codecov/patch

binder/mapping.go#L192-L193 

Added lines #L192 - L193 were not covered by tests
}

// Is the embedded struct field type equal to the input?
if structFieldField.Kind() == kind {
return true

Check warning on line 198 in binder/mapping.go

View check run for this annotation

Codecov / codecov/patch

binder/mapping.go#L197-L198 

Added lines #L197 - L198 were not covered by tests
}
}
}
Comment on lines +185 to +201
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The addition of the check for embedded struct fields in the equalFieldType function is a significant improvement to the query parsing mechanism. This change allows for a more accurate and reliable parsing of queries that include embedded structs, directly addressing the issue highlighted in the related problem report.

However, there are a few considerations and potential improvements to ensure the robustness and maintainability of this solution:

  1. Performance Considerations: The nested loop introduced for checking embedded struct fields could potentially impact performance, especially for structs with a large number of fields or deeply nested embedded structs. It might be beneficial to benchmark this change with various struct configurations to understand its impact on performance.

  2. Recursion for Deeply Nested Structs: The current implementation handles one level of embedded structs. If there are structs embedded within embedded structs, this implementation might not correctly identify the field type. Consider using a recursive approach to handle arbitrary levels of nested structs.

  3. Error Handling and Logging: While the current implementation focuses on enhancing the comparison logic, adding error handling or logging for unexpected scenarios (e.g., when reflection operations fail) could improve the robustness and debuggability of the code.

  4. Code Comments and Documentation: Adding detailed comments explaining the logic behind handling embedded structs and the rationale for this approach would enhance the maintainability of the code. Future contributors would benefit from understanding the context and reasoning behind these changes.

Overall, this enhancement is a valuable addition to the system, and with some refinements, it can be further improved to ensure its effectiveness and maintainability.

Consider implementing the suggested improvements to address potential performance concerns, support deeper levels of nested structs, and enhance the maintainability of the code.


continue
}
// Get tag from field if exist
Expand Down
19 changes: 13 additions & 6 deletions ctx_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1358,7 +1358,12 @@ func Test_Ctx_Parsers(t *testing.T) {
// setup
app := New()

type TestEmbeddedStruct struct {
Names []string `query:"names"`
}

type TestStruct struct {
TestEmbeddedStruct
Name string
Class int
NameWithDefault string `json:"name2" xml:"Name2" form:"name2" cookie:"name2" query:"name2" params:"name2" header:"Name2"`
Expand All @@ -1377,36 +1382,37 @@ func Test_Ctx_Parsers(t *testing.T) {
require.Equal(t, 111, testStruct.Class)
require.Equal(t, "bar", testStruct.NameWithDefault)
require.Equal(t, 222, testStruct.ClassWithDefault)
require.Equal(t, []string{"foo", "bar", "test"}, testStruct.TestEmbeddedStruct.Names)
}

t.Run("BodyParser:xml", func(t *testing.T) {
t.Parallel()
withValues(t, func(c Ctx, testStruct *TestStruct) error {
c.Request().Header.SetContentType(MIMEApplicationXML)
c.Request().SetBody([]byte(`<TestStruct><Name>foo</Name><Class>111</Class><Name2>bar</Name2><Class2>222</Class2></TestStruct>`))
c.Request().SetBody([]byte(`<TestStruct><Name>foo</Name><Class>111</Class><Name2>bar</Name2><Class2>222</Class2><Names>foo</Names><Names>bar</Names><Names>test</Names></TestStruct>`))
return c.Bind().Body(testStruct)
})
})
t.Run("BodyParser:form", func(t *testing.T) {
t.Parallel()
withValues(t, func(c Ctx, testStruct *TestStruct) error {
c.Request().Header.SetContentType(MIMEApplicationForm)
c.Request().SetBody([]byte(`name=foo&class=111&name2=bar&class2=222`))
c.Request().SetBody([]byte(`name=foo&class=111&name2=bar&class2=222&names=foo,bar,test`))
return c.Bind().Body(testStruct)
})
})
t.Run("BodyParser:json", func(t *testing.T) {
t.Parallel()
withValues(t, func(c Ctx, testStruct *TestStruct) error {
c.Request().Header.SetContentType(MIMEApplicationJSON)
c.Request().SetBody([]byte(`{"name":"foo","class":111,"name2":"bar","class2":222}`))
c.Request().SetBody([]byte(`{"name":"foo","class":111,"name2":"bar","class2":222,"names":["foo","bar","test"]}`))
return c.Bind().Body(testStruct)
})
})
t.Run("BodyParser:multiform", func(t *testing.T) {
t.Parallel()
withValues(t, func(c Ctx, testStruct *TestStruct) error {
body := []byte("--b\r\nContent-Disposition: form-data; name=\"name\"\r\n\r\nfoo\r\n--b\r\nContent-Disposition: form-data; name=\"class\"\r\n\r\n111\r\n--b\r\nContent-Disposition: form-data; name=\"name2\"\r\n\r\nbar\r\n--b\r\nContent-Disposition: form-data; name=\"class2\"\r\n\r\n222\r\n--b--")
body := []byte("--b\r\nContent-Disposition: form-data; name=\"name\"\r\n\r\nfoo\r\n--b\r\nContent-Disposition: form-data; name=\"class\"\r\n\r\n111\r\n--b\r\nContent-Disposition: form-data; name=\"name2\"\r\n\r\nbar\r\n--b\r\nContent-Disposition: form-data; name=\"class2\"\r\n\r\n222\r\n--b\r\nContent-Disposition: form-data; name=\"names\"\r\n\r\nfoo\r\n--b\r\nContent-Disposition: form-data; name=\"names\"\r\n\r\nbar\r\n--b\r\nContent-Disposition: form-data; name=\"names\"\r\n\r\ntest\r\n--b--")
c.Request().SetBody(body)
c.Request().Header.SetContentType(MIMEMultipartForm + `;boundary="b"`)
c.Request().Header.SetContentLength(len(body))
Expand All @@ -1416,14 +1422,14 @@ func Test_Ctx_Parsers(t *testing.T) {
t.Run("CookieParser", func(t *testing.T) {
t.Parallel()
withValues(t, func(c Ctx, testStruct *TestStruct) error {
c.Request().Header.Set("Cookie", "name=foo;name2=bar;class=111;class2=222")
c.Request().Header.Set("Cookie", "name=foo;name2=bar;class=111;class2=222;names=foo,bar,test")
return c.Bind().Cookie(testStruct)
})
})
t.Run("QueryParser", func(t *testing.T) {
t.Parallel()
withValues(t, func(c Ctx, testStruct *TestStruct) error {
c.Request().URI().SetQueryString("name=foo&name2=bar&class=111&class2=222")
c.Request().URI().SetQueryString("name=foo&name2=bar&class=111&class2=222&names=foo,bar,test")
return c.Bind().Query(testStruct)
})
})
Expand All @@ -1443,6 +1449,7 @@ func Test_Ctx_Parsers(t *testing.T) {
c.Request().Header.Add("name2", "bar")
c.Request().Header.Add("class", "111")
c.Request().Header.Add("class2", "222")
c.Request().Header.Add("names", "foo,bar,test")
return c.Bind().Header(testStruct)
})
})
Expand Down