Repository navigation
Open
Labels
area/generatorAffects: plugin, CLI, config file.Affects: plugin, CLI, config file.area/runtimeAffects: the runtime library.Affects: the runtime library.kind/enhancementImprovements to existing feature.Improvements to existing feature.
Description
When using a schema that has a field from a reference that is both non-required and "nullable" this is a common way of defining it in OpenAPI: 3.1.0
UpdateTaskOccurrenceCompletionResponse:
properties:
old_task:
$ref: '#/components/schemas/TaskOccurenceDTO'
next_task:
oneOf:
- $ref: '#/components/schemas/TaskOccurenceDTO'
- type: 'null'
type: object
required:
- old_taskbut this currently generates the following API:
/// - Remark: Generated from `#/components/schemas/UpdateTaskOccurrenceCompletionResponse`.
public struct UpdateTaskOccurrenceCompletionResponse: Codable, Hashable, Sendable {
/// - Remark: Generated from `#/components/schemas/UpdateTaskOccurrenceCompletionResponse/old_task`.
public var old_task: Components.Schemas.TaskOccurenceDTO
/// - Remark: Generated from `#/components/schemas/UpdateTaskOccurrenceCompletionResponse/next_task`.
@frozen public enum next_taskPayload: Codable, Hashable, Sendable {
/// - Remark: Generated from `#/components/schemas/UpdateTaskOccurrenceCompletionResponse/next_task/case1`.
case TaskOccurenceDTO(Components.Schemas.TaskOccurenceDTO)
/// - Remark: Generated from `#/components/schemas/UpdateTaskOccurrenceCompletionResponse/next_task/case2`.
case case2(OpenAPIRuntime.OpenAPIValueContainer)
}
/// - Remark: Generated from `#/components/schemas/UpdateTaskOccurrenceCompletionResponse/next_task`.
public var next_task: Components.Schemas.UpdateTaskOccurrenceCompletionResponse.next_taskPayload
This is quite confusing syntax at the callsite, since we have this weird case2, instead of just having only an optional TaskOccurenceDTO. Is this expected behaviour or could we do something to improve the ergonomics before the 1.0.0 release? I'll happily work on a PR, if you could point me in the right direction to where in the codebase the relevant parts are!
Reacted by Yousif Al-Raheem
Metadata
Metadata
Assignees
Labels
area/generatorAffects: plugin, CLI, config file.Affects: plugin, CLI, config file.area/runtimeAffects: the runtime library.Affects: the runtime library.kind/enhancementImprovements to existing feature.Improvements to existing feature.
Activity
czechboy0 commentedon Dec 3, 2023
Hi @madsodgaard,
what's the meaning of marking the value as nullable beyond just defining as
and not putting it in the required list?
How would you want to see it represented in Swift, if not as a simple optional property of TaskOccurenceDTO?
madsodgaard commentedon Dec 3, 2023
@czechboy0 One place where I use this pattern a lot is when designing
PATCHendpoints that allows partial updates of models. If I have fieldnamethat can actually beNULLin the database, I would give that the type:since this is a partial update endpoint, I want to differentiate between the user providing a "null" value (thus updating the value of
nametoNULL) and not providing a value at all (not updatingname). That's why I don't include it in the required. From what I understandrequiredmeans that the property must be present, regardless of the value. As you said I could just not include it in therequiredand remove thenulltype. But then I would only be doing that because I know I am going to use this library to generate a client, and I'll get a nicer API.The ideal situtation, in my mind, is that the library could somehow recognize this pattern and just provide an optional property of TaskOccurenceDTO, as it does currently when not specifying the property in the
requiredlist and with only onetype. Hope that makes some sense:)czechboy0 commentedon Dec 3, 2023
Swift OpenAPI Generator delegates JSON parsing to automatic Codable implementations (wherever possible) and to Foundation's JSONDecoder, which doesn't make the distinction between a
nullvalue and an absent value, so unfortunately what you're trying to do won't work. Several pieces of the stack, including those not owned by this tool (such as Foundation itself), would have to change.You'll need to make resetting values somehow an explicit part of the API, not relying on the distinction between
nulland an absent value. And for values you're not trying to overwrite, just define them as regular properties and omit them from the required array.madsodgaard commentedon Dec 10, 2023
@czechboy0 Thanks! Of course the fact that Codable does not differentiate between absent values and null is quite challenge to this issue. But, this pattern is so common in APIs today, that perhaps we should find a way to support? By "this pattern", I am referring to PATCH endpoints for partial updates instead of PUT endpoints to replace the entire resource. I have used this property wrapper previously in server-side applications. Do you think such a type or something similar would have a place in the library? (or is it too niché?) It would be used in places where you have a non-required nullable field.
czechboy0 commentedon Dec 10, 2023
I don't think it's likely for us to try to solve this in Swift OpenAPI Generator. The fix should be in Codable and JSONDecoder. Once it's there, we can see what'd remain for Swift OpenAPI Generator to have to do to support this case.
But only doing it here would be a significant amount of work that introduces a lot of complexity.
One way this could be better represented that'd work today would be a PATCH call that takes two values, a "toSet" dictionary, which has keys matching the property to set, and the value to set. And if a value is nil for a property, the property isn't changed.
And the second value would be called "toReset", which would just be a list of property names whose values to reset to their default.
[-]Improve ergonomics of inline nullable refs[/-][+]Surface the distinction between an explicit "null" value and an absent value[/+]czechboy0 commentedon Dec 11, 2023
Renamed the issue to reflect that this is a missing feature. See my comment above explaining that this goes deeper in the stack we're based on, so would likely need some improvements from JSONDecoder first, but keeping open to allow tracking progress.
brandonbloom commentedon Feb 19, 2024
I ran into this (or a similar) issue. The encoded output of my server omits a field with a nil value, but some code consuming this service expects an explicit null value.
One idea is this property wrapper: https://stackoverflow.com/a/53142581/67280
czechboy0 commentedon Feb 19, 2024
I think it'd be best to first help Foundation get the desired behavior, and then Swift OpenAPI Generator and all other projects relying on JSONDecoder get it for free.
https://github.com/apple/swift-foundation
18 remaining items
simonjbeaumont commentedon Mar 21, 2025
I understand, but the tradeoff isn't symmetric. Users consuming documents with inline schemas in PATCH requests will be forced to do a significant refactor of the document (cf. just add an annotation).
I'm not suggestion we only do the extension and not config—my original suggestion upthread was to consider both—just that if we're really gonna pick one, I would suggest picking the one that solves the problem for all cases.
I'm sympathetic to folks preferring the config file where that is an option to them: is it significantly more work to offer both?
simonjbeaumont commentedon Mar 21, 2025
Or, alternatively, is it possible to identify an inline schema from the config file, by some JSON Path syntax? That way we could do only config but still support inline schema PATCH requests.
czechboy0 commentedon Mar 21, 2025
Yeah maybe, here's the OpenAPIKit impl of it: https://github.com/mattpolzin/OpenAPIKit/blob/fd27b297993923c9a1725dc62553d06579c7a33e/Sources/OpenAPIKit30/JSONReference.swift#L193
But since this is growing in scope, I might not be able to get to this as quickly as I hoped, just a heads up. If anyone else wants to investigate this, go ahead.
nikitaame commentedon Mar 22, 2025
Thanks for the continued discussion here!
@czechboy0 what was the reason for the shift in preference to do this in the config rather than the document considering the option to do OpenAPI Overlays?
Btw, here's the implementation of the extension in OpenAPITools.
czechboy0 commentedon Mar 22, 2025
I think in the fullness of time we could support all of these, just that the quickest and easiest to implement is the config approach + named schemas only. That'll only require changes in a handful of methods in the codegen logic, and historically adopters often preferred a codegen option over having to edit their OpenAPI doc.
The OpenAPI doc + overlay, and inline schema support are both great enhancements, they just also add quite a lot of scope both in generator implementation and documentation enhancements to show it all working end-to-end.
jpsim commentedon Mar 25, 2025
I would love if there was at least one way to achieve this and would hope that the approach can then be expanded over time to support the full extent of use cases. Right now there's no way to achieve this and no workaround has been found.
simonjbeaumont commentedon Mar 27, 2025
One alternative we discussed a little, is to solve this in a way that uses additive API without the need to opt-in for specific operations or schemas, which would hopefully make it more widely useful, more discoverable, and use a unified implementation under the hood.
The idea would be to add a side-array (likely an OptionSet) to each generated type for schemas with at least one nullable property, with one option per nullable property, matching the generated name of the property. For example:
This would require us to stop relying on the synthesized
Codableconformance or to provide a wrapped JSON encoder and decoder.I'm curious about feedback here as it seems to be a more general solution to the problem of surfacing and influencing explicitly null properties.
nikitaame commentedon Mar 27, 2025
I like the flexibility and built in discoverability this solution provides!
We'd probably only want this functionality exposed to
PATCHendpoints but otherwise looks good to me 🫡simonjbeaumont commentedon Mar 28, 2025
This would be on the schema, but I suppose we could only apply it to schemas that are used in at least one PATCH request. Are there any other scenarios where distinguishing explicitly- vs implicitly-
nilis useful?czechboy0 commentedon Mar 28, 2025
Personally, I'd prefer for this to be opt-in more explicitly. Searching all operations and identifying which schemas end up being (transitively) used by PATCH operations could result in unexpected spooky action at a distance.
nikitaame commentedon Mar 28, 2025
For simplicity, perhaps doesn't matter to search if the schemas is used in at least one
PATCHendpoint and can just generally apply to all schemas that have at least one nullable field.Not seeing any other scenarios where this distinction can be beneficial but also don't see anything spooky with applying to all schemas with a nullable field.
@czechboy0 thoughts if we apply as originally suggested? Still think it should be more explicit? If so, which of the more explicit solutions are you leaning towards?
simonjbeaumont commentedon Mar 28, 2025
This was my original suggestion in #419 (comment). If we managed to achieve this with a custom Encoder/Decoder instead of having to provide a custom Codable conformance then it shouldn't drastically affect the generated code size either.
nikitaame commentedon Mar 28, 2025
Yep, totally good with the original suggestion!
czechboy0 commentedon Jun 18, 2025
A different feature landed that offers a temporary workaround for the lack of proper PATCH support here - you can hand-write the request body to correctly encode the difference between null and absent, and then use that type instead of the generated one: https://swiftpackageindex.com/apple/swift-openapi-generator/main/documentation/swift-openapi-generator/configuring-the-generator#Type-overrides
= nildefault and are omitted on encode, so a conforming request is impossible to construct #934 as a duplicate of this issue