Add support for sorted keys. #9

jseibert · 2018-05-25T18:41:32Z

Swift on macOS and Linux have different dictionary enumeration algorithms that lead to non-deterministic encoding output, which breaks testing.

Mimicking JSONSerialization (see https://github.com/apple/swift-corelibs-foundation/blob/master/Foundation/JSONSerialization.swift), we must offer support to sort dictionary keys in order to guarantee output consistency.

jseibert · 2018-05-25T18:42:49Z

Note that I have not changed the default encoder implementation. This must be explicitly enable by overriding the contentConfig:

// Fix the key order for the standard URLEncodedFormEncoder
do {
    let encoder = URLEncodedFormEncoder()
    encoder.outputFormatting = .sortedKeys
    contentConfig.use(encoder: encoder, for: .urlEncodedForm)
    contentConfig.use(decoder: URLEncodedFormDecoder(), for: .urlEncodedForm)
}

tanner0101

Changes LGTM, thanks! Just some gardening comments, plus:

test to verify key sorting works (also will help ensure API doesn't break)

tanner0101 · 2018-05-30T00:40:47Z

Sources/URLEncodedForm/Codable/URLEncodedFormEncoder.swift

@@ -11,6 +11,22 @@
 /// See [Mozilla's](https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/POST) docs for more information about
 /// url-encoded forms.
 public final class URLEncodedFormEncoder: DataEncoder {
+    /// The formatting of the output data.
+    public struct OutputFormatting : OptionSet {


JSON coder calls it "writing options". We should probably follow suit.

Maybe we're looking at different things? This code is copied from https://github.com/apple/swift/blob/master/stdlib/public/SDK/Foundation/JSONEncoder.swift where it is called:

/// The output format to produce. Defaults to `[]`. open var outputFormatting: OutputFormatting = []

Small change here:

- OutputFormatting : OptionSet + OutputFormatting: OptionSet

tanner0101 · 2018-05-30T00:41:14Z

Sources/URLEncodedForm/Data/URLEncodedFormSerializer.swift

-    static let `default` = URLEncodedFormSerializer()
+    static let `default` = URLEncodedFormSerializer(sortedKeys: false)
+
+    private var sortedKeys: Bool


Missing comment

tanner0101 · 2018-05-30T00:41:22Z

Sources/URLEncodedForm/Data/URLEncodedFormSerializer.swift

+
+            for (key, val) in elements {
+                let key = try key.urlEncodedFormEncoded()
+                let subdata = try serialize(val, forKey: key)


Any way we can reduce code dupe here?

jseibert · 2018-05-31T15:26:40Z

@tanner0101 ok should be good to go!

jseibert · 2018-07-18T14:52:52Z

@tanner0101 any chance this can ship?

tanner0101

Sorry for the long delay on this. We should try to get it into 1.1.0 tag for this package. Just needs some small code style changes and it will be good to go.

tanner0101 · 2018-09-17T11:34:46Z

Sources/URLEncodedForm/Data/URLEncodedFormSerializer.swift


    /// Serializes the data.
    func serialize(_ URLEncodedFormEncoded: [String: URLEncodedFormData]) throws -> Data {
        var data: [Data] = []
-        for (key, val) in URLEncodedFormEncoded {
+        var elements: [(key: String, value: URLEncodedFormData)]!


We should be able to avoid using an IUO here since the compiler can be sure we set elements w/ the following if / else.

tanner0101 · 2018-09-17T11:36:05Z

Sources/URLEncodedForm/Codable/URLEncodedFormEncoder.swift

+    }
+
+    /// The output format to produce. Defaults to `[]`.
+    public var outputFormatting: OutputFormatting = []


Vapor style is to set all properties in the init method. We try to avoid default properties on members since they can be easy to miss when refactoring.

tanner0101 · 2018-09-17T11:36:24Z

Sources/URLEncodedForm/Codable/URLEncodedFormEncoder.swift

@@ -11,6 +11,22 @@
 /// See [Mozilla's](https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/POST) docs for more information about
 /// url-encoded forms.
 public final class URLEncodedFormEncoder: DataEncoder {
+    /// The formatting of the output data.
+    public struct OutputFormatting : OptionSet {


Small change here:

- OutputFormatting : OptionSet + OutputFormatting: OptionSet

tanner0101 · 2018-09-17T11:36:49Z

Sources/URLEncodedForm/Data/URLEncodedFormSerializer.swift

@@ -32,10 +48,21 @@ final class URLEncodedFormSerializer {

    /// Serializes a `[String: URLEncodedFormData]` at a given key.
    private func serialize(_ dictionary: [String: URLEncodedFormData], forKey key: Data) throws -> Data {
-        let values = try dictionary.map { subKey, value -> Data in
+        var elements: [(key: String, value: URLEncodedFormData)]!


We should be able to remove the IUO here as well.

Add support for sorted keys.

4047df5

tanner0101 reviewed May 30, 2018

View reviewed changes

tanner0101 added the enhancement New feature or request label May 30, 2018

tanner0101 self-assigned this May 30, 2018

Add tests, remove duplicated code, fix sub-dictionary sorting.

aa70c1b

Add to allTests

deae090

tanner0101 reviewed Sep 17, 2018

View reviewed changes

This was referenced Sep 17, 2018

enums doesn't work #10

Closed

Could not convert Decimal to form-urlencoded data. #11

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for sorted keys. #9

Add support for sorted keys. #9

jseibert commented May 25, 2018

jseibert commented May 25, 2018

tanner0101 left a comment

tanner0101 May 30, 2018

jseibert May 31, 2018

tanner0101 Sep 17, 2018

tanner0101 May 30, 2018

tanner0101 May 30, 2018

jseibert commented May 31, 2018

jseibert commented Jul 18, 2018

tanner0101 left a comment

tanner0101 Sep 17, 2018

tanner0101 Sep 17, 2018

tanner0101 Sep 17, 2018

tanner0101 Sep 17, 2018

Add support for sorted keys. #9

Are you sure you want to change the base?

Add support for sorted keys. #9

Conversation

jseibert commented May 25, 2018

jseibert commented May 25, 2018

tanner0101 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jseibert commented May 31, 2018

jseibert commented Jul 18, 2018

tanner0101 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment