[Typescript]: add deprecated tags by gcatanese · Pull Request #21436 · OpenAPITools/openapi-generator

gcatanese · 2025-06-19T04:57:39Z

PR to add @deprecation tags when endpoints or parameters are deprecated

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 || exit
./bin/generate-samples.sh ./bin/configs/*.yaml || exit
./bin/utils/export_docs_generators.sh || exit
```
(For Windows users, please run the script in WSL)
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.x.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.

@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) @joscha (2024/10)

joscha · 2025-06-19T06:31:31Z

👏 first of all: great change

I started reviewing this PR and then noticed that I am not completely sure I understand the rubric at which what tag gets added where;

An endpoint is deprecated -> a function covering the endpoint gets the tag
A deprecated endpoint has a parameter (like status) -> the parameter gets the tag, independent of where it is used?

Is that the current behavior? Because I can see some generated functions where a parameter is marked deprecated, but the function receiving the parameter is not, even though there is no alternative, as the parameter is not optional.

Q: does marking an endpoint really "infect" all parameters?
Q: if the former, would that mean that from this point on, any function using one of the "infected" parameters would also need to be marked deprecated, if said parameter is not optional?

Cheers,
Joscha

gcatanese · 2025-06-19T20:38:16Z

👏 first of all: great change

I started reviewing this PR and then noticed that I am not completely sure I understand the rubric at which what tag gets added where;

An endpoint is deprecated -> a function covering the endpoint gets the tag

A deprecated endpoint has a parameter (like status) -> the parameter gets the tag, independent of where it is used?

Is that the current behavior? Because I can see some generated functions where a parameter is marked deprecated, but the function receiving the parameter is not, even though there is no alternative, as the parameter is not optional.

Q: does marking an endpoint really "infect" all parameters? Q: if the former, would that mean that from this point on, any function using one of the "infected" parameters would also need to be marked deprecated, if said parameter is not optional?

Cheers, Joscha

Thanks for the feedback Joscha.

The deprecation tags are independent:

if the operation is deprecated in the specs -> the method is deprecated
if a parameter is deprecated in the specs -> the parameter of the method is deprecated

I have implemented this approach because ultimately the OpenAPI definition is the source of truth. The API contract establishes what is deprecated, the generated source code just reflects that.

joscha · 2025-06-19T21:11:28Z

The deprecation tags are independent:

if the operation is deprecated in the specs -> the method is deprecated

if a parameter is deprecated in the specs -> the parameter of the method is deprecated

That makes sense. Do the samples overlap? I feel like I'm the code I reviewed I saw at least one place where both the method and a parameter was deprecated.
If yes, would it make sense to separate the fixtures, so that the effects become really clear and we can understand how the code works by looking at the generated fixtures (and see when a mistake is made)

gcatanese · 2025-06-20T04:42:59Z

The deprecation tags are independent:

if the operation is deprecated in the specs -> the method is deprecated

if a parameter is deprecated in the specs -> the parameter of the method is deprecated

That makes sense. Do the samples overlap? I feel like I'm the code I reviewed I saw at least one place where both the method and a parameter was deprecated. If yes, would it make sense to separate the fixtures, so that the effects become really clear and we can understand how the code works by looking at the generated fixtures (and see when a mistake is made)

Do you mean:

2 different unit tests with 2 different OpenAPI files?
one OpenAPI file that deprecates an operation and deprecates parameter of a different operation?
update the samples?

gcatanese · 2025-06-25T04:15:49Z

@joscha @wing328 I'd like to keep momentum and not have this PR open for too long 😊 Any essential feedback or corrections to implement now? Anything else can be improved in a second iteration

joscha · 2025-06-25T12:48:06Z

2 different unit tests with 2 different OpenAPI files?

sorry, yes, this one. So the effects of the change are clear - one change affects only the function, the other only the parameter

gcatanese · 2025-06-25T16:49:52Z

Done. Thank you

joscha

Thank you. Splitting them is much appreciated, however if we use the echo service, I still see an issue with the assertions. please let me know what you think.

joscha · 2025-06-25T19:14:50Z

+        // verify operation is deprecated
+        TestUtils.assertFileContains(
+                Paths.get(output + "/apis/DefaultApi.ts"),
+                "* @deprecated"


I see two issues with these assertions (of this test and the next): They don't assert that the deprecated tag is only put on the expected locations
I.e.:

if the tag was added to the wrong operation it would still pass the assertion.

if more than one tag was added in a different (incorrect) location in the same file, it would still pass the assertion.

it does not ensure no other files are affected (i.e. a file that is not DefaultApi.ts could have a deprecated tag on every method and every parameter and it would still pass

I know, it is not great, but this is the testing approach used in most generators. There are fundamental limitations with TestUtils (maybe there are helpers that can be used instead?) that only verifies a given content is found.

A better approach is to test the samples, using Python or Node scripts, to have the flexibility to check specific lines, for example. But this is a major effort at this stage.

I think the testing framework should improve, but not as part of this PR IMO. I can add few more assertions (if those add more value!?), but without a different approach we would still cover partially the scenarios we want to verify.

My suggestion is to move on with this PR (deprecation tags are very valuable in the generated code) and look at improving testing in a way that would benefit all generators and guide better all contributors.

maybe we can an add another assertion about the max number of expected @deprecateds in the file, so at least we identify if there's more than expected; won't help with asserting their location, but better than nothing.
Agree with the test env not being an issue of this PR.

Pls have a look at my last commit? what do you think? I have added a new method in TestUtils to assert a given text on a specific lineNumber. This might address/mitigate some of the concerns

Whilst being a good idea, I think this is too fragile. We had this come up a few times as it is fairly simple to implement, but it breaks easily when the general file context changes, independently from the asserted code. I.e. let's say someone adds an empty line somewhere in the generator, then loads of tests will fail that use line based assertions. They are tricky to fix, as you need to look at the echo response, etc.
Thus, whilst I appreciate your effort, I think longterm a line-based assertion is onerous for future changes and extremely fragile.

An assertion that applies a diff with context or similar would possibly work. So would a snapshot test with a proper diff.

I guess it is a trade-off between thorough testing and maintainability. It could work if the tests are independent and don't share the same specs or data.
Anyway, I will revert the last commit and improve the existing tests as suggested 👍

This reverts commit d3935e8.

joscha

Thank you!

gcatanese added 3 commits June 19, 2025 06:54

Add deprecation markers

5829fb7

Move inputSpec (avoid sharing with other generators)

a0c59fb

Generate samples

8fa6f33

Refactor unit tests

78a0f3b

joscha reviewed Jun 25, 2025

View reviewed changes

gcatanese added 3 commits June 25, 2025 23:14

Add test helper method

d3935e8

Revert "Add test helper method"

6902833

This reverts commit d3935e8.

Assert number of expected @deprecated

063109e

joscha approved these changes Jun 26, 2025

View reviewed changes

macjohnny added the Client: TypeScript label Jun 27, 2025

macjohnny merged commit b6b71cd into OpenAPITools:master Jun 27, 2025
15 checks passed

wing328 added this to the 7.15.0 milestone Aug 9, 2025

Uh oh!

Conversation

gcatanese commented Jun 19, 2025

PR checklist

Uh oh!

joscha commented Jun 19, 2025

Uh oh!

gcatanese commented Jun 19, 2025

Uh oh!

joscha commented Jun 19, 2025

Uh oh!

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

Uh oh!

Uh oh!

gcatanese commented Jun 25, 2025

Uh oh!

joscha commented Jun 25, 2025

Uh oh!

gcatanese commented Jun 25, 2025

Uh oh!

joscha left a comment

Choose a reason for hiding this comment

Uh oh!

joscha Jun 25, 2025

Choose a reason for hiding this comment

Uh oh!

gcatanese Jun 25, 2025

Choose a reason for hiding this comment

Uh oh!

joscha Jun 25, 2025

Choose a reason for hiding this comment

Uh oh!

gcatanese Jun 25, 2025

Choose a reason for hiding this comment

Uh oh!

joscha Jun 25, 2025

Choose a reason for hiding this comment

Uh oh!

gcatanese Jun 25, 2025

Choose a reason for hiding this comment

Uh oh!

joscha left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gcatanese commented Jun 20, 2025 •

edited

Loading