Skip to content

Commit

Permalink
[Generator] Multiple content types support (#146)
Browse files Browse the repository at this point in the history
[Generator] Multiple content types support

Depends on apple/swift-openapi-runtime#29

### Motivation

Up until now, when an OpenAPI document contained more than one content type in either a request or a response body, we would only generate code for now, and ignored the other ones.

However, that was a temporary workaround until we could add proper support for multiple content types, which is what this PR is about.

Addresses #6 and #7, except for the "Accept header" propagation, which will be addressed separately and have its own PR and even a proposal. That's one of the reasons this feature is landing disabled, hidden behind the `multipleContentTypes` feature flag.

A tip for reviewing this change: first check out how the test OpenAPI document and the reference files changed, that shows the details of how we encode and decode multiple content types. I also added comments in the code for easier review.

### Modifications

- Main: all supported content types found in a `content` map in request and response bodies are now generated in the `Body` enum.
- Added a method `supportedTypedContents` that returns all supported content types, used now instead of the `bestTypedContent` method we used before. What is a "supported" content type? One that has a schema that doesn't return `false` to `isSchemaSupported`. (This is a pre-existing concept.)
- Updated the logic for generating request and response bodies to use the new method, both on client and server.
- Updated content type -> Swift enum case name logic, but that will go through a full proposal, so please hold feedback for the proposal, this is just a first pass based on some collected data of most commonly used content types.
- 

### Open Questions
- What to do in an operation with multiple request/response content types when _no_ `content-type` header is provided? Previously, with up to 1 content type support, we still went ahead and tried to parse it. But here, we don't know which content to parse it as, so we just try it with the first in the list (we respect the order in the YAML dictionary, so what the user put first will be what we try to use when no `content-type` header is provided). If an invalid `content-type` value is provided, we throw an error (pre-existing behavior). Q: Is choosing the first reasonable? What would be better? Note that, unfortunately, many servers don't send `content-type` today.

### Result

When an adopter provides multiple content types in their OpenAPI document, we generate one case in the Body enum for each! However, the new functionality is disabled by default, has to be explicitly requested by enabling the `multipleContentTypes` feature flag either on the CLI or in the config file.

### Test Plan

Added `setStats` and `getStats` operations that employ multiple content types, one in a request body, the other in a response body. Added unit tests for it in `PetstoreConsumerTests` to verify it all works correctly at runtime.

Deleted `Tests/OpenAPIGeneratorCoreTests/Translator/RequestBody/Test_translateRequestBody.swift` as snippet tests do this job better, and I didn't want to spend the time updating the test.

Adds `PetstoreConsumerTests_FF_MultipleContentTypes`, a variant of `PetstoreConsumerTests` for when the `multipleContentTypes` feature flag is enabled, so we test both how the existing logic handles multiple content types (it picks one), and the new logic (it generates code for all supported ones it found).

## TODOs
- [x] rename `IfConditionPair` to `IfBranch`
- [x] break out the first `if` from the subsequence `else if`s in `IfStatement`, to 3 properties: first if, then an array of else ifs, then an optional else; to avoid creating invalid code
- [x] create an SPI type `ContentType` to avoid repeatedly parsing the received content type



Reviewed by: gjcairo, simonjbeaumont

Builds:
     ✔︎ pull request validation (5.8) - Build finished. 
     ✔︎ pull request validation (5.9) - Build finished. 
     ✔︎ pull request validation (docc test) - Build finished. 
     ✔︎ pull request validation (integration test) - Build finished. 
     ✔︎ pull request validation (nightly) - Build finished. 
     ✔︎ pull request validation (soundness) - Build finished. 

#146
  • Loading branch information
czechboy0 authored Aug 4, 2023
1 parent 01c12f5 commit cb41d40
Show file tree
Hide file tree
Showing 36 changed files with 5,137 additions and 334 deletions.
2 changes: 1 addition & 1 deletion .swift-format
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
"OnlyOneTrailingClosureArgument" : true,
"OrderedImports" : false,
"ReturnVoidInsteadOfEmptyTuple" : true,
"UseEarlyExits" : true,
"UseEarlyExits" : false,
"UseLetInEveryBoundCaseVariable" : false,
"UseShorthandTypeNames" : true,
"UseSingleLinePropertyGetter" : false,
Expand Down
14 changes: 13 additions & 1 deletion Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ let package = Package(
),

// Tests-only: Runtime library linked by generated code
.package(url: "https://github.com/apple/swift-openapi-runtime", .upToNextMinor(from: "0.1.6")),
.package(url: "https://github.com/apple/swift-openapi-runtime", .upToNextMinor(from: "0.1.7")),

// Build and preview docs
.package(url: "https://github.com/apple/swift-docc-plugin", from: "1.0.0"),
Expand Down Expand Up @@ -143,6 +143,18 @@ let package = Package(
swiftSettings: swiftSettings
),

// PetstoreConsumerTestsFFMultipleContentTypes
// Builds and tests the reference code from GeneratorReferenceTests
// to ensure it actually works correctly at runtime.
// Enabled feature flag: multipleContentTypes
.testTarget(
name: "PetstoreConsumerTestsFFMultipleContentTypes",
dependencies: [
"PetstoreConsumerTestCore"
],
swiftSettings: swiftSettings
),

// Generator CLI
.executableTarget(
name: "swift-openapi-generator",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -664,6 +664,41 @@ struct SwitchDescription: Equatable, Codable {
var cases: [SwitchCaseDescription]
}

/// A description of an if branch and the corresponding code block.
///
/// For example: in `if foo { bar }`, the condition pair represents
/// `foo` + `bar`.
struct IfBranch: Equatable, Codable {

/// The expressions evaluated by the if statement and their corresponding
/// body blocks. If more than one is provided, an `else if` branch is added.
///
/// For example, in `if foo { bar }`, `condition` is `foo`.
var condition: Expression

/// The body executed if the `condition` evaluates to true.
///
/// For example, in `if foo { bar }`, `body` is `bar`.
var body: [CodeBlock]
}

/// A description of an if[[/elseif]/else] statement expression.
///
/// For example: `if foo { } else if bar { } else { }`.
struct IfStatementDescription: Equatable, Codable {

/// The primary `if` branch.
var ifBranch: IfBranch

/// Additional `else if` branches.
var elseIfBranches: [IfBranch]

/// The body of an else block.
///
/// No `else` statement is added when `elseBody` is nil.
var elseBody: [CodeBlock]?
}

/// A description of a do statement.
///
/// For example: `do { try foo() } catch { return bar }`.
Expand Down Expand Up @@ -709,6 +744,9 @@ enum KeywordKind: Equatable, Codable {

/// The await keyword.
case `await`

/// The throw keyword.
case `throw`
}

/// A description of an expression that places a keyword before an expression.
Expand Down Expand Up @@ -751,8 +789,14 @@ enum BinaryOperator: String, Equatable, Codable {
/// The += operator, adds and then assigns another value.
case plusEquals = "+="

/// The == operator, checks equality between two values.
case equals = "=="

/// The ... operator, creates an end-inclusive range between two numbers.
case rangeInclusive = "..."

/// The || operator, used between two Boolean values.
case booleanOr = "||"
}

/// A description of a binary operation expression.
Expand Down Expand Up @@ -832,6 +876,11 @@ indirect enum Expression: Equatable, Codable {
/// For example: `switch foo {`.
case `switch`(SwitchDescription)

/// An if statement, with optional else if's and an else statement attached.
///
/// For example: `if foo { bar } else if baz { boo } else { bam }`.
case ifStatement(IfStatementDescription)

/// A do statement.
///
/// For example: `do { try foo() } catch { return bar }`.
Expand Down Expand Up @@ -1202,6 +1251,26 @@ extension Expression {
)
}

/// Returns an if statement, with optional else if's and an else
/// statement attached.
/// - Parameters:
/// - ifBranch: The primary `if` branch.
/// - elseIfBranches: Additional `else if` branches.
/// - elseBody: The body of an else block.
static func ifStatement(
ifBranch: IfBranch,
elseIfBranches: [IfBranch] = [],
elseBody: [CodeBlock]? = nil
) -> Self {
.ifStatement(
.init(
ifBranch: ifBranch,
elseIfBranches: elseIfBranches,
elseBody: elseBody
)
)
}

/// Returns a new function call expression.
///
/// For example `foo(bar: 42)`.
Expand Down
24 changes: 24 additions & 0 deletions Sources/_OpenAPIGeneratorCore/Renderer/TextBasedRenderer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,26 @@ struct TextBasedRenderer: RendererProtocol {
return lines.joinedLines()
}

/// Renders the specified if statement.
func renderedIf(_ ifDesc: IfStatementDescription) -> String {
var lines: [String] = []
let ifBranch = ifDesc.ifBranch
lines.append("if \(renderedExpression(ifBranch.condition)) {")
lines.append(renderedCodeBlocks(ifBranch.body))
lines.append("}")
for branch in ifDesc.elseIfBranches {
lines.append("else if \(renderedExpression(branch.condition)) {")
lines.append(renderedCodeBlocks(branch.body))
lines.append("}")
}
if let elseBody = ifDesc.elseBody {
lines.append("else {")
lines.append(renderedCodeBlocks(elseBody))
lines.append("}")
}
return lines.joinedLines()
}

/// Renders the specified switch expression.
func renderedDoStatement(_ description: DoStatementDescription) -> String {
var lines: [String] = ["do {"]
Expand All @@ -201,6 +221,8 @@ struct TextBasedRenderer: RendererProtocol {
return "try\(hasPostfixQuestionMark ? "?" : "")"
case .await:
return "await"
case .throw:
return "throw"
}
}

Expand Down Expand Up @@ -268,6 +290,8 @@ struct TextBasedRenderer: RendererProtocol {
return renderedAssignment(assignment)
case .switch(let switchDesc):
return renderedSwitch(switchDesc)
case .ifStatement(let ifDesc):
return renderedIf(ifDesc)
case .doStatement(let doStmt):
return renderedDoStatement(doStmt)
case .valueBinding(let valueBinding):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,78 @@ extension FileTranslator {
return .init(content: content, typeUsage: associatedType)
}

/// Extract the supported content types.
/// - Parameters:
/// - map: The content map from the OpenAPI document.
/// - excludeBinary: A Boolean value controlling whether binary content
/// type should be skipped, for example used when encoding headers.
/// - parent: The parent type of the chosen typed schema.
/// - Returns: The supported content type + schema + type names.
func supportedTypedContents(
_ map: OpenAPI.Content.Map,
excludeBinary: Bool = false,
inParent parent: TypeName
) throws -> [TypedSchemaContent] {
let contents = supportedContents(
map,
excludeBinary: excludeBinary,
foundIn: parent.description
)
return try contents.compactMap { content in
guard
try validateSchemaIsSupported(
content.schema,
foundIn: parent.description
)
else {
return nil
}
let identifier = contentSwiftName(content.contentType)
let associatedType = try typeAssigner.typeUsage(
usingNamingHint: identifier,
withSchema: content.schema,
inParent: parent
)
return .init(content: content, typeUsage: associatedType)
}
}

/// Extract the supported content types.
/// - Parameters:
/// - contents: The content map from the OpenAPI document.
/// - excludeBinary: A Boolean value controlling whether binary content
/// type should be skipped, for example used when encoding headers.
/// - foundIn: The location where this content is parsed.
/// - Returns: the detected content type + schema, nil if no supported
/// schema found or if empty.
func supportedContents(
_ contents: OpenAPI.Content.Map,
excludeBinary: Bool = false,
foundIn: String
) -> [SchemaContent] {
guard !contents.isEmpty else {
return []
}
guard config.featureFlags.contains(.multipleContentTypes) else {
return bestSingleContent(
contents,
excludeBinary: excludeBinary,
foundIn: foundIn
)
.flatMap { [$0] } ?? []
}
return
contents
.compactMap { key, value in
parseContentIfSupported(
contentKey: key,
contentValue: value,
excludeBinary: excludeBinary,
foundIn: foundIn + "/\(key.rawValue)"
)
}
}

/// While we only support a single content at a time, choose the best one.
///
/// Priority:
Expand All @@ -72,6 +144,7 @@ extension FileTranslator {
/// - map: The content map from the OpenAPI document.
/// - excludeBinary: A Boolean value controlling whether binary content
/// type should be skipped, for example used when encoding headers.
/// - foundIn: The location where this content is parsed.
/// - Returns: the detected content type + schema, nil if no supported
/// schema found or if empty.
func bestSingleContent(
Expand All @@ -88,8 +161,81 @@ extension FileTranslator {
foundIn: foundIn
)
}
let chosenContent: (SchemaContent, OpenAPI.Content)?
if let (contentKey, contentValue) = map.first(where: { $0.key.isJSON }),
let contentType = ContentType(contentKey.typeAndSubtype)
{
chosenContent = (
.init(
contentType: contentType,
schema: contentValue.schema
),
contentValue
)
} else if let (contentKey, contentValue) = map.first(where: { $0.key.isText }),
let contentType = ContentType(contentKey.typeAndSubtype)
{
chosenContent = (
.init(
contentType: contentType,
schema: .b(.string)
),
contentValue
)
} else if !excludeBinary,
let (contentKey, contentValue) = map.first(where: { $0.key.isBinary }),
let contentType = ContentType(contentKey.typeAndSubtype)
{
chosenContent = (
.init(
contentType: contentType,
schema: .b(.string(format: .binary))
),
contentValue
)
} else {
diagnostics.emitUnsupported(
"Unsupported content",
foundIn: foundIn
)
chosenContent = nil
}
if let chosenContent {
let rawMIMEType = chosenContent.0.contentType.rawMIMEType
if rawMIMEType.hasPrefix("multipart/") || rawMIMEType.contains("application/x-www-form-urlencoded") {
diagnostics.emitUnsupportedIfNotNil(
chosenContent.1.encoding,
"Custom encoding for JSON content",
foundIn: "\(foundIn), content \(rawMIMEType)"
)
}
}
return chosenContent?.0
}

/// Returns a wrapped version of the provided content if supported, returns
/// nil otherwise.
///
/// Priority of checking for known MIME types:
/// 1. JSON
/// 2. text
/// 3. binary
///
/// - Parameters:
/// - contentKey: The content key from the OpenAPI document.
/// - contentValue: The content value from the OpenAPI document.
/// - excludeBinary: A Boolean value controlling whether binary content
/// type should be skipped, for example used when encoding headers.
/// - foundIn: The location where this content is parsed.
/// - Returns: The detected content type + schema, nil if unsupported.
func parseContentIfSupported(
contentKey: OpenAPI.ContentType,
contentValue: OpenAPI.Content,
excludeBinary: Bool = false,
foundIn: String
) -> SchemaContent? {
if contentKey.isJSON,
let contentType = ContentType(contentKey.typeAndSubtype)
{
diagnostics.emitUnsupportedIfNotNil(
contentValue.encoding,
Expand All @@ -100,27 +246,28 @@ extension FileTranslator {
contentType: contentType,
schema: contentValue.schema
)
} else if let (contentKey, _) = map.first(where: { $0.key.isText }),
}
if contentKey.isText,
let contentType = ContentType(contentKey.typeAndSubtype)
{
return .init(
contentType: contentType,
schema: .b(.string)
)
} else if !excludeBinary,
let (contentKey, _) = map.first(where: { $0.key.isBinary }),
}
if !excludeBinary,
contentKey.isBinary,
let contentType = ContentType(contentKey.typeAndSubtype)
{
return .init(
contentType: contentType,
schema: .b(.string(format: .binary))
)
} else {
diagnostics.emitUnsupported(
"Unsupported content",
foundIn: foundIn
)
return nil
}
diagnostics.emitUnsupported(
"Unsupported content",
foundIn: foundIn
)
return nil
}
}
Loading

0 comments on commit cb41d40

Please sign in to comment.