apple / apple/FHIRModels

thoughts on `FHIRDecodingDepthTracker`

Open
#45 0 comments 1 reaction 1 assignee Claimed by @p2-apple View on GitHub
Dominant language
Swift
Stars
209
Forks
36
PR merge metrics
No merged PRs in 30d

Description

i'd like to provide some unsolicited thoughts on the new `FHIRDecodingDepthTracker` API:

i'm not sure if this is a good idea as currently implemented, as `JSONDecoder` instances are explicitly allowed to be used concurrently, i.e. used to run multiple decoding operations at the same time. a `JSONDecoder` that internally (via its `userInfo`) contains a `FHIRDecodingDepthTracker` breaks this contract, since the tracker will be shared across all concurrently-running decoding operations.
the docs currently already mention that users should "Use one instance per decode call, do not share across concurrent decodes.",
but tbh given the fact that it is not clear, when a `JSONDecoder` instance is being passed around the program, whether it contains any depth trackers, this doesn't really solve the issue, as it is super easy for a program to run into concurrency issues here.

a potentially better solution, IMO, might be adding depth tracking logic that operates based on `decoder.codingPath.count`: i did some testing here and it does appear that this would function as a workaround. the `Swift.Decoder` created and used by `Foundation.JSONDecoder` implements `codingPath` in a way that corectly grows the path whenever the decoder descends into a nested container.
my suggestion here would be providing an API that allows the caller to configure a `JSONDecoder`'s `maxFHIRModelsDepth` (as an `Int?` stored in the userInfo).

the important criterion for the alternative suggested above to work correctly, is that there must not exist any potential zero-growth cycles within the package's data structures, i.e. no types that contain a non-nested instance of themselves.

this requirement is currently satisfied, and as long as the general structure of the auto-generated types in the package remains the same (which will likely be the case, as the structure is ultimately defined by FHIR, which doesn't contain any self-referential types that would not result in the decoder's `codingPath.count` increasing).

---

proposed new API:

i implemented this in a branch [here](https://github.com/lukaskollmer/FHIRModels/tree/lukas/depth-tracking-rework); check out the [FHIRDecodingDepthTracker.swift](https://github.com/lukaskollmer/FHIRModels/blob/lukas/depth-tracking-rework/Sources/FMCore/FHIRDecodingDepthTracker.swift) file in particular.
the rest of the diff is lowering the deployment target back to iOS 15 (see also #44), and updating all `init(from:)`s to use the new API.

```swift
extension JSONDecoder {
/// A maximum depth requirement enforced by FHIRModels, if set to a value greater than zero.
///
/// The purpose of this check is to prevent stack exhaustion from recursive structures.
/// You can either enable this check by setting the ``maxFHIRModelsDepth`` value on a `JSONDecoder` created by your program,
/// or you can use ``Foundation/JSONDecoder/fhirModelsReadyDecoder(maxDepth:)`` to obtain a `JSONDecoder` with this check enabled.
///
/// A `nil` value, or any value `<= 0` disables the check entirely.
///
/// - Note: "Depth" here does not necessarily relate to the logical decoding descent depth,
/// but rather to an opaque depth value that monotonically increases with the decoder descending into the data being decoded.
public var maxFHIRModelsDepth: Int? {
get { userInfo[.maxFHIRModelsDepthKey] as? Int }
set { userInfo[.maxFHIRModelsDepthKey] = newValue }
}

/// A `JSONDecoder` preconfigured with FHIRModels' recommended decoding-depth limit.
public static func fhirModelsReadyDecoder(maxDepth: Int = 48) -> JSONDecoder {
let decoder = JSONDecoder()
decoder.maxFHIRModelsDepth = maxDepth
return decoder
}
}

extension Decoder {
/// Throws a `DecodingError.dataCorrupted` if the current decoding `codingPath` is deeper than the
/// limit configured via ``Foundation/JSONDecoder/maxFHIRModelsDepth``. A no-op when no limit is set
/// (e.g. a plain `JSONDecoder()` with no configuration), or when the decoder does not carry the
/// FHIRModels depth limit in its `userInfo`.
///
/// This should be called at the top of every generated model's `init(from:)`.
package func enforceFHIRModelsDepthLimit() throws(DecodingError) {
...
}
}
```

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.