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 configurable parameter handling to URLEncoding. #2431

Merged
merged 4 commits into from Feb 20, 2018

Conversation

Projects
None yet
3 participants
@heiberg
Contributor

heiberg commented Feb 8, 2018

Goals ⚽️

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

For arrays, two conventions are in common use:

  1. foo=[1,2] is encoded as foo[]=1&foo[]=2
  2. foo=[1,2] is encoded as foo=1&foo=2

Alamofire follows the first convention, as does libraries such as jQuery and Rails.

The second convention is not currently supported by Alamofire, but is commonly required for communicating with Java-based web services.

This PR introduces optional support for the convention where square brackets are omitted
from array keys.

Implementation Details 🚧

  • A new ArrayEncoding enumeration is added to URLEncoding. The cases are .brackets and .noBrackets.
  • An arrayEncoding parameter is added to URLEncoding.init with a default value of .brackets to preserve the existing default behavior and maintain source compatibility.
  • The arrayEncoding passed to init is used to control the array key serialization in URLEncoding.queryComponents.

Testing Details 🔍

The two existing tests for URL encoding of arrays are:

  • testURLParameterEncodeStringKeyArrayValueParameter (top level array)
  • testURLParameterEncodeStringKeyNestedDictionaryArrayValueParameter (array nested inside a dictionary)

These have been duplicated and adapted to test an instance of URLEncoding(arrayEncoding: .noBrackets) instead of the default URLEncoding.

The two new tests are named like the existing tests, with an appended suffix:

  • testURLParameterEncodeStringKeyArrayValueParameterWithoutBrackets
  • testURLParameterEncodeStringKeyNestedDictionaryArrayValueParameterWithoutBrackets

All tests pass.


Update

Amended the pull request with configurable boolean parameter encoding, as per @jshier 's comment.

  • The bool value configuration follows the pattern of the array key configuration closely for consistency.
  • Source compatibility is maintained and the default bool encoding behavior is preserved.

Added the following test case:

testURLParameterLiteralBoolEncodingWorksAndDoesNotAffectNumbers

The test verifies that

  • Boolean values can be encoded to literals as expected.
  • Enabling literal bool encoding has no effect on the encoding of truthy or falsy numeric parameters like NSNumber(value: 1) etc.
@Vyazovoy

This comment has been minimized.

Vyazovoy commented Feb 8, 2018

👍 really useful PR!

@jshier

This comment has been minimized.

Contributor

jshier commented Feb 8, 2018

We’ve wanted this for a while, so thanks! In addition to Array encoding, a Bool encoding has also been requested in the past, 0, 1 vs. literal true/false, so if you want to add that we’d appreciate it. I’m on vacation right now so it may be a little while before a full review.

@heiberg

This comment has been minimized.

Contributor

heiberg commented Feb 9, 2018

Sure! I'll update this with configurable Bool encoding shortly. No rush, have a nice vacation.

@heiberg heiberg changed the title from Add configurable array parameter handling to URLEncoding. to Add configurable parameter handling to URLEncoding. Feb 12, 2018

@jshier

Overall it looks great! Just a few minor changes. And if I could ask a favor, could you update the URL Encoding docs in Usage.md to add these new parameters, I'd appreciate it.

@@ -82,6 +88,23 @@ public struct URLEncoding: ParameterEncoding {
case methodDependent, queryString, httpBody
}
/// Configures how array parameters are encoded.

This comment has been minimized.

@jshier

jshier Feb 18, 2018

Contributor

I think having Array instead of array makes it a bit more clear.

case brackets, noBrackets
}
/// Configures how boolean parameters are encoded.

This comment has been minimized.

@jshier

jshier Feb 18, 2018

Contributor

Bool here, like Array.

/// Configures how boolean parameters are encoded.
///
/// - numeric: Encode `true` as 1 and `false` as 0. This is the default behavior.

This comment has been minimized.

@jshier

jshier Feb 18, 2018

Contributor

I think ticks around 1 and 0 are appropriate here.

@@ -99,15 +122,24 @@ public struct URLEncoding: ParameterEncoding {
/// The destination defining where the encoded query string is to be applied to the URL request.
public let destination: Destination
/// The convention for encoding array parameters.

This comment has been minimized.

@jshier

jshier Feb 18, 2018

Contributor

It's a bit clearer to just say "The encoding to use for Array parameters."

public let arrayEncoding: ArrayEncoding
/// The convention for encoding boolean parameters.
public let boolEncoding: BoolEncoding

This comment has been minimized.

@jshier

jshier Feb 18, 2018

Contributor

As with arrayEncoding, it's a bit clearer to say "The encoding to use for Bool parameters."

@@ -261,6 +293,24 @@ public struct URLEncoding: ParameterEncoding {
return false
}
}
private func encodeArrayKey(_ key: String) -> String {

This comment has been minimized.

@jshier

jshier Feb 18, 2018

Contributor

It would be cleaner if this was simply an encode function on ArrayEncoding. Better encapsulation that way.

}
}
private func encodeBoolValue(_ value: Bool) -> String {

This comment has been minimized.

@jshier

jshier Feb 18, 2018

Contributor

Like the array encoding function, this should be an encode function on BoolEncoding.

@@ -161,16 +193,16 @@ public struct URLEncoding: ParameterEncoding {
}
} else if let array = value as? [Any] {
for value in array {
components += queryComponents(fromKey: "\(key)[]", value: value)
components += queryComponents(fromKey: encodeArrayKey(key), value: value)

This comment has been minimized.

@jshier

jshier Feb 18, 2018

Contributor

Moving the encode functionality to ArrayEncoding would make this arrayEncoding.encode(key), which I think is much clearer.

}
} else if let value = value as? NSNumber {
if value.isBool {
components.append((escape(key), escape((value.boolValue ? "1" : "0"))))
components.append((escape(key), escape(encodeBoolValue(value.boolValue))))

This comment has been minimized.

@jshier

jshier Feb 18, 2018

Contributor

With encode on BoolEncoding, this becomes boolEncoding.encode(value.boolValue).

} else {
components.append((escape(key), escape("\(value)")))
}
} else if let bool = value as? Bool {
components.append((escape(key), escape((bool ? "1" : "0"))))
components.append((escape(key), escape(encodeBoolValue(bool))))

This comment has been minimized.

@jshier

jshier Feb 18, 2018

Contributor

With encode on BoolEncoding, this becomes boolEncoding.encode(bool).

@jshier jshier added this to the 4.7.0 milestone Feb 18, 2018

@heiberg

This comment has been minimized.

Contributor

heiberg commented Feb 20, 2018

Excellent suggestions! I added commits to address them.

The only deviation from your suggestions is that I wanted to keep the external parameter name for the encode methods. So the usage becomes arrayEncoding.encode(key: ...) and boolEncoding.encode(value: ...) at the call sites.

One is intended for keys and the other is intended for values, and I think keeping the external parameter name help to differentiate and reduce the risk of mistakes when using them.

@jshier

jshier approved these changes Feb 20, 2018

👍

@jshier jshier merged commit 0456f5b into Alamofire:master Feb 20, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

jshier added a commit that referenced this pull request Mar 5, 2018

Add configurable parameter handling to URLEncoding. (#2431)
* Add configurable array parameter handling to URLEncoding.

* Add configurable boolean parameter handling to URLEncoding.

* Addressing PR feedback for URLEncoding.

* Documentation for BoolEncoding and ArrayEncoding options in URLEncoding.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment