Reuse the container when requesting for a new nested container with the old key in JSONEncoder and PlistEncoder #20951

pitiphong-p · 2018-12-02T12:17:30Z

At the moment, JSONEncoder and PlistEncoder always create a new container when asking for nested keyed container and nested unkeyed container. It works fine for most of the case. But this causes a bug in the case when a type may need to ask for 2 separate nested containers for the same key.

This PR change the logic of both encoders to check for the existing container and reuse it if one found, otherwise will create a new container like the old behavior.

Resolves SR-9385.

stdlib/public/SDK/Foundation/PlistEncoder.swift

itaiferber

Thanks for putting this together! I think this is pretty close — we could probably rephrase the precondition message to be a little bit more direct (suggestion inline), and a few tests to ensure the precondition is being tested would be good to have.

After this, we'll need to port the changes over to swift-corelibs-foundation as well.

itaiferber · 2018-12-03T16:27:49Z

stdlib/public/SDK/Foundation/JSONEncoder.swift

-        let dictionary = NSMutableDictionary()
-        self.container[_converted(key).stringValue] = dictionary
+        let containerKey = _converted(key).stringValue
+        let existingContainer = self.container[containerKey]


I feel that on the whole, this might be more idiomatically expressed as something like

let containerKey = _converted(key).stringValue let dictionary: NSMutableDictionary if let existingContainer = self.container[containerKey] { precondition(existingContainer is NSMutableDictionary, "Attempt to re-encode into nested KeyedEncodingContainer<\(Key.self)> for key \"\(containerKey)\" is invalid: non-keyed container already encoded for this key") dictionary = existingContainer as! NSMutableDictionary } else { dictionary = NSMutableDictionary() self.container[containerKey] = dictionary }

Same for plist

itaiferber · 2018-12-03T16:30:29Z

stdlib/public/SDK/Foundation/JSONEncoder.swift

@@ -510,8 +514,12 @@ fileprivate struct _JSONKeyedEncodingContainer<K : CodingKey> : KeyedEncodingCon
    }

    public mutating func nestedUnkeyedContainer(forKey key: Key) -> UnkeyedEncodingContainer {
-        let array = NSMutableArray()
-        self.container[_converted(key).stringValue] = array
+        let containerKey = _converted(key).stringValue


Similarly, I think we can match here with

let containerKey = _converted(key).stringValue let array: NSMutableArray if let existingContainer = self.container[containerKey] { precondition(existingContainer is NSMutableArray, "Attempt to re-encode into nested UnkeyedEncodingContainer for key \"\(containerKey)\" is invalid: keyed container/single value already encoded for this key") array = existingContainer as! NSMutableArray } else { array = NSMutableArray() self.container[containerKey] = array }

I also thought about this approach too but just end up with the one in PR. Will update to this style

itaiferber · 2018-12-03T16:32:28Z

test/stdlib/TestJSONEncoder.swift

+      }
+    }
+
+    enum TopLevelCodingKeys : String, CodingKey {


Can these key types be declared within Model itself? Just as a scoping thing

itaiferber · 2018-12-03T16:38:50Z

test/stdlib/TestJSONEncoder.swift

@@ -1615,6 +1671,7 @@ JSONEncoderTests.test("testEncodingTopLevelStructuredSingleClass") { TestJSONEnc
 JSONEncoderTests.test("testEncodingTopLevelDeepStructuredType") { TestJSONEncoder().testEncodingTopLevelDeepStructuredType()}
 JSONEncoderTests.test("testEncodingClassWhichSharesEncoderWithSuper") { TestJSONEncoder().testEncodingClassWhichSharesEncoderWithSuper() }
 JSONEncoderTests.test("testEncodingTopLevelNullableType") { TestJSONEncoder().testEncodingTopLevelNullableType() }
+JSONEncoderTests.test("testEncodingMultipleNestedContainersWithTheSameTopLevelKey") { TestJSONEncoder().testEncodingMultipleNestedContainersWithTheSameTopLevelKey() }


We should add tests to ensure that re-encoding into different containers fails. Each test runs in its own process, so the expected fatal error will not take down the rest of the tests. I believe that the right way to assert that we expect a test to crash is JSONEncoderTests.test("<test name here>").xfail(.always).code { TestJSONEncoder().<whatever> }

I didn’t know this trick. I manually tested it and wait for it to fail then delete the code. This is great!!! I’ll update my test for this

If you want it to crash, you shouldn't XFAIL it at all. You should use expectCrash(withMessage: "message goes here") { … } in the body of the test.

@jrose-apple Wasn't aware of that — is that new?
@pitiphong-p Sorry to have given bad info; we should probably do what Jordan suggests instead of XFAIL'ing the test.

Nope, it's been around! There just aren't that many crash tests, I guess.

pitiphong-p · 2018-12-04T08:30:08Z

@itaiferber Already change as you requested. Please have a look again :)

itaiferber

A few small leftover nits but the rest looks great! If you're willing to make these final tiny changes, I think we'll be good to go.

itaiferber · 2018-12-04T17:11:31Z

test/stdlib/TestJSONEncoder.swift

+    }
+  }
+
+  func testEncodingConfilictedTypeNestedContainersWithTheSameTopLevelKey() {


Nit: small typo — s/Confilicted/Conflicted

itaiferber · 2018-12-04T17:12:39Z

test/stdlib/TestJSONEncoder.swift

+  }
+
+  func testEncodingConfilictedTypeNestedContainersWithTheSameTopLevelKey() {
+    struct Model : Codable, Equatable {


Since only the encode side is being tested here, we can simplify this by dropping the Decodable side of things and SecondNestedCodingKeys

itaiferber · 2018-12-04T17:14:14Z

test/stdlib/TestJSONEncoder.swift

+        var firstNestedContainer = container.nestedContainer(keyedBy: FirstNestedCodingKeys.self, forKey: .top)
+        try firstNestedContainer.encode(self.first, forKey: .first)
+
+        var secondNestedContainer = container.nestedUnkeyedContainer(forKey: .top)


It would be good to explicitly comment here (and below before the _testRoundTrip to indicate that this access is invalid, and that we expect this to assert (and that the test is explicitly xfail'd). (The xfail comment below covers this well, but for someone just looking at the test, the behavior alone is confusing unless they know to scroll all the way down to look for the xfail)

itaiferber · 2018-12-04T17:14:33Z

test/stdlib/TestPlistEncoder.swift

+  }
+
+  func testEncodingConfilictedTypeNestedContainersWithTheSameTopLevelKey() {
+    struct Model : Codable, Equatable {


Same typo and Decodable comments here

test/stdlib/TestPlistEncoder.swift

test/stdlib/TestJSONEncoder.swift

pitiphong-p · 2018-12-05T06:14:44Z

@itaiferber @xwu Thank you. I added and changed as you ask already. Please have a look :)

itaiferber · 2018-12-07T18:18:29Z

@pitiphong-p Everything looks good to me, short of switching over to Jordan's suggestion to expectCrash. Looks like we have some merge conflicts now though 😕 If we can resolve those I'll kick off the tests.

pitiphong-p · 2018-12-08T06:35:47Z

@itaiferber OK I'll rebase the branch against the current develop branch and fix the test as Jordan's suggestion

…he Same Top Level Key case Fixed test

…with the same key in JSONEncoder

…with the same key in PlistEncoder

…ested container with the same key

… message on the condition

…ed mismatched container type

pitiphong-p · 2018-12-08T09:51:31Z

@itaiferber @jrose-apple I rebased my branch against the current master to fix the conflicted already. Also change the test to use the expectCrash instead of the xfail too. However I can't manage to get the expectCrash(withMessage: ) variant to success. It always said that it can't find the expected message

Please have a look and feel free to give me an advice on the issue related to expectCrash(withMessage: )

jrose-apple · 2018-12-10T17:20:03Z

What was the message you were trying to match?

pitiphong-p · 2018-12-11T06:16:34Z

stderr>>> FAIL: saw expected "crashed: sigill", but without message "Attempt to re-encode into nested UnkeyedEncodingContainer for key "top" is invalid: keyed container/single value already encoded for this key" before it

This is a message I got on the failed test (which should succeed). I was trying to find the stderr but couldn't find one. So I'm not sure what's the full message that was thrown

jrose-apple · 2018-12-11T17:03:21Z

That definitely sounds like it should work. You should be able to see the full crash and stderr by running the command printed at the bottom of the StdlibUnittest output, the one that has --stdlib-unittest-in-process in it.

pitiphong-p · 2018-12-12T04:52:49Z

StdlibUnittest: using filter: testEncodingConflictedTypeNestedContainersWithTheSameTopLevelKey
[ RUN ] TestJSONEncoder.testEncodingConflictedTypeNestedContainersWithTheSameTopLevelKey
STDLIB_UNITTEST;expectCrash;false
STDLIB_UNITTEST;expectCrash;Attempt to re-encode into nested UnkeyedEncodingContainer for key "top" is invalid: keyed container/single value already encoded for this key
Illegal instruction: 4

@jrose-apple Thank you for the trick. Above is the output from my console with the given option

jrose-apple · 2018-12-12T17:18:40Z

That…definitely seems like the message is there. Hm.

pitiphong-p · 2018-12-13T05:14:24Z

@jrose-apple Any clue that I can use for further investigation?

itaiferber · 2018-12-13T19:46:17Z

@pitiphong-p Is it sufficient to just leave the crash message empty for now? I don't think we particularly care about the exact wording of the message, and if it allows the test to pass, that might be enough.

jrose-apple · 2018-12-13T20:09:44Z

I'm unhappy that we don't know why it's not working, but yeah, that is an option.

pitiphong-p · 2018-12-13T20:59:23Z

@itaiferber it’s sufficient to leave the crash message empty at the moment. The test will pass.

Also I already leave the crash message empty at the moment. You can trigger Swift CI to run test if you want to

itaiferber · 2018-12-13T21:01:31Z

Let's just get this testing and I'll see if I can get some time to dig into why the crash message doesn't work.

itaiferber · 2018-12-13T21:01:38Z

@swift-ci Please test macOS

itaiferber · 2018-12-13T21:01:45Z

@swift-ci Please smoke test Linux

pitiphong-p · 2018-12-20T17:31:14Z

Hi @itaiferber, sorry to ping you here but the tests are all passed so how should we proceed on this PR

itaiferber · 2018-12-21T20:47:51Z

@jrose-apple Do you have any further objections to merging this now and investigating the crash message separately?

jrose-apple · 2018-12-21T21:11:14Z

No. I mostly just don't want to lose it!

pitiphong-p · 2019-02-04T06:04:35Z

Hi, @itaiferber @jrose-apple, any update on this PR? Sorry for the ping though

jrose-apple · 2019-02-04T17:38:27Z

*sigh* I guess we both thought the other was going to merge it. Let me run the tests again, since it's been a month and a half!

@swift-ci Please test

itaiferber · 2019-02-04T21:38:28Z

@pitiphong-p Sorry about that! I had planned on merging but this was right before vacation, and it slipped through the cracks. My fault.

@jrose-apple Thanks for merging :)

pitiphong-p · 2019-02-05T04:18:35Z

@itaiferber @jrose-apple Thank both of you for helping me on this PR 😄

One more question, should I open another PR onto the swift-5?

jrose-apple · 2019-02-05T22:16:00Z

I think we've missed the boat on that at this point. The release managers are pretty much only taking very bad regressions and ABI issues.

pitiphong-p · 2019-02-06T04:27:16Z

OK I got it. Thank you again :)

- https://bugs.swift.org/browse/SR-9385 - apple/swift#20951

xwu reviewed Dec 2, 2018

View reviewed changes

stdlib/public/SDK/Foundation/PlistEncoder.swift Outdated Show resolved Hide resolved

stdlib/public/SDK/Foundation/PlistEncoder.swift Outdated Show resolved Hide resolved

xwu reviewed Dec 2, 2018

View reviewed changes

stdlib/public/SDK/Foundation/PlistEncoder.swift Outdated Show resolved Hide resolved

xwu reviewed Dec 2, 2018

View reviewed changes

stdlib/public/SDK/Foundation/PlistEncoder.swift Outdated Show resolved Hide resolved

xwu reviewed Dec 2, 2018

View reviewed changes

stdlib/public/SDK/Foundation/PlistEncoder.swift Outdated Show resolved Hide resolved

pitiphong-p force-pushed the encoders-nested-key-edge-case-bug branch from 0953217 to 781b797 Compare December 3, 2018 05:06

xwu reviewed Dec 3, 2018

View reviewed changes

stdlib/public/SDK/Foundation/PlistEncoder.swift Outdated Show resolved Hide resolved

itaiferber self-requested a review December 3, 2018 16:23

itaiferber suggested changes Dec 3, 2018

View reviewed changes

itaiferber reviewed Dec 4, 2018

View reviewed changes

xwu reviewed Dec 4, 2018

View reviewed changes

test/stdlib/TestPlistEncoder.swift Outdated Show resolved Hide resolved

test/stdlib/TestJSONEncoder.swift Outdated Show resolved Hide resolved

pitiphong-p added 11 commits December 8, 2018 13:39

Write the test cases of the Encoding Multiple NestedContainers with T…

a0272f9

…he Same Top Level Key case Fixed test

Look up and reuse the container when asking for the nested container …

32ef877

…with the same key in JSONEncoder

Look up and reuse the container when asking for the nested container …

1750e04

…with the same key in PlistEncoder

Precondition for the mismatch container type when requesing for the n…

9a3038d

…ested container with the same key

Fixed the wrong indentation

c7a2b31

Fixed the wrong indentation due to the GitHub editor

80056e2

Change the tab character to be space for the wrong indentation

d61830b

Refactor the looking for existing container code and change the error…

a75d98f

… message on the condition

Added: Expected to failed test cases on requesting an already request…

8097d34

…ed mismatched container type

NIT fixing and add some comments in the test methods

02df514

Uses the expectCrash instead of xfail in the Encoder Tests

61238ed

pitiphong-p force-pushed the encoders-nested-key-edge-case-bug branch from a519e49 to 61238ed Compare December 8, 2018 09:48

itaiferber approved these changes Dec 21, 2018

View reviewed changes

jrose-apple self-assigned this Feb 4, 2019

This comment has been minimized.

Sign in to view

jrose-apple merged commit b0f5815 into apple:master Feb 4, 2019

pitiphong-p deleted the encoders-nested-key-edge-case-bug branch February 5, 2019 04:16

nearfri added a commit to nearfri/ObjectCoder that referenced this pull request Jan 5, 2020

Resolves SR-9385.

72e6454

- https://bugs.swift.org/browse/SR-9385 - apple/swift#20951

pitiphong-p mentioned this pull request Dec 2, 2018

[SR-9385] Encoded data was lost when trying to encode with 2 separated nested keys container with 2 difference Coding Keys on the same key with JSONEncoder and PListEncoder #51851

Closed

Reuse the container when requesting for a new nested container with the old key in JSONEncoder and PlistEncoder #20951

Reuse the container when requesting for a new nested container with the old key in JSONEncoder and PlistEncoder #20951

Conversation

pitiphong-p commented Dec 2, 2018

itaiferber 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

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

pitiphong-p commented Dec 4, 2018

itaiferber 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

pitiphong-p commented Dec 5, 2018

itaiferber commented Dec 7, 2018 • edited

pitiphong-p commented Dec 8, 2018

pitiphong-p commented Dec 8, 2018

jrose-apple commented Dec 10, 2018

pitiphong-p commented Dec 11, 2018

jrose-apple commented Dec 11, 2018

pitiphong-p commented Dec 12, 2018

jrose-apple commented Dec 12, 2018

pitiphong-p commented Dec 13, 2018

itaiferber commented Dec 13, 2018

jrose-apple commented Dec 13, 2018

pitiphong-p commented Dec 13, 2018

itaiferber commented Dec 13, 2018

itaiferber commented Dec 13, 2018

itaiferber commented Dec 13, 2018

pitiphong-p commented Dec 20, 2018

itaiferber commented Dec 21, 2018

jrose-apple commented Dec 21, 2018

pitiphong-p commented Feb 4, 2019 • edited

jrose-apple commented Feb 4, 2019

This comment has been minimized.

itaiferber commented Feb 4, 2019

pitiphong-p commented Feb 5, 2019 • edited

jrose-apple commented Feb 5, 2019

pitiphong-p commented Feb 6, 2019

itaiferber commented Dec 7, 2018 •

edited

pitiphong-p commented Feb 4, 2019 •

edited

pitiphong-p commented Feb 5, 2019 •

edited