Skip to content

[Swift] fix(#18074): correctly map OpenAPIDateWithoutTime to string in path#18077

Merged
4brunu merged 1 commit into
OpenAPITools:masterfrom
kalinjul:fix-swift-date
Jul 15, 2024
Merged

[Swift] fix(#18074): correctly map OpenAPIDateWithoutTime to string in path#18077
4brunu merged 1 commit into
OpenAPITools:masterfrom
kalinjul:fix-swift-date

Conversation

@kalinjul
Copy link
Copy Markdown
Contributor

@kalinjul kalinjul commented Mar 11, 2024

This fixes #18074, OpenAPIDateWithoutTime handling when a date is part of the uri path (parameter).
There are several options to fix the issue, this is just one suggestion.

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh ./bin/configs/*.yaml
    ./bin/utils/export_docs_generators.sh
    
    (For Windows users, please run the script in Git BASH)
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
  • File the PR against the correct branch: master (upcoming 7.1.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@wing328
Copy link
Copy Markdown
Member

wing328 commented Mar 11, 2024

the CI tests failed. can you please take a look?

cc
@jgavris (2017/07) @ehyche (2017/08) @Edubits (2017/09) @jaz-ah (2017/09) @4brunu (2019/11) @dydus0x14 (2023/06)

@kalinjul
Copy link
Copy Markdown
Contributor Author

CI was right, hope it's fixed now.

BTW: Another possible solution would be for OpenAPIDateWithoutTime to implement RawRepresentable and handle RawRepresentable in mapValueToPathItem, too. I'm not sure if either solution is better, though.

Also, with useCustomDateWithoutTime = false, paths would still be broken with this patch. But that's no regression and a fix would require Date Formatting.

@4brunu
Copy link
Copy Markdown
Contributor

4brunu commented Mar 11, 2024

Hi @kalinjul thanks for creating this PR.
If we make it conformable with raw representale, it would work everywhere without any changes elsewhere, right?
Which solution do you prefer?
Do you see other options?
Thanks

@kalinjul
Copy link
Copy Markdown
Contributor Author

kalinjul commented Mar 12, 2024

@4brunu : we would still need an additional case here (but for RawRepresentable instead of DateWithoutTime)

if let date = source as? OpenAPIDateWithoutTime {
return date.encodeToJSON()
}{{/useCustomDateWithoutTime}}
return source
but the special case here
return "\(value.rawValue)"
{{#useCustomDateWithoutTime}}} else if let date = value as? OpenAPIDateWithoutTime {
return "\(date.encodeToJSON())"
{{/useCustomDateWithoutTime}}} else {
return "\(value)"
}
}
would no longer be necessary, because in Collections RawRepresentable is already treated correctly.

I'm really not sure which is better. I decided for the solution in this PR, because there are no other custom data types introduced by the generator, so i guessed special treatment for this one would be fair.
On the other hand, there already is a case for RawPresentable in Collections, so they should be converted correctly in non-collections, as well - this should be added in any case.

If you are leaning towards the RawPresentable solution, I'd happily agree and change the PR.

@4brunu
Copy link
Copy Markdown
Contributor

4brunu commented Mar 13, 2024

I think the RawPresentable solution would be simpler, what do you think?
By the way, do you know if Date properties have the same problem as OpenAPIDateWithoutTime, or if they work correctly?
Thanks

@kalinjul
Copy link
Copy Markdown
Contributor Author

I updated the branch with the RawRepresentable solution.
As far as i know, Date will have the same problem when used as path parameter (actually I'm not sure why it would work as body parameter as i can't find any JSON serializing logic that would only output the Date).
Maybe useCustomDateWithoutTime should just be true by default..

@4brunu
Copy link
Copy Markdown
Contributor

4brunu commented Apr 10, 2024

Sorry, I forgot about this PR.
We can't enable useCustomDateWithoutTime by default because that assumes that dates don't have time.
I think Dates work because of the JSONEconder/JSONDecoder dateEncodingStrategy

encoder.dateEncodingStrategy = .formatted(CodableHelper.dateFormatter)

decoder.dateDecodingStrategy = .formatted(CodableHelper.dateFormatter)

Actually if OpenAPIDateWithoutTime would inherit from Date it would work out of the box everywhere without any code changes.

@kalinjul
Copy link
Copy Markdown
Contributor Author

Sorry, I forgot about this PR.

Ok now i forgot about this too, but today i saw our workaround in generated code and remembered :)

We can't enable useCustomDateWithoutTime by default because that assumes that dates don't have time.

I think useCustomDateWithoutTime only applies to format: date-only, which is supposed to encode a date only.
You can still have attributes with format: date which would be encoded as Date in swift.
I hate swift for not having useful date types, so I'd be happy with useCustomDateWithoutTime = true by default, but this is not the scope of this PR i guess.

Actually if OpenAPIDateWithoutTime would inherit from Date it would work out of the box everywhere without any code changes.

But then a format: date would be encoded as date-time, even if the OpenAPIDateWithoutTime was used.
However, maybe most Apis would be fine with this?

But: Date is a struct, we cannot inherit from it anyways, so that's not a solution.

@4brunu
Copy link
Copy Markdown
Contributor

4brunu commented Jul 15, 2024

Thanks for the explanation and the contribution, looks good to me. 👍

@4brunu 4brunu merged commit 304ff96 into OpenAPITools:master Jul 15, 2024
@wing328 wing328 added this to the 7.8.0 milestone Aug 18, 2024
@kalinjul kalinjul deleted the fix-swift-date branch November 4, 2024 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Swift: Date as Path parameter not correctly mapped to String

3 participants