crichez / swift-bison Goto Github PK

Is your improvement request related to a problem? Please describe.

UUID is a BSON type, but is not supported by BSONKit. BSONCodable works with UUID only by encoding and decoding from uuidString.

Describe the solution you'd like

• Expose a new protocol that generates BSON binary value metadata before returning bsonBytes.
• Conform UUID to that protocol.
• Have BSONEncoder check for ValueProtocol conformance and handle those types separately.
• Create decoding counterparts to the steps above.

Describe alternatives you've considered

We could also conform to ValueProtocol directly, but metadata generation will have to be done manually, and that code will be repeated for each new binary type exposed by the package.

Is your improvement request related to a problem? Please describe.

Now that UUID and Data conform to ParsableValue and ValueProtocol, we should be able to use them with BSONEncoder and BSONDecoder. Right now UUID is encoded as a String, and Data
is 8 times larger than it should be in a document.

Describe the solution you'd like

Have each container check for BSON values before encoding or decoding a generic Encodable value. If a BSON value is found, use its appropriate protocol implementation to encode or decode it.

Describe alternatives you've considered

This design was considered secondary since the main goal of BSONEncodable and BSONDecodable was to adapt existing code, which works fine as is. For projects with multi-language stacks looking to adopt BSON, we need to make sure the documents we encode are as expected for other BSON libraries, and this includes these modules.

Additional context

This will likely be done through an existential same-type check in each container's generic encode(_:) and decode(_:) methods as follows:

func encode<T: Encodable>(_ value: T, forKey key: Key) throws {
    if let bsonValue = value as? ValueProtocol {
        contents.append(key, ValueBox(bsonValue))
    } else {
        let encoder = BSONEncodingContainerProvider(codingPath: codingPath)
        try value.encode(to: encoder)
        contents.append(key, encoder)
    }
}

For this to work for decoding we need to close #25, since we can't use a protocol with an associated type as an existential. Ultimately it should look like this with some error conversion:

func decode<T: Decodable>(_ type: T.Type, forKey key: Key) throws -> T {
    let encodedValue = try valueData(forKey: key)
    if let bsonType = type as? ParsableValue.Type {
        return bsonType.init(bsonBytes: encodedValue) as! T
    } else {
        let decoder = DecodingContainerProvider(encodedValue: encodedValue, codingPath: codingPath)
        return try type.init(from: decoder) 
    }
}

Is your improvement request related to a problem? Please describe.

BSON ObjectID is a specification-native type, so should be included in BSONKit.

Describe the solution you'd like

Implement the ObjectID struct and conform to ValueProtocol and ParsableValue.

Describe alternatives you've considered

The ObjectID type has low utility outside of MongoDB, so its inclusion is debatable. The main motivation is that we cannot claim full specification compliance without it.

Additional context

The current implementation will have to use Foundation.Date and its extended API to generate and manipulate the timestamp bytes of ObjectID. This will be very familiar to current Swift developers, but we will want to transition to the upcoming Clock related APIs in the standard library when they become available (Swift 5.7?).

Is your improvement request related to a problem? Please describe.

BSONEncoder's objective is to enable developers to adopt BSON encoding using existing Encodable implementations. Because its return data type is [UInt8], we lose a lot of interoperability and the change to BSON leads to more friction.

Describe the solution you'd like

Change the return type of the encoder's encode(_:) method to Data.

We should measure the performance impact of copying the current byte array into a Data buffer and compare it to an additional protocol that makes the conversion at the value-level. The latter is a bigger refactor, but should be considered nonetheless.

Describe alternatives you've considered

Adding a second method: we could have a second method called encodeData(_:) that returns
Data instead by just copying the result of the original encode(_:) method. This messes with conformance to Combine.TopLevelEncodable and generally makes the API more difficult to adopt, and may not offer meaningful performance improvements compared to the added complexity.

Making ValueProtocol.bsonBytes into a generic method: we previously had a version of the package that declared the Encoded associated type on ValueProtocol and DocComponent. The current bsonBytes computed property was a bsonBytes() method that returned the Encoded type, a Collection of UInt8 bytes. This was ultimately discarded after performance tests that revealed the chained buffers used as Encoded types for most types were slow and didn't offer significant memory usage improvements. This could be re-implemented without the chained buffers, and this would allow us to generalize those protocols into a BSONEncodable protocol that exclusively uses Data as its Encoded type. This would allows us to use the generalized protocol as an existential type, making it compatible with the Encoder set of protocols & containers. This may be used eventually, but the scale of this refactor would need to be warranted by the performance results of the current proposed solution.

Additional context

• Discussion on Data vs [UInt8]

Expected Behavior

Using Xcode 13.3 and above, the following command should build the project successfully:

xcodebuild build -scheme swift-bson-Package -destination "platform=macOS,id=$"

where $ is the destination ID for the current macOS machine.

Actual Behavior

Building fails due to a series of "undefined symbol" errors. For examples, see the last failed test run of #1.

Environment

• Platform: macOS
• OS version: 11.6, 12.3
• Xcode version: 13.3
• Swift version: 5.4.2, 5.6

Additional Information

Builds using the Swift open-source toolchains work as expected. Builds using the swift command on Xcode versions of the Swift toolchains also work as expected. Any builds using xcodebuild on those same versions fail.

Is your improvement request related to a problem? Please describe.

The last three pull requests on main implement the BSONEncodable and BSONDecodable modules but they are not introduced in README.md.

Describe the solution you'd like

Write a brief overview and usage guide on BSONEncodable and BSONDecodable.

Describe alternatives you've considered

N/A

Additional context

N/A

Is your improvement request related to a problem? Please describe.

Working with array documents currently requires the caller to define keys. In most docs we use ForEach to loop over an enumerated sequence, but this restricts array documents to a single value type.

Describe the solution you'd like

Array documents that use an API similar to Array, where indices are deduced from the position of each element. Except using a result builder allows conditional, optional and looped composition.

let arrayDoc = ComposedArrayDocument {
    1.295
    "test"
    if Bool.random() {
        -109_000
    }
}

For cases where the caller still wants to compose a homogeneously-typed document, they should be able to use an array literal.

let arrayDoc: ComposedArrayDocument = [
    1.295,
    1.098,
]

Describe alternatives you've considered

The original document composition API didn't even feature an array document type, since it was considered a subset of the traditional keyed document type. The ComposedArrayDocument type was added when the need arose in the development of BSONEncoder (the type byte is now the only difference). This is still an option, where the declaration uses an abstracted API but the resulting document type is specific to that declaration.

/// No keys are defined, so indices are automatically deduced and an array document is composed.
let arrayDoc = ComposedDocument {
    for _ in 1...19 {
        "value"
    }
    "end"
}

/// At least one key is defined, so the document cannot be composed.
let doc = ComposedDocument {
    for _ in 1...19 {
        "key" => "value"
    }
    "end" // Compiler error
}

The question is whether this is more obvious to the caller than a separate ArrayDocBuilder result builder. In my opinion using a separate builder is more clear, since it avoid ambiguity associated with a mix of keyed and unkeyed values.

Additional context

I anticipate the largest problem in implementation is index deduction. In the way DocBuilder is currently implemented, components have no awareness of their position relative to other components in the document. The only time we can reliably generate indices with the API as-is would be when documents are declared with a single Tuple arity.

// A document made of a single `Tuple3`, where indices can be deduced as-is.
let doc = ComposedDocument { 0; 1; 2 }

// A document made of two `Group` values, where indices are only known within a group.
let otherDoc = ComposedDocument {
    Group { 1; 2 }
    Group { 1; 2 }
}

In the latter case, we may need to store state about the start and end index of each component. This is difficult with the current implementation, since the order in which the declaration is parsed is undefined. We can determine this by storing an increment in the state of the document initializer, but this significantly increases the complexity of the result builder

In Swift 5.7, the new buildPartialBlock method may make this easier, as there is now a defined order in which each component is parsed. This should be released soon, but I don't want such a large bump in the minimum swift version so close to the first release.

Is your improvement request related to a problem? Please describe.

All but two ParsableValue implementations share the same errors, yet they are defined in separate .Error types. This makes error handling tedious and doesn't provide any obvious benefits.

Describe the solution you'd like

Remove the requirement for an Error associated type on ParsableValue and move coming errors to their own type.

Describe alternatives you've considered

The original intent for this requirement was to make it clear to the caller what errors they need to handle, and which they can ignore when initializing a value. Since we only end up with two different errors outside of ParsedDocument, using a single type for simple values is clearer.

Additional context

This might enable us to remove some of the code duplication in BSONDecodable containers. Now that we don't need to catch type-specific errors, we may be able to generalize the code over the type property passed to each method.

Is your improvement request related to a problem? Please describe.

We currently have the BinaryValueProtocol for UUID and Data, but we don't have a decoding counterpart.

Describe the solution you'd like

Implement the BinaryParsableValue protocol with similar logic to BinaryValueProtocol. The metadata for each value should be parsed by a default implementation, so conforming can focus on the actual data bytes of the value's encoded representation.

Describe alternatives you've considered

See #23 for alternatives to this system.

Additional context

N/A

Is your improvement request related to a problem? Please describe.

Working with Foundation.Date currently requires us to store it as a Double or other numerical types, which means the utc datetime BSON type is never used. Date is a perfect candidate for it.

Describe the solution you'd like

Conform Date to ParsableValue and ValueProtocol and declare the utc datetime type.

Describe alternatives you've considered

The BSON UTC DateTime type is still an Int64, and therefore won't be nearly as precise as Date. We could introduce a wrapper type around Int64 that declares a different type, and this would align the implementation and its binary representation, as well as make the API line up with other platforms. Ultimately no one in Swift works with Int64 as a date though, so the extension would likely almost never be used. Using Date is far more natural in Swift.

Additional context

We already import Foundation and use Date and Data in other parts of the project. We always want to support Windows, and parts of Foundation are exposed but still off-limits. So far these types have caused no problems.

Issue Description

WritableDoc and its associated protocols currently output encoded documents as [UInt8].
In applications that rely on Foundation for data transfer, this requires that the resulting byte
array be converted to Data before they can be useful.

Proposed Solution

Change the output type of the WritableValue.bsonBytes property to Foundation.Data.

import Foundation 

public protocol WritableValue {
    var bsonBytes: Data { get }
    var bsonType: UInt8 { get }
}

Alternatives Considered

This was tested a while back, and byte arrays seemed to provide a very small performance advantage.
They were chosen mostly for this reason, and to potentially avoid some Objective-C bridging and
compability issues related to Foundation on Linux and Windows. However, we already use Data in
BisonEncode and BisonDecode, and those modules pass extensive tests on all Swift platforms. Without
the advantage of compability, the modest performance gain in server applications is the only reason
to continue using byte arrays. For applications that work on Apple platforms, this actually
decreases performance from having to copy documents to Data before interacting with the file system
or the network. I believe switching to Data is a worthwhile trade-off.

Is your improvement request related to a problem? Please describe.

When attempting to read a valid BSON document with a key that declares a deprecated type byte, the current implementation throws ReadableDoc.Error.unknownType. Not supporting deprecated types makes adapting old codebases impossible.

Describe the solution you'd like

Support sizing deprecated types. Providing type analogs is not necessary, clients have the tools to build their own using ReadableValue.

Describe alternatives you've considered

The original decision not to support deprecated types was meant to accelerate development. As we approach the first release, these should be at least partially supported.

Additional context

N/A.