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

[Generator] Add support of deepObject style in query params #659

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

[Generator] Add support of deepObject style in query params #659

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Package.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ let package = Package(
// Tests-only: Runtime library linked by generated code, and also
// helps keep the runtime library new enough to work with the generated
// code.
.package(url: "https://github.com/apple/swift-openapi-runtime", from: "1.3.2"),
.package(url: "https://github.com/apple/swift-openapi-runtime", from: "1.4.0"),
.package(url: "https://github.com/apple/swift-http-types", from: "1.0.2"),
],
targets: [
Expand Down
3 changes: 3 additions & 0 deletions Sources/_OpenAPIGeneratorCore/Translator/CommonTypes/Constants.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,9 @@ enum Constants {

/// The form style.
static let form = "form"

/// The deepObject style.
static let deepObject = "deepObject"
}
}

Expand Down
6 changes: 5 additions & 1 deletion Sources/_OpenAPIGeneratorCore/Translator/Parameters/TypedParameter.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,10 @@ extension FileTranslator {
let location = parameter.location
switch location {
case .query:
guard case .form = style else {
switch style {
case .form, .deepObject:
break
default:
Comment on lines +133 to +136
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
switch style {
case .form, .deepObject:
break
default:
switch style {
case .form:
break
case .deepObject where explode:
break
default:

Copy link
Contributor

Choose a reason for hiding this comment

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

style: deepObject + explode: true is supported, but not with explode: false, so let's catch it here and provide a descriptive error.

See in the runtime changes that we only support explode: true: https://github.com/apple/swift-openapi-runtime/pull/100/files#diff-a382c8525d6209dfbc3e90daaf0262692d9df73f1b7a914b9eebe143f04c287dR76

try diagnostics.emitUnsupported(
"Query params of style \(style.rawValue), explode: \(explode)",
foundIn: foundIn
Expand Down Expand Up @@ -243,6 +246,7 @@ extension OpenAPI.Parameter.SchemaContext.Style {
var runtimeName: String {
switch self {
case .form: return Constants.Components.Parameters.Style.form
case .deepObject: return Constants.Components.Parameters.Style.deepObject
default: preconditionFailure("Unsupported style")
}
}
Expand Down
12 changes: 12 additions & 0 deletions Tests/OpenAPIGeneratorReferenceTests/Resources/Docs/petstore.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,18 @@ paths:
schema:
format: uuid
type: string
- name: sort
in: query
required: false
style: deepObject
explode: true
schema:
type: object
properties:
id:
type: string
name:
type: string
- $ref: '#/components/parameters/query.born-since'
responses:
'200':
Expand Down
7 changes: 7 additions & 0 deletions Tests/OpenAPIGeneratorReferenceTests/Resources/ReferenceSources/Petstore/Client.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,13 @@ public struct Client: APIProtocol {
name: "My-Request-UUID",
value: input.headers.My_hyphen_Request_hyphen_UUID
)
try converter.setQueryItemAsURI(
in: &request,
style: .deepObject,
explode: true,
name: "sort",
value: input.query.sort
)
try converter.setQueryItemAsURI(
in: &request,
style: .form,
Expand Down
7 changes: 7 additions & 0 deletions Tests/OpenAPIGeneratorReferenceTests/Resources/ReferenceSources/Petstore/Server.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,13 @@ fileprivate extension UniversalServer where APIHandler: APIProtocol {
name: "feeds",
as: Operations.listPets.Input.Query.feedsPayload.self
),
sort: try converter.getOptionalQueryItemAsURI(
in: request.soar_query,
style: .deepObject,
explode: true,
name: "sort",
as: Operations.listPets.Input.Query.sortPayload.self
),
since: try converter.getOptionalQueryItemAsURI(
in: request.soar_query,
style: .form,
Expand Down
28 changes: 28 additions & 0 deletions Tests/OpenAPIGeneratorReferenceTests/Resources/ReferenceSources/Petstore/Types.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1855,6 +1855,31 @@ public enum Operations {
public typealias feedsPayload = [Operations.listPets.Input.Query.feedsPayloadPayload]
/// - Remark: Generated from `#/paths/pets/GET/query/feeds`.
public var feeds: Operations.listPets.Input.Query.feedsPayload?
/// - Remark: Generated from `#/paths/pets/GET/query/sort`.
public struct sortPayload: Codable, Hashable, Sendable {
/// - Remark: Generated from `#/paths/pets/GET/query/sort/id`.
public var id: Swift.String?
/// - Remark: Generated from `#/paths/pets/GET/query/sort/name`.
public var name: Swift.String?
/// Creates a new `sortPayload`.
///
/// - Parameters:
/// - id:
/// - name:
public init(
id: Swift.String? = nil,
name: Swift.String? = nil
) {
self.id = id
self.name = name
}
public enum CodingKeys: String, CodingKey {
case id
case name
}
}
/// - Remark: Generated from `#/paths/pets/GET/query/sort`.
public var sort: Operations.listPets.Input.Query.sortPayload?
/// Supply this parameter to filter pets born since the provided date.
///
/// - Remark: Generated from `#/paths/pets/GET/query/since`.
Expand All @@ -1865,16 +1890,19 @@ public enum Operations {
/// - limit: How many items to return at one time (max 100)
/// - habitat:
/// - feeds:
/// - sort:
/// - since: Supply this parameter to filter pets born since the provided date.
public init(
limit: Swift.Int32? = nil,
habitat: Operations.listPets.Input.Query.habitatPayload? = nil,
feeds: Operations.listPets.Input.Query.feedsPayload? = nil,
sort: Operations.listPets.Input.Query.sortPayload? = nil,
since: Components.Parameters.query_period_born_hyphen_since? = nil
) {
self.limit = limit
self.habitat = habitat
self.feeds = feeds
self.sort = sort
self.since = since
}
}
Expand Down
48 changes: 46 additions & 2 deletions Tests/OpenAPIGeneratorReferenceTests/SnippetBasedReferenceTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2514,6 +2514,18 @@ final class SnippetBasedReferenceTests: XCTestCase {
type: array
items:
type: string
- name: sort
in: query
required: true
style: deepObject
explode: true
schema:
type: object
properties:
option1:
type: string
option2:
type: string
responses:
default:
description: Response
Expand All @@ -2524,18 +2536,36 @@ final class SnippetBasedReferenceTests: XCTestCase {
public var single: Swift.String?
public var manyExploded: [Swift.String]?
public var manyUnexploded: [Swift.String]?
public struct sortPayload: Codable, Hashable, Sendable {
public var option1: Swift.String?
public var option2: Swift.String?
public init(
option1: Swift.String? = nil,
option2: Swift.String? = nil
) {
self.option1 = option1
self.option2 = option2
}
public enum CodingKeys: String, CodingKey {
case option1
case option2
}
}
public var sort: Operations.get_sol_foo.Input.Query.sortPayload
public init(
single: Swift.String? = nil,
manyExploded: [Swift.String]? = nil,
manyUnexploded: [Swift.String]? = nil
manyUnexploded: [Swift.String]? = nil,
sort: Operations.get_sol_foo.Input.Query.sortPayload
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
sort: Operations.get_sol_foo.Input.Query.sortPayload
sort: Operations.get_sol_foo.Input.Query.sortPayload = nil

This should end up being defaulted to nil as well. It's possible it'll require improvements in inspecting the nested object and whether it can be initialized with .init().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That parameter has required: true though, I don't think it should be defaulted tonil. It should be defaulted to .init() instead I think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh that's interesting! What does it mean for a query parameter of type object to be required, when all properties are optional?

On the wire, it won't look any different to the whole parameter being optional.

Maybe we should just change the sample to have an optional parameter? I think that's more common. Alternatively, make one of the properties required. Either is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, that's interesting. Updating the sample to have an optional parameter would resolve the issues you highlighted in the review with this test.

What if all the properties of the object have default values though? I tried getting an idea of what it would look like by updating the tests but I can get any default value to show in the generated swift types.

) {
self.single = single
self.manyExploded = manyExploded
self.manyUnexploded = manyUnexploded
self.sort = sort
}
}
public var query: Operations.get_sol_foo.Input.Query
public init(query: Operations.get_sol_foo.Input.Query = .init()) {
public init(query: Operations.get_sol_foo.Input.Query) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public init(query: Operations.get_sol_foo.Input.Query) {
public init(query: Operations.get_sol_foo.Input.Query = .init()) {

Yeah this should not go away, since all the query items are still optional, so it should be possible to init the Input with just .init().

self.query = query
}
}
Expand Down Expand Up @@ -2572,6 +2602,13 @@ final class SnippetBasedReferenceTests: XCTestCase {
name: "manyUnexploded",
value: input.query.manyUnexploded
)
try converter.setQueryItemAsURI(
in: &request,
style: .deepObject,
explode: true,
name: "sort",
value: input.query.sort
)
return (request, nil)
}
""",
Expand All @@ -2598,6 +2635,13 @@ final class SnippetBasedReferenceTests: XCTestCase {
explode: false,
name: "manyUnexploded",
as: [Swift.String].self
),
sort: try converter.getRequiredQueryItemAsURI(
in: request.soar_query,
style: .deepObject,
explode: true,
name: "sort",
as: Operations.get_sol_foo.Input.Query.sortPayload.self
)
)
return Operations.get_sol_foo.Input(query: query)
Expand Down
Loading