-
-
Notifications
You must be signed in to change notification settings - Fork 53
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
Add support for array indices in query params decoding #542
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #542 +/- ##
==========================================
- Coverage 84.86% 83.77% -1.10%
==========================================
Files 98 99 +1
Lines 5320 4413 -907
==========================================
- Hits 4515 3697 -818
+ Misses 805 716 -89 ☔ View full report in Codecov by Sentry. |
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.
In general looks good. I've added some comments about decoding the integers.
@@ -84,6 +84,11 @@ enum URLEncodedFormNode: CustomStringConvertible, Equatable { | |||
// currently don't support arrays and maps inside arrays | |||
throw Error.notSupported | |||
} | |||
case (.array(let array), .arrayWithIndices(let index)): | |||
guard keys.count == 0, array.values.count == index else { | |||
throw Error.notSupported |
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.
Maybe we should add a new error here. Error.invalidArrayIndex(Int)
values.append(.arrayWithIndices(arrayIndex)) | ||
} else { | ||
values.append(.array) | ||
} | ||
index = key.index(after: index) | ||
} else { | ||
// an open bracket is unexpected |
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.
Instead of parsing the array index above you should do it here once we have realised the next character isn't a ]
. You could also replace the values.append(.map(key[index..<bracketIndex]))
call below with the following to do the integer conversion.
// If key can convert to an integer assume it is an array index
if let index = Int(key[index..<bracketIndex]) {
values.append(.arrayWithIndices(index))
} else {
values.append(.map(key[index..<bracketIndex]))
}
let decoder = URLEncodedFormDecoder() | ||
// incorrect indices | ||
let query = "arr[0]=2?arr[2]=4" | ||
XCTAssertThrowsError(try decoder.decode(Test.self, from: query)) |
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.
You should verify what error is thrown here.
Thanks @adam-fowler for the review. I updated the PR, so please take a look when you find the time. |
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.
This looks great, thank you
This PR tries to resolve the following issue.
With the proposed implementation hummingbird will be able to decode arrays which contain indices from query parameters.
e.g.
arr[0]=23&arr[1]=34&arr[2]=4
The proposed implementation only allows such arrays if the indices are sent in an array of integers which increase by 1, meaning the indices must come in the strict order of
0, 1, 2, 3..
etc. Also we allow only ASCII numbers to be used as indices hence the need for custom extension forCharacter
.Otherwise the decoding will fail.