Add validating setters to OpenAPI dynamic value containers#198
Open
ayush-that wants to merge 1 commit into
Open
Add validating setters to OpenAPI dynamic value containers#198ayush-that wants to merge 1 commit into
ayush-that wants to merge 1 commit into
Conversation
Resolves apple/swift-openapi-generator#782 `OpenAPIValueContainer`, `OpenAPIObjectContainer`, and `OpenAPIArrayContainer` exposed `var value` as a stored property whose setter accepted any input, including types the public initializers would reject. That made it possible to construct a "validated" container whose contents were never validated: var c = OpenAPIObjectContainer() c.value["bad"] = BadGuy() Per the design discussion on #782 (czechboy0, weissi, simonjbeaumont), this change: - Converts each container's `value` to a computed property backed by a private stored property, keeping read access public and unchanged. - Marks only the setter as `@available(*, deprecated, message:)` so existing direct assignment still compiles with a deprecation warning but is steered toward the new API. - Adds `mutating func setValue(validating:)` that runs the same validation as `init(unvalidatedValue:)` and leaves the existing value intact on failure. Source compatibility is preserved for all public clients: reading `value` is unchanged, writing `value` still compiles but emits a deprecation warning. ### Motivation The original setter let validated containers drift out of a valid state. SwiftPM-generated code passes user data through these containers, so silent acceptance of unsupported types broke the validation contract. ### Modifications - `Sources/OpenAPIRuntime/Base/OpenAPIValue.swift`: same change applied to all three containers (`OpenAPIValueContainer`, `OpenAPIObjectContainer`, `OpenAPIArrayContainer`). - `Tests/OpenAPIRuntimeTests/Base/Test_OpenAPIValue.swift`: six new tests covering the accept/reject paths for each container and verifying the previous value is unchanged on a failed validation. ### Result Users get a validated path to update container contents and a deprecation warning when they use the old direct-assignment path. All 27 existing and new tests pass locally.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR introduces validated mutation APIs for OpenAPI container types, discouraging direct assignment that bypasses runtime type validation.
Changes:
- Added
setValue(validating:)toOpenAPIValueContainer,OpenAPIObjectContainer, andOpenAPIArrayContainer. - Introduced private backing storage (
_value) and deprecated directvaluesetters to steer callers toward validated mutation. - Added unit tests to cover accepted/rejected values and “no partial mutation on failure” behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| Tests/OpenAPIRuntimeTests/Base/Test_OpenAPIValue.swift | Adds tests for the new validated setter APIs (success paths, failure paths, and rollback behavior). |
| Sources/OpenAPIRuntime/Base/OpenAPIValue.swift | Adds backing storage, deprecates unsafe setters, and implements validated mutation via setValue(validating:). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+52
to
+54
| try container.setValue(validating: ["nested": 1] as [String: any Sendable]) | ||
| let dict = try XCTUnwrap(container.value as? [String: Int]) | ||
| XCTAssertEqual(dict, ["nested": 1]) |
Comment on lines
+61
to
+65
| /// Replaces the contained value with the given value after validating that | ||
| /// it consists of supported types. | ||
| /// - Parameter newValue: A value of a JSON-compatible type, such as | ||
| /// `String`, `[Any]`, and `[String: Any]`. | ||
| /// - Throws: When the value is not supported. |
Comment on lines
+66
to
+68
| public mutating func setValue(validating newValue: (any Sendable)?) throws { | ||
| self._value = try Self.tryCast(newValue) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
resolves apple/swift-openapi-generator#782
OpenAPIValueContainer,OpenAPIObjectContainer, andOpenAPIArrayContainerexposedvar valueas a stored property whose setter accepted any input, including types the public initializers would reject. that made it possible to construct a "validated" container whose contents were never validated:per the design discussion on #782 (czechboy0, weissi, simonjbeaumont):
valuebecomes a computed property backed by a private stored property. read access stays public and unchanged.@available(*, deprecated, message:), so existing direct assignment still compiles with a deprecation warning.mutating func setValue(validating:)runs the same validation asinit(unvalidatedValue:)and leaves the existing value intact on failure.source compatibility is preserved for all public clients: reading
valueis unchanged, writingvaluestill compiles but emits a deprecation warning pointing at the new method.Motivation
the original setter let validated containers drift out of a valid state. swift-openapi-generator passes user data through these containers, so silent acceptance of unsupported types broke the validation contract.
Modifications
Sources/OpenAPIRuntime/Base/OpenAPIValue.swift: same change applied to all three containers.Tests/OpenAPIRuntimeTests/Base/Test_OpenAPIValue.swift: six new tests covering the accept/reject paths and verifying that a failedsetValue(validating:)leaves the previous value intact.Result
users get a validated path to update container contents and a deprecation warning when they use the old direct-assignment path.
Test Plan
swift test --filter Test_OpenAPIValue