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.

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

Update JSON unknown field support #771

Merged
merged 7 commits into from Jul 24, 2018
Merged

Update JSON unknown field support #771

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
dd141c5
Fix typo in test field names.
thomasvl Jul 17, 2018
03eb4de
Support erroring on unknown fields during JSON parse.
thomasvl Jul 17, 2018
f24881e
Add a specific test around JSON unknown field handing.
thomasvl Jul 17, 2018
877cb28
Update the generated_swift_names_* files.
thomasvl Jul 17, 2018
b0b4a38
Support the conformance test's new ignore_unknown_json.
thomasvl Jul 17, 2018
9c42e1f
Fix up typos from review feedback.
thomasvl Jul 17, 2018
5781bb9
Bump the version number to 1.1.0
thomasvl Jul 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion Sources/SwiftProtobuf/AnyMessageStorage.swift
View file
Open in desktop
Expand Up @@ -62,7 +62,9 @@ fileprivate func unpack(contentJSON: Data,
var value = String()
try contentJSON.withUnsafeBytes { (bytes:UnsafePointer<UInt8>) in
let buffer = UnsafeBufferPointer(start: bytes, count: contentJSON.count)
var scanner = JSONScanner(source: buffer, messageDepthLimit: options.messageDepthLimit)
var scanner = JSONScanner(source: buffer,
messageDepthLimit: options.messageDepthLimit,
ignoreUnknownFields: options.ignoreUnknownFields)
let key = try scanner.nextQuotedString()
if key != "value" {
// The only thing within a WKT should be "value".
Expand Down
4 changes: 3 additions & 1 deletion Sources/SwiftProtobuf/JSONDecoder.swift
View file
Open in desktop
Expand Up @@ -27,7 +27,9 @@ internal struct JSONDecoder: Decoder {

internal init(source: UnsafeBufferPointer<UInt8>, options: JSONDecodingOptions) {
self.options = options
self.scanner = JSONScanner(source: source, messageDepthLimit: self.options.messageDepthLimit)
self.scanner = JSONScanner(source: source,
messageDepthLimit: self.options.messageDepthLimit,
ignoreUnknownFields: self.options.ignoreUnknownFields)
}

private init(decoder: JSONDecoder) {
Expand Down
4 changes: 4 additions & 0 deletions Sources/SwiftProtobuf/JSONDecodingError.swift
View file
Open in desktop
Expand Up @@ -55,4 +55,8 @@ public enum JSONDecodingError: Error {
case conflictingOneOf
/// Reached the nesting limit for messages within messages while decoding.
case messageDepthLimit
/// Encountered a unknown field with the given name. When parsing JSON, you
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/Encountered a/Encountered an/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

/// can instead instructe the library to ignore this via
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/instructe/instruct/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

/// JSONDecodingOptions.ignoreUnknownFields.
case unknownField(String)
}
4 changes: 4 additions & 0 deletions Sources/SwiftProtobuf/JSONDecodingOptions.swift
View file
Open in desktop
Expand Up @@ -21,5 +21,9 @@ public struct JSONDecodingOptions {
/// while parsing.
public var messageDepthLimit: Int = 100

/// If unknown fields in the JSON should be ignored. If they aren't
/// ignored, and error will be raised if one is encountered.
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/and error/an error/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

public var ignoreUnknownFields: Bool = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Just out of curiosity, why is the default value for this option false?

I ask because it seems more natural to set this to true, to me, as someone who uses protobuf to help manage version compatibility across various components.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I also had the pointer in the second commit on the PR, but it is in the spec –

Spec: https://developers.google.com/protocol-buffers/docs/proto3#json_options

Ignore unknown fields: Proto3 JSON parser should reject unknown fields by default but may provide an option to ignore unknown fields in parsing.

With the binary format, the unknown fields can be parsed and preserved to be reflected back out. But for the JSON format, there is no way to know if something was an int32, int64, uint32, etc., so while it could be reflected back out in JSON, you can't convert it or even expose it via a reflection (if you language has proto reflection).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, since the "ignore" is destructive, it really shouldn't be the default since it means by default we lose things we can't deal with. If we tried to capture the text and reflect it out for JSON, maybe it would make sense, but the fact that a binary conversion would loose things is also bad.

There is a similar issue with enums, if you get a value string you don't know, there is no way to keep it. They also offer up that folks could provide an encoding option to use number, but we haven't added that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, that makes a lot of sense. I am not accustomed to using JSON with proto aside from debugging.


public init() {}
}
15 changes: 14 additions & 1 deletion Sources/SwiftProtobuf/JSONScanner.swift
View file
Open in desktop
Expand Up @@ -376,6 +376,7 @@ internal struct JSONScanner {
private var numberFormatter = DoubleFormatter()
internal var recursionLimit: Int
internal var recursionBudget: Int
private var ignoreUnknownFields: Bool

/// True if the scanner has read all of the data from the source, with the
/// exception of any trailing whitespace (which is consumed by reading this
Expand All @@ -397,11 +398,16 @@ internal struct JSONScanner {
return source[index]
}

internal init(source: UnsafeBufferPointer<UInt8>, messageDepthLimit: Int) {
internal init(
source: UnsafeBufferPointer<UInt8>,
messageDepthLimit: Int,
ignoreUnknownFields: Bool
) {
self.source = source
self.index = source.startIndex
self.recursionLimit = messageDepthLimit
self.recursionBudget = messageDepthLimit
self.ignoreUnknownFields = ignoreUnknownFields
}

private mutating func incrementRecursionDepth() throws {
Expand Down Expand Up @@ -1248,13 +1254,20 @@ internal struct JSONScanner {
if let fieldNumber = names.number(forJSONName: key) {
return fieldNumber
}
if !ignoreUnknownFields {
let fieldName = utf8ToString(bytes: key.baseAddress!, count: key.count)!
throw JSONDecodingError.unknownField(fieldName)
}
} else {
// Slow path: We parsed a String; lookups from String are slower.
let key = try nextQuotedString()
try skipRequiredCharacter(asciiColon) // :
if let fieldNumber = names.number(forJSONName: key) {
return fieldNumber
}
if !ignoreUnknownFields {
throw JSONDecodingError.unknownField(key)
}
}
// Unknown field, skip it and try to parse the next field name
try skipValue()
Expand Down
9 changes: 7 additions & 2 deletions Tests/SwiftProtobufTests/TestHelpers.swift
View file
Open in desktop
Expand Up @@ -266,9 +266,14 @@ extension PBTestHelpers where MessageTestType: SwiftProtobuf.Message & Equatable
}
}

func assertJSONDecodeFails(_ json: String, file: XCTestFileArgType = #file, line: UInt = #line) {
func assertJSONDecodeFails(
_ json: String,
options: JSONDecodingOptions = JSONDecodingOptions(),
file: XCTestFileArgType = #file,
line: UInt = #line
) {
do {
let _ = try MessageTestType(jsonString: json)
let _ = try MessageTestType(jsonString: json, options: options)
XCTFail("Swift decode should have failed: \(json)", file: file, line: line)
} catch {
// Yay! It failed!
Expand Down
1 change: 1 addition & 0 deletions Tests/SwiftProtobufTests/Test_JSONDecodingOptions.swift
View file
Open in desktop
Expand Up @@ -41,6 +41,7 @@ class Test_JSONDecodingOptions: XCTestCase {
do {
var options = JSONDecodingOptions()
options.messageDepthLimit = limit
options.ignoreUnknownFields = true
let _ = try ProtobufUnittest_TestRecursiveMessage(jsonString: jsonInput, options: options)
if !expectSuccess {
XCTFail("Should not have succeed, pass: \(i), limit: \(limit)")
Expand Down
70 changes: 37 additions & 33 deletions Tests/SwiftProtobufTests/Test_Unknown_proto2.swift
View file
Open in desktop
Expand Up @@ -28,7 +28,9 @@ class Test_Unknown_proto2: XCTestCase, PBTestHelpers {
/// Verify that json decode ignores the provided fields but otherwise succeeds
func assertJSONIgnores(_ json: String, file: XCTestFileArgType = #file, line: UInt = #line) {
do {
let empty = try ProtobufUnittest_TestEmptyMessage(jsonString: json)
var options = JSONDecodingOptions()
options.ignoreUnknownFields = true
let empty = try ProtobufUnittest_TestEmptyMessage(jsonString: json, options: options)
do {
let json = try empty.jsonString()
XCTAssertEqual("{}", json, file: file, line: line)
Expand Down Expand Up @@ -122,38 +124,40 @@ class Test_Unknown_proto2: XCTestCase, PBTestHelpers {
assertJSONIgnores("{\"unknown\": 7, \"unknown\": 8}") // ???

// Badly formed JSON should fail to decode, even in unknown sections
assertJSONDecodeFails("{\"unknown\": 1e999}")
assertJSONDecodeFails("{\"unknown\": \"hi!\"")
assertJSONDecodeFails("{\"unknown\": \"hi!}")
assertJSONDecodeFails("{\"unknown\": qqq }")
assertJSONDecodeFails("{\"unknown\": { }")
assertJSONDecodeFails("{\"unknown\": [ }")
assertJSONDecodeFails("{\"unknown\": { ]}")
assertJSONDecodeFails("{\"unknown\": ]}")
assertJSONDecodeFails("{\"unknown\": null true}")
assertJSONDecodeFails("{\"unknown\": nulll }")
assertJSONDecodeFails("{\"unknown\": nul }")
assertJSONDecodeFails("{\"unknown\": Null }")
assertJSONDecodeFails("{\"unknown\": NULL }")
assertJSONDecodeFails("{\"unknown\": True }")
assertJSONDecodeFails("{\"unknown\": False }")
assertJSONDecodeFails("{\"unknown\": nan }")
assertJSONDecodeFails("{\"unknown\": NaN }")
assertJSONDecodeFails("{\"unknown\": Infinity }")
assertJSONDecodeFails("{\"unknown\": infinity }")
assertJSONDecodeFails("{\"unknown\": Inf }")
assertJSONDecodeFails("{\"unknown\": inf }")
assertJSONDecodeFails("{\"unknown\": 1}}")
assertJSONDecodeFails("{\"unknown\": {1, 2}}")
assertJSONDecodeFails("{\"unknown\": 1.2.3.4.5}")
assertJSONDecodeFails("{\"unknown\": -.04}")
assertJSONDecodeFails("{\"unknown\": -19.}")
assertJSONDecodeFails("{\"unknown\": -9.3e+}")
assertJSONDecodeFails("{\"unknown\": 1 2 3}")
assertJSONDecodeFails("{\"unknown\": { true false }}")
assertJSONDecodeFails("{\"unknown\"}")
assertJSONDecodeFails("{\"unknown\": }")
assertJSONDecodeFails("{\"unknown\", \"a\": 1}")
var options = JSONDecodingOptions()
options.ignoreUnknownFields = true
assertJSONDecodeFails("{\"unknown\": 1e999}", options: options)
assertJSONDecodeFails("{\"unknown\": \"hi!\"", options: options)
assertJSONDecodeFails("{\"unknown\": \"hi!}", options: options)
assertJSONDecodeFails("{\"unknown\": qqq }", options: options)
assertJSONDecodeFails("{\"unknown\": { }", options: options)
assertJSONDecodeFails("{\"unknown\": [ }", options: options)
assertJSONDecodeFails("{\"unknown\": { ]}", options: options)
assertJSONDecodeFails("{\"unknown\": ]}", options: options)
assertJSONDecodeFails("{\"unknown\": null true}", options: options)
assertJSONDecodeFails("{\"unknown\": nulll }", options: options)
assertJSONDecodeFails("{\"unknown\": nul }", options: options)
assertJSONDecodeFails("{\"unknown\": Null }", options: options)
assertJSONDecodeFails("{\"unknown\": NULL }", options: options)
assertJSONDecodeFails("{\"unknown\": True }", options: options)
assertJSONDecodeFails("{\"unknown\": False }", options: options)
assertJSONDecodeFails("{\"unknown\": nan }", options: options)
assertJSONDecodeFails("{\"unknown\": NaN }", options: options)
assertJSONDecodeFails("{\"unknown\": Infinity }", options: options)
assertJSONDecodeFails("{\"unknown\": infinity }", options: options)
assertJSONDecodeFails("{\"unknown\": Inf }", options: options)
assertJSONDecodeFails("{\"unknown\": inf }", options: options)
assertJSONDecodeFails("{\"unknown\": 1}}", options: options)
assertJSONDecodeFails("{\"unknown\": {1, 2}}", options: options)
assertJSONDecodeFails("{\"unknown\": 1.2.3.4.5}", options: options)
assertJSONDecodeFails("{\"unknown\": -.04}", options: options)
assertJSONDecodeFails("{\"unknown\": -19.}", options: options)
assertJSONDecodeFails("{\"unknown\": -9.3e+}", options: options)
assertJSONDecodeFails("{\"unknown\": 1 2 3}", options: options)
assertJSONDecodeFails("{\"unknown\": { true false }}", options: options)
assertJSONDecodeFails("{\"unknown\"}", options: options)
assertJSONDecodeFails("{\"unknown\": }", options: options)
assertJSONDecodeFails("{\"unknown\", \"a\": 1}", options: options)
}


Expand Down
70 changes: 37 additions & 33 deletions Tests/SwiftProtobufTests/Test_Unknown_proto3.swift
View file
Open in desktop
Expand Up @@ -28,7 +28,9 @@ class Test_Unknown_proto3: XCTestCase, PBTestHelpers {
/// Verify that json decode ignores the provided fields but otherwise succeeds
func assertJSONIgnores(_ json: String, file: XCTestFileArgType = #file, line: UInt = #line) {
do {
let empty = try Proto3ArenaUnittest_TestEmptyMessage(jsonString: json)
var options = JSONDecodingOptions()
options.ignoreUnknownFields = true
let empty = try Proto3ArenaUnittest_TestEmptyMessage(jsonString: json, options: options)
do {
let json = try empty.jsonString()
XCTAssertEqual("{}", json, file: file, line: line)
Expand Down Expand Up @@ -122,38 +124,40 @@ class Test_Unknown_proto3: XCTestCase, PBTestHelpers {
assertJSONIgnores("{\"unknown\": 7, \"unknown\": 8}") // ???

// Badly formed JSON should fail to decode, even in unknown sections
assertJSONDecodeFails("{\"unknown\": 1e999}")
assertJSONDecodeFails("{\"unknown\": \"hi!\"")
assertJSONDecodeFails("{\"unknown\": \"hi!}")
assertJSONDecodeFails("{\"unknown\": qqq }")
assertJSONDecodeFails("{\"unknown\": { }")
assertJSONDecodeFails("{\"unknown\": [ }")
assertJSONDecodeFails("{\"unknown\": { ]}")
assertJSONDecodeFails("{\"unknown\": ]}")
assertJSONDecodeFails("{\"unknown\": null true}")
assertJSONDecodeFails("{\"unknown\": nulll }")
assertJSONDecodeFails("{\"unknown\": nul }")
assertJSONDecodeFails("{\"unknown\": Null }")
assertJSONDecodeFails("{\"unknown\": NULL }")
assertJSONDecodeFails("{\"unknown\": True }")
assertJSONDecodeFails("{\"unknown\": False }")
assertJSONDecodeFails("{\"unknown\": nan }")
assertJSONDecodeFails("{\"unknown\": NaN }")
assertJSONDecodeFails("{\"unknown\": Infinity }")
assertJSONDecodeFails("{\"unknown\": infinity }")
assertJSONDecodeFails("{\"unknown\": Inf }")
assertJSONDecodeFails("{\"unknown\": inf }")
assertJSONDecodeFails("{\"unknown\": 1}}")
assertJSONDecodeFails("{\"unknown\": {1, 2}}")
assertJSONDecodeFails("{\"unknown\": 1.2.3.4.5}")
assertJSONDecodeFails("{\"unknown\": -.04}")
assertJSONDecodeFails("{\"unknown\": -19.}")
assertJSONDecodeFails("{\"unknown\": -9.3e+}")
assertJSONDecodeFails("{\"unknown\": 1 2 3}")
assertJSONDecodeFails("{\"unknown\": { true false }}")
assertJSONDecodeFails("{\"unknown\"}")
assertJSONDecodeFails("{\"unknown\": }")
assertJSONDecodeFails("{\"unknown\", \"a\": 1}")
var options = JSONDecodingOptions()
options.ignoreUnknownFields = true
assertJSONDecodeFails("{\"unknown\": 1e999}", options: options)
assertJSONDecodeFails("{\"unknown\": \"hi!\"", options: options)
assertJSONDecodeFails("{\"unknown\": \"hi!}", options: options)
assertJSONDecodeFails("{\"unknown\": qqq }", options: options)
assertJSONDecodeFails("{\"unknown\": { }", options: options)
assertJSONDecodeFails("{\"unknown\": [ }", options: options)
assertJSONDecodeFails("{\"unknown\": { ]}", options: options)
assertJSONDecodeFails("{\"unknown\": ]}", options: options)
assertJSONDecodeFails("{\"unknown\": null true}", options: options)
assertJSONDecodeFails("{\"unknown\": nulll }", options: options)
assertJSONDecodeFails("{\"unknown\": nul }", options: options)
assertJSONDecodeFails("{\"unknown\": Null }", options: options)
assertJSONDecodeFails("{\"unknown\": NULL }", options: options)
assertJSONDecodeFails("{\"unknown\": True }", options: options)
assertJSONDecodeFails("{\"unknown\": False }", options: options)
assertJSONDecodeFails("{\"unknown\": nan }", options: options)
assertJSONDecodeFails("{\"unknown\": NaN }", options: options)
assertJSONDecodeFails("{\"unknown\": Infinity }", options: options)
assertJSONDecodeFails("{\"unknown\": infinity }", options: options)
assertJSONDecodeFails("{\"unknown\": Inf }", options: options)
assertJSONDecodeFails("{\"unknown\": inf }", options: options)
assertJSONDecodeFails("{\"unknown\": 1}}", options: options)
assertJSONDecodeFails("{\"unknown\": {1, 2}}", options: options)
assertJSONDecodeFails("{\"unknown\": 1.2.3.4.5}", options: options)
assertJSONDecodeFails("{\"unknown\": -.04}", options: options)
assertJSONDecodeFails("{\"unknown\": -19.}", options: options)
assertJSONDecodeFails("{\"unknown\": -9.3e+}", options: options)
assertJSONDecodeFails("{\"unknown\": 1 2 3}", options: options)
assertJSONDecodeFails("{\"unknown\": { true false }}", options: options)
assertJSONDecodeFails("{\"unknown\"}", options: options)
assertJSONDecodeFails("{\"unknown\": }", options: options)
assertJSONDecodeFails("{\"unknown\", \"a\": 1}", options: options)
}


Expand Down