Skip to content

[Typescript]: add deprecated tags #21436

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

Merged
macjohnny merged 7 commits into from
Jun 27, 2025
Merged

[Typescript]: add deprecated tags #21436

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5829fb7
Add deprecation markers
gcatanese Jun 19, 2025
a0c59fb
Move inputSpec (avoid sharing with other generators)
gcatanese Jun 19, 2025
8fa6f33
Generate samples
gcatanese Jun 19, 2025
78a0f3b
Refactor unit tests
gcatanese Jun 25, 2025
d3935e8
Add test helper method
gcatanese Jun 25, 2025
6902833
Revert "Add test helper method"
gcatanese Jun 25, 2025
063109e
Assert number of expected @deprecated
gcatanese Jun 26, 2025
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 bin/configs/typescript-echo-api.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
generatorName: typescript
outputDir: samples/client/echo_api/typescript/build
inputSpec: modules/openapi-generator/src/test/resources/3_0/echo_api.yaml
inputSpec: modules/openapi-generator/src/test/resources/3_0/typescript/echo_api.yaml
templateDir: modules/openapi-generator/src/main/resources/typescript
additionalProperties:
artifactId: echo-api-typescript
Expand Down
6 changes: 5 additions & 1 deletion modules/openapi-generator/src/main/resources/typescript/api/api.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,18 @@ export class {{classname}}RequestFactory extends BaseAPIRequestFactory {

{{#operation}}
/**
{{#isDeprecated}}
* @deprecated
*
{{/isDeprecated}}
{{#notes}}
* {{&notes}}
{{/notes}}
{{#summary}}
* {{&summary}}
{{/summary}}
{{#allParams}}
* @param {{paramName}} {{description}}
* @param {{paramName}} {{description}}{{#isDeprecated}} (@deprecated){{/isDeprecated}}
{{/allParams}}
*/
public async {{nickname}}({{#allParams}}{{paramName}}{{^required}}?{{/required}}: {{{dataType}}}, {{/allParams}}_options?: Configuration): Promise<RequestContext> {
Expand Down
17 changes: 17 additions & 0 deletions modules/openapi-generator/src/test/java/org/openapitools/codegen/TestUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import static org.testng.Assert.*;

Expand Down Expand Up @@ -177,6 +179,21 @@ public static void assertFileContains(Path path, String... lines) {
}
}

/**
* Count occurrences of the given text
* @param content content of the file
* @param text text to find
* @return
*/
public static int countOccurrences(String content, String text) {
Matcher matcher = Pattern.compile(text).matcher(content);
int count = 0;
while (matcher.find()) {
count++;
}
return count;
}

public static String linearize(String target) {
return target.replaceAll("\r?\n", "").replaceAll("\\s+", "\\s");
}
Expand Down
57 changes: 57 additions & 0 deletions ...erator/src/test/java/org/openapitools/codegen/typescript/TypeScriptClientCodegenTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,14 @@

import java.io.File;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.Collections;
import java.util.List;
import java.util.Map;

import static org.junit.jupiter.api.Assertions.assertEquals;

@Test(groups = {TypeScriptGroups.TYPESCRIPT})
public class TypeScriptClientCodegenTest {
@Test
Expand Down Expand Up @@ -210,4 +213,58 @@ public void testForAllSanitizedEnum() throws Exception {
"}"
);
}

@Test
public void testDeprecatedOperation() throws Exception {
final File output = Files.createTempDirectory("typescriptnodeclient_").toFile();
output.deleteOnExit();

final CodegenConfigurator configurator = new CodegenConfigurator()
.setGeneratorName("typescript")
.setInputSpec("src/test/resources/3_0/typescript/deprecated-operation.yaml")
.setOutputDir(output.getAbsolutePath().replace("\\", "/"));

final ClientOptInput clientOptInput = configurator.toClientOptInput();
final DefaultGenerator generator = new DefaultGenerator();
final List<File> files = generator.opts(clientOptInput).generate();
files.forEach(File::deleteOnExit);

// verify operation is deprecated
Path file = Paths.get(output + "/apis/DefaultApi.ts");
TestUtils.assertFileContains(
file,
"* @deprecated"
Copy link
Copy Markdown
Contributor

@joscha joscha Jun 25, 2025

Choose a reason for hiding this comment

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

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

  1. if the tag was added to the wrong operation it would still pass the assertion.
  2. if more than one tag was added in a different (incorrect) location in the same file, it would still pass the assertion.
  3. 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

Copy link
Copy Markdown
Contributor Author

@gcatanese gcatanese Jun 25, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@joscha joscha Jun 25, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@gcatanese gcatanese Jun 25, 2025

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@joscha joscha Jun 25, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@gcatanese gcatanese Jun 25, 2025

Choose a reason for hiding this comment

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

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 👍

);

String content = Files.readString(file);
assertEquals(1, TestUtils.countOccurrences(content, "@deprecated"));

}

@Test
public void testDeprecatedParameter() throws Exception {
final File output = Files.createTempDirectory("typescriptnodeclient_").toFile();
output.deleteOnExit();

final CodegenConfigurator configurator = new CodegenConfigurator()
.setGeneratorName("typescript")
.setInputSpec("src/test/resources/3_0/typescript/deprecated-parameter.yaml")
.setOutputDir(output.getAbsolutePath().replace("\\", "/"));

final ClientOptInput clientOptInput = configurator.toClientOptInput();
final DefaultGenerator generator = new DefaultGenerator();
final List<File> files = generator.opts(clientOptInput).generate();
files.forEach(File::deleteOnExit);

// verify parameter is deprecated parameter
Path file = Paths.get(output + "/apis/DefaultApi.ts");
TestUtils.assertFileContains(
file,
"* @param name name of pet (@deprecated)"
);

String content = Files.readString(file);
assertEquals(1, TestUtils.countOccurrences(content, "@deprecated"));

}
}
43 changes: 43 additions & 0 deletions modules/openapi-generator/src/test/resources/3_0/typescript/deprecated-operation.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
openapi: 3.0.0
info:
description: test order parameters
version: 1.0.0
title: Test order parameters
license:
name: Apache-2.0
url: 'https://www.apache.org/licenses/LICENSE-2.0.html'
paths:
/pets:
get:
tags:
- default
summary: Finds Pets
deprecated: true
description: Find all pets
operationId: findPets
parameters:
- name: type
in: query
description: type of pet
style: form
explode: false
schema:
type: string
default: available
- name: name
in: query
description: name of pet
required: true
schema:
type: string
- name: age
in: query
description: age of pet
schema:
type: number
format: int32
responses:
'200':
description: successful operation
'400':
description: Invalid status value
43 changes: 43 additions & 0 deletions modules/openapi-generator/src/test/resources/3_0/typescript/deprecated-parameter.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
openapi: 3.0.0
info:
description: test order parameters
version: 1.0.0
title: Test order parameters
license:
name: Apache-2.0
url: 'https://www.apache.org/licenses/LICENSE-2.0.html'
paths:
/pets:
get:
tags:
- default
summary: Finds Pets
description: Find all pets
operationId: findPets
parameters:
- name: type
in: query
description: type of pet
style: form
explode: false
schema:
type: string
default: available
- name: name
in: query
deprecated: true
description: name of pet
required: true
schema:
type: string
- name: age
in: query
description: age of pet
schema:
type: number
format: int32
responses:
'200':
description: successful operation
'400':
description: Invalid status value
Loading
Loading