[Typescript Fetch] Handle date parameters in the URL path #19313

scottaj · 2024-08-07T00:04:43Z

If a schema contains a date or date-time parameter in the url path, the typescript-fetch generator will expect that parameter to be passed as a JavaScript Date object. When serializing date objects for other parameter types, the generator serializes them as ISO8601 strings. For path parameters however, it is currently just stringifying the date object, leading to values like 'Tue Aug 06 2024 17:49:37 GMT-0600 (Mountain Daylight Time)' instead of '2024-08-06T23:49:37.781Z'. This causes issues with many webservers that expect the date to be in the ISO8601 format.

This pull request changes the behavior of path params to match the existing behavior of query and body params and use the ISO8601 format.

I didn't see a past issue for this exact problem, some relevant issues and past PRs I did find:

A recent change that did the same fix for url bodies
I think this might actually be a bug report of the issue from a couple years ago. The title mentions query params, but the example spec in the issue is a path param and would reproduce this bug

CC: @TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) @topce (2018/10) @akehir (2019/07) @petejohansonxo (2019/11) @amakhrov (2020/02) @davidgamero (2022/03) @mkusaka (2022/04)

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.6.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.

scottaj · 2024-08-07T00:08:02Z

Hopefully this PR is helpful, I have currently run into this behavior breaking calls to an API.

A couple questions I had:

I think it is correct to open this against master, and the recent very similar PR for form bodies was, but it is technically a breaking change if anyone was relying on the old date format, so should it be against the next major release instead?
The petstore example doesn't have any endpoints that hit this use case, it was unclear to me if I should somehow update that to include one?

macjohnny

Thanks for your contribution!

macjohnny · 2024-08-07T08:11:55Z

modules/openapi-generator/src/main/resources/typescript-fetch/apis.mustache

        {{/hasFormParams}}
        const response = await this.request({
-            path: `{{{path}}}`{{#pathParams}}.replace(`{${"{{baseName}}"}}`, encodeURIComponent(String(requestParameters['{{paramName}}']))){{/pathParams}},
+            path: `{{{path}}}`{{#pathParams}}{{#isDateTimeType}}.replace(`{${"{{baseName}}"}}`, encodeURIComponent(requestParameters['{{paramName}}'].toISOString())){{/isDateTimeType}}{{^isDateTimeType}}{{#isDateType}}.replace(`{${"{{baseName}}"}}`, encodeURIComponent(requestParameters['{{paramName}}'].toISOString().substring(0,10))){{/isDateType}}{{^isDateType}}.replace(`{${"{{baseName}}"}}`, encodeURIComponent(String(requestParameters['{{paramName}}']))){{/isDateType}}{{/isDateTimeType}}{{/pathParams}},


how about

Suggested change

path: `{{{path}}}`{{#pathParams}}{{#isDateTimeType}}.replace(`{${"{{baseName}}"}}`, encodeURIComponent(requestParameters['{{paramName}}'].toISOString())){{/isDateTimeType}}{{^isDateTimeType}}{{#isDateType}}.replace(`{${"{{baseName}}"}}`, encodeURIComponent(requestParameters['{{paramName}}'].toISOString().substring(0,10))){{/isDateType}}{{^isDateType}}.replace(`{${"{{baseName}}"}}`, encodeURIComponent(String(requestParameters['{{paramName}}']))){{/isDateType}}{{/isDateTimeType}}{{/pathParams}},

path: `{{{path}}}`.replace(`{${"{{baseName}}"}}`,

{{#pathParams}}

{{#isDateTimeType}}

encodeURIComponent(requestParameters['{{paramName}}'].toISOString())

{{/isDateTimeType}}

{{^isDateTimeType}}

{{#isDateType}}

encodeURIComponent(requestParameters['{{paramName}}'].toISOString().substring(0,10))

{{/isDateType}}

{^isDateType}}

encodeURIComponent(String(requestParameters['{{paramName}}']))

{{/isDateType}}

{{/isDateTimeType}}

{{/pathParams}},

to simplify the template?

moreover, encodeURIComponent(requestParameters['{{paramName}}'].toISOString().substring(0,10)) is problematic because devs might be using a local date, and this converts the date to be in UTC. But at least this matches

openapi-generator/modules/openapi-generator/src/main/resources/typescript-fetch/modelGeneric.mustache

Line 114 in f8a5051

'{{baseName}}': {{^required}}value['{{name}}'] == null ? undefined : {{/required}}({{#required}}{{#isNullable}}value['{{name}}'] == null ? null : {{/isNullable}}{{/required}}(value['{{name}}']{{#isNullable}} as any{{/isNullable}}).toISOString().substring(0,10)),

I can do the expanded style. I wasn't sure what the standards (if any are for the generated code, so I was trying to maintain the style of that. But your version makes the template more readable.

Regarding the iso string handling, I pretty much just copied the code above that is handling query params (line 140 of this file). So while I don't disagree about the UTC thing, I think if that is an issue it might need to be addressed as part of a larger change

I can add the null handling stuff you have, but are path params allowed to be nullable? I'll admit I don't know the spec well enough to be sure but that seems really weird. I suppose in that case though that putting in null as the path value is the correct behavior?

Regarding the nullable bit, my reading of the spec (both version 2 and version 3.1.0) is that a path parameter can't be null because the required property must be set to true: https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.1.0.md#fixed-fields-10

Determines whether this parameter is mandatory. If the parameter location is "path", this property is REQUIRED and its value MUST be true. Otherwise, the property MAY be included and its default value is false.

We can also just add the null-safe operator „.?“ to be cautious

macjohnny · 2024-08-08T06:38:19Z

@scottaj thanks for your contribution
Can you please re-generate the samples? See PR description

scottaj · 2024-08-08T07:57:39Z

I did run those commands from the PR template. I don't believe any of the samples have a date as a path param, so running those did not result in any changes in git.

macjohnny · 2024-08-08T08:51:21Z

please run

./mvnw clean package 
./bin/generate-samples.sh ./bin/configs/*.yaml
./bin/utils/export_docs_generators.sh

to re-generate the samples from your changed code.

Couldn't get handlebars to format the inline replaces in a sane way without collapsing everything onto one very ugly line. This way keeps some sane template formatting while still outputting workable code.

scottaj · 2024-08-20T16:20:43Z

@macjohnny I updated the template and examples.

I ended up changing the template code structure because I could not get handlebars to format the inline replace calls in a nice way without collapsing the whole template section onto a single line like it was before.

NiltiakSivad · 2025-05-02T05:13:56Z

Thanks @scottaj for putting this together. I also need this change... @macjohnny any reason to not merge this change?

macjohnny · 2025-05-02T07:48:20Z

oops, this one slipped my mind. @scottaj can you merge the most recent master and re-generate the samples please?

macjohnny · 2025-05-02T07:49:33Z

modules/openapi-generator/src/main/resources/typescript-fetch/apis.mustache

+        let urlPath = `{{{path}}}`;
+        {{#pathParams}}
+        {{#isDateTimeType}}
+        urlPath = urlPath.replace(`{${"{{baseName}}"}}`, encodeURIComponent((requestParameters['{{paramName}}'] as any).toISOString()));


please also check here whether it actually is a date object, i.e. instanceof Date

same below

If it isn't a date what should we do? Obvious options seem to be to stringify whatever it is, or throw an error in the generated code

Also, just to call out, none of the other parameter types are making that check now (query params, form params). Assume that if I add the check I should update all those to have the same handling?

yeah if its not a Date, just stringify. not sure this can happen in typescript-fetch, but just to be sure.
the other places (query params) should probably also have some safeguard, but ideally in a separate PR

Added type checking to date params: 373a34c

…-fetch/dates-in-path-params

scottaj · 2025-05-09T19:26:30Z

@macjohnny Merged with master, regenerated samples, and updated the Date checking

macjohnny · 2025-05-14T10:10:28Z

somehow circleci failed

can you

merge the latest master again?

…-fetch/dates-in-path-params

scottaj · 2025-05-14T18:10:24Z

I merged master again. I am seeing this error for CircleCI:

Could not find a usable config.yml, you may have revoked the CircleCI OAuth app.

I haven't touched any CI configs, and i don't think I would be the one to give permission to that? So I'm not sure what to do.

macjohnny · 2025-06-16T10:56:39Z

@scottaj sorry i dont know how to fix the circle ci config issue. can you please create a new PR, based on the latest master?

maybe you can also try the steps described in https://support.circleci.com/hc/en-us/articles/12504745606171-Why-am-I-seeing-the-Could-not-find-a-usable-config-yml-you-may-have-revoked-the-CircleCI-OAuth-app-message

scottaj · 2025-06-18T17:07:30Z

@macjohnny that was a good call. I followed the steps in that link and reset everything for CircleCI and now it seems to have worked.

Handle date parameters in the URL path

7747522

macjohnny reviewed Aug 7, 2024

View reviewed changes

Reformat template and add cast for date fields

2c3501c

Change code structure by moving path tranformations to assignment

ffa3354

Couldn't get handlebars to format the inline replaces in a sane way without collapsing everything onto one very ugly line. This way keeps some sane template formatting while still outputting workable code.

macjohnny reviewed May 2, 2025

View reviewed changes

scottaj added 3 commits May 2, 2025 11:09

Merge remote-tracking branch 'upstream/master' into bugfix/typescript…

78b74c2

…-fetch/dates-in-path-params

Regenerate samples after merge

244190e

Updated param checking

373a34c

Merge remote-tracking branch 'upstream/master' into bugfix/typescript…

9dbfa08

…-fetch/dates-in-path-params

macjohnny closed this May 16, 2025

macjohnny reopened this May 16, 2025

Try a new commit after manually re-logging into CircleCI

b24c0be

scottaj force-pushed the bugfix/typescript-fetch/dates-in-path-params branch from 17f6d99 to b24c0be Compare June 18, 2025 15:59

macjohnny merged commit 88bc79e into OpenAPITools:master Jun 19, 2025
15 checks passed

macjohnny added the Client: TypeScript label Jun 19, 2025

wing328 added this to the 7.14.0 milestone Jun 19, 2025

Loeffeldude mentioned this pull request Jul 11, 2025

[Typescript Fetch] Fix missing closing bracket in date handling #21549

Merged

4 tasks

Uh oh!

[Typescript Fetch] Handle date parameters in the URL path #19313

[Typescript Fetch] Handle date parameters in the URL path #19313

Uh oh!

Conversation

scottaj commented Aug 7, 2024

PR checklist

Uh oh!

scottaj commented Aug 7, 2024

Uh oh!

macjohnny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

macjohnny commented Aug 8, 2024

Uh oh!

scottaj commented Aug 8, 2024

Uh oh!

macjohnny commented Aug 8, 2024

Uh oh!

scottaj commented Aug 20, 2024

Uh oh!

NiltiakSivad commented May 2, 2025

Uh oh!

macjohnny commented May 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottaj commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

macjohnny commented May 14, 2025

Uh oh!

scottaj commented May 14, 2025

Uh oh!

macjohnny commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scottaj commented Jun 18, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

scottaj commented May 9, 2025 •

edited

Loading

macjohnny commented Jun 16, 2025 •

edited

Loading