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.

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 .indexInBrackets array serialization option for URL parameters #3516

Merged

Conversation

TiagoMaiaL
Copy link
Contributor

@TiagoMaiaL TiagoMaiaL commented Nov 5, 2021

Issue Link 🔗

Goals ⚽

There is no real specification for how to encode URL parameters like arrays and dictionaries.

For ["foo": ["a", 1, true, ["bar": 2], ["qux": 3], ["quy": ["quz": 3]]]]

  1. default .brackets encode is:
    foo[]=a&foo[]=1&foo[]=1&foo[][bar]=2&foo[][qux]=3&foo[][quy][quz]=3

  2. .noBrackets encode is:
    foo=a&foo=1&foo=1&foo[bar]=2&foo[qux]=3&foo[quy][quz]=3

  3. jQuery >=1.4 and Node.js package query-string provide index style:
    foo[0]=a&foo[1]=1&foo[2]=1&foo[3][bar]=2&foo[4][qux]=3&foo[5][quy][quz]=3

Implementation Details 🚧

A new ArrayEncoding enumeration case is added to both URLEncoding and URLEncodedFormEncoder. The cases now are .brackets and .noBrackets and .indexInBrackets. The arrayEncoding option passed to the initializer is used to control the array key serialization in URLEncodedFormEncoder and URLEncoding.

Testing Details 🔍

New tests were added to the ParameterEncoderTests and ParameterEncodingTests suites. They follow the same naming convention but with an appended WithIndexInBrackets suffix.

Note:

This PR continues the work started at #3015.

@TiagoMaiaL TiagoMaiaL force-pushed the feature/indexed-array-serialization branch from 95eb534 to b085ad3 Compare November 5, 2021 02:33
@TiagoMaiaL TiagoMaiaL changed the title Feature/indexed array serialization Add .indexInBrackets array serialization to URL parameters Nov 5, 2021
@TiagoMaiaL TiagoMaiaL changed the title Add .indexInBrackets array serialization to URL parameters Add .indexInBrackets array serialization for URL parameters Nov 5, 2021
@TiagoMaiaL TiagoMaiaL changed the title Add .indexInBrackets array serialization for URL parameters Add .indexInBrackets array serialization option for URL parameters Nov 5, 2021
Copy link
Contributor

@jshier jshier left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, this looks great! Just a few doc formatting suggestions and some naming and this should be all set.

Source/ParameterEncoding.swift Outdated Show resolved Hide resolved
Source/ParameterEncoding.swift Outdated Show resolved Hide resolved
Source/URLEncodedFormEncoder.swift Outdated Show resolved Hide resolved
Source/URLEncodedFormEncoder.swift Outdated Show resolved Hide resolved
Copy link
Contributor

@jshier jshier left a comment

Choose a reason for hiding this comment

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

I think you missed one use site of the updated naming. Once that's updated I can trigger the test suite.

Source/URLEncodedFormEncoder.swift Outdated Show resolved Hide resolved
@TiagoMaiaL TiagoMaiaL force-pushed the feature/indexed-array-serialization branch from fac8ac5 to db327e3 Compare November 5, 2021 03:08
Copy link
Contributor

@jshier jshier left a comment

Choose a reason for hiding this comment

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

👍

@jshier jshier merged commit 92c7683 into Alamofire:master Nov 5, 2021
@jshier jshier added this to the 5.5.0 milestone Dec 13, 2021
jshier pushed a commit that referenced this pull request Jan 15, 2022
…3516)

* Add the indexInBrackets option to URLEncoding

* Add the indexInBrackets option to URLEncodedFormEncoder

* Add .indexInBrackets tests for structs and classes

* Address PR comments regarding .indexInBrackets serialization
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants