Skip to content

Implementing jsonObject with streams api of NSJSONSerialization. #466

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 9, 2017
Merged

Implementing jsonObject with streams api of NSJSONSerialization. #466

merged 1 commit into from
Mar 9, 2017

Conversation

mamabusi
Copy link
Contributor

No description provided.

mamabusi
mamabusi reviewed Jul 22, 2016
View reviewed changes
Foundation/NSJSONSerialization.swift Outdated
@@ -125,8 +125,8 @@ public class JSONSerialization : NSObject {
The data must be in one of the 5 supported encodings listed in the JSON specification: UTF-8, UTF-16LE, UTF-16BE, UTF-32LE, UTF-32BE. The data may or may not have a BOM. The most efficient encoding to use for parsing is UTF-8, so if you have a choice in encoding the data passed to this method, use UTF-8.
*/
/// - Experiment: Note that the return type of this function is different than on Darwin Foundation (Any instead of AnyObject). This is likely to change once we have a more complete story for bridging in place.
public class func jsonObject(with data: Data, options opt: ReadingOptions = []) throws -> Any {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implementation NSJSONSerialization.jsonObject(with: stream) internally needs to call the api NSJSONSerialization.jsonObject(with: data). Currently, return type of NSJSONSerialization.jsonObject(with: data) returns 'Any' instead of 'AnyObject' and it is not possible to cast 'Any' to 'AnyObject'.

Wrote a query to the mailing list regarding the above hiccup. Following is the response from Philippe Hausler:
"We had changed the return type of JSONSerialization to be Any such that it avoided the caller needing to bridge types (which works poorly on linux). Even though this is an API change technically, just change the return type to match the other JSONSerialization methods."

@phausler: In accordance with the above suggestion, modified the return type and the related code (including the existing test cases) to resolve errors caused by the change.

@parkera
Copy link
Contributor

parkera commented Jul 28, 2016

I think we wound up with two implementations of the JSON stream methods. @mamabusi , I like the test cases from this - can you rebase vs master so we can merge this?

@mamabusi mamabusi force-pushed the nsjsonser-branch branch 2 times, most recently from 06d0314 to 4b857b9 Compare July 28, 2016 06:19
@mamabusi
Copy link
Contributor Author

@parkera , I see NSJSONSerialization.writeObject(with streams) implementation is merged but not NSJSONSerialization.jsonObject(with streams). This PR implements the latter.
Is there an implementation already for this as well?

I have resolved the conflicts. Please review.
Thanks!

@mamabusi
Copy link
Contributor Author

mamabusi commented Aug 1, 2016

Rebased to resolve the conflicts.

@mamabusi mamabusi force-pushed the nsjsonser-branch branch 3 times, most recently from 9d0e640 to 5fc7041 Compare August 25, 2016 12:08
@mamabusi
Copy link
Contributor Author

mamabusi commented Sep 8, 2016

@parkera This PR implements an unimplemented method in NSJSONSerialization. Could you please review.

phausler
phausler reviewed Sep 8, 2016
View reviewed changes
Foundation/NSJSONSerialization.swift Outdated
data.append(&buffer, count: bytesRead!)
}
}
} while bytesRead == buffer.count
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this condition be while stream.status == .open || stream.status == .reading?

@parkera
Copy link
Contributor

parkera commented Sep 28, 2016

@swift-ci please test

@mamabusi
Copy link
Contributor Author

streamError implementation is now available. Will work on the review comments and resolution of conflicts.

@mamabusi mamabusi force-pushed the nsjsonser-branch branch 2 times, most recently from 116270c to 54a6a33 Compare March 1, 2017 09:07
@mamabusi
Copy link
Contributor Author

mamabusi commented Mar 1, 2017

@parkera @phausler Have addressed the previous review comments. Please have a look.
Thanks!

parkera
parkera requested changes Mar 5, 2017
View reviewed changes
Foundation/NSJSONSerialization.swift Outdated
open class func jsonObject(with stream: InputStream, options opt: ReadingOptions = []) throws -> Any {
var data = Data()
guard stream.streamStatus == .open || stream.streamStatus == .reading else {
throw NSError(domain: NSCocoaErrorDomain, code: CocoaError.propertyListReadCorrupt.rawValue, userInfo: [
Copy link
Contributor

Choose a reason for hiding this comment

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

This should actually be fatalError to match the Darwin behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok..

Foundation/NSJSONSerialization.swift Outdated
])
}
repeat {
var buffer = [UInt8](repeating: 0, count: 1024)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to use a Data here instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to use Data instead of the buffer here, but got stuck at the point where I am supposed to send in the UnsafeMutablePointer and an Int of maxLength to read the contents of the stream:

var dataChunk = Data()
try dataChunk.withUnsafeMutableBytes {(bytes: UnsafeMutablePointer)->Void in
bytesRead = stream.read(bytes, maxLength: ??)

Is there a way I could read the InputStream to Data without the buffer?

TestFoundation/TestNSJSONSerialization.swift Outdated
do {
try FileManager.default.removeItem(atPath: location)
} catch _ {

}
}
}

//MARK: - JSONObjectWithStreams
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I appreciate the extended test coverage here - but there has got to be a better way than copy/pasting all the existing test cases and just passing a stream with the same content.

Can we perhaps make a helper function which tests both data and stream versions and modify the existing tests?

Copy link
Contributor Author

@mamabusi mamabusi Mar 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Sure, will try and reduce the redundancy.

@mamabusi mamabusi force-pushed the nsjsonser-branch branch 2 times, most recently from 0878f80 to 251e7d2 Compare March 9, 2017 07:10
@mamabusi
Copy link
Contributor Author

mamabusi commented Mar 9, 2017

@parkera Have addressed the changes requested.
Thanks!

@parkera
Copy link
Contributor

parkera commented Mar 9, 2017

Thanks!

@parkera
Copy link
Contributor

parkera commented Mar 9, 2017

@swift-ci test and merge

@swift-ci swift-ci merged commit 7dbef6d into swiftlang:master Mar 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@parkera parkera parkera approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mamabusi @parkera @phausler @swift-ci