Skip to content
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

[Typescript-Node] Mark deprecated model attributes #19756

Merged
merged 6 commits into from
Oct 2, 2024
Merged

[Typescript-Node] Mark deprecated model attributes #19756

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
860bacd
Add @deprecated tag for deprecated fields
gcatanese Oct 2, 2024
3836068
Add tests
gcatanese Oct 2, 2024
bbc749e
Generate samples from OpenAPI 3.0 file
gcatanese Oct 2, 2024
0d7879c
Revert to generate with previous OpenAPI 2.0 file
gcatanese Oct 2, 2024
4aa0a44
Add new config 3.0 and generate samples
gcatanese Oct 2, 2024
1196446
Regenerate samples
gcatanese Oct 2, 2024
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
4 changes: 4 additions & 0 deletions bin/configs/typescript-node-3.0.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
generatorName: typescript-node
outputDir: samples/client/petstore/typescript-node/3_0
inputSpec: modules/openapi-generator/src/test/resources/3_0/typescript-node/SampleProject.yaml
templateDir: modules/openapi-generator/src/main/resources/typescript-node
11 changes: 11 additions & 0 deletions modules/openapi-generator/src/main/resources/typescript-node/model.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,18 @@ export class {{classname}} {{#parent}}extends {{{.}}} {{/parent}}{
{{#description}}
/**
* {{{.}}}
{{#deprecated}}
*
* @deprecated
{{/deprecated}}
*/
{{/description}}
{{^description}}
{{#deprecated}}
/**
* @deprecated
*/
{{/deprecated}}
{{/description}}
'{{name}}'{{^required}}?{{/required}}: {{#isEnum}}{{{datatypeWithEnum}}}{{/isEnum}}{{^isEnum}}{{{dataType}}}{{#isNullable}} | null{{/isNullable}}{{/isEnum}}{{#defaultValue}} = {{#isEnum}}{{classname}}.{{/isEnum}}{{{.}}}{{/defaultValue}};
{{/vars}}
Expand Down
69 changes: 69 additions & 0 deletions ...a/org/openapitools/codegen/typescript/typescriptnode/TypeScriptNodeClientCodegenTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,11 @@
import io.swagger.v3.oas.models.media.ObjectSchema;
import io.swagger.v3.oas.models.media.Schema;
import io.swagger.v3.oas.models.media.StringSchema;
import org.openapitools.codegen.ClientOptInput;
import org.openapitools.codegen.CodegenModel;
import org.openapitools.codegen.DefaultGenerator;
import org.openapitools.codegen.TestUtils;
import org.openapitools.codegen.config.CodegenConfigurator;
import org.openapitools.codegen.languages.TypeScriptNodeClientCodegen;
import org.openapitools.codegen.model.ModelMap;
import org.openapitools.codegen.model.ModelsMap;
Expand All @@ -16,6 +19,10 @@
import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;

import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.util.*;

@Test(groups = {TypeScriptGroups.TYPESCRIPT, TypeScriptGroups.TYPESCRIPT_NODE})
Expand Down Expand Up @@ -227,6 +234,68 @@ public void postProcessOperationsWithModelsTestWithModelNamePrefix() {
Assert.assertEquals(tsImports.get(0).get("filename"), "./prefixChild");
}

@Test
public void testApisGeneration() throws IOException {

File output = Files.createTempDirectory("typescriptnodeclient_").toFile();
output.deleteOnExit();

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

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

TestUtils.assertFileContains(Paths.get(output + "/api/basicApi.ts"),
"* Sample project");
TestUtils.assertFileContains(Paths.get(output + "/api/basicApi.ts"),
"export class BasicApi {");
}

@Test
public void testModelsGeneration() throws IOException {

File output = Files.createTempDirectory("typescriptnodeclient_").toFile();
output.deleteOnExit();

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

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

TestUtils.assertFileContains(Paths.get(output + "/model/group.ts"),
"export class Group {");
}
Comment on lines +238 to +277
Copy link
Contributor

Choose a reason for hiding this comment

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

I think if these were broken we'd notice it in other ways, too, so I don't think these tests cover anything new?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was no test that generated the client code and tested the creation of models, so I have added one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, what I meant was that there are a whole bunch of generated samples with files checked in. If code were broken that removed these models it would either show as a diff (removals) in a pull request or the build would fail.


@Test
public void testDeprecatedAttribute() throws IOException {

File output = Files.createTempDirectory("typescriptnodeclient_").toFile();
output.deleteOnExit();

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

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

TestUtils.assertFileContains(Paths.get(output + "/model/group.ts"),
"* @deprecated");
Copy link
Contributor

Choose a reason for hiding this comment

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

I see a few issues with this:

  • The mustache template content is now bound to this particular unit test
  • The match is quite imprecise, if I put the token @deprecated anywhere in any description of any variable of this model, the match here would be truthy

I think the sample output below should be sufficient?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed not the best approach, but this is similar in most generators where tests search for patterns. The alternative is maybe what it was done with the Go Server Generator, adding a workflow that tests the generated samples and checks the specific file.
What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

moving it into a workflow means that any local development will not pick up on it anymore, the feedback look would get much slower.
I am suggesting to remove these tests altogether and rely on the generated sample output and the assertion about them being up-to-date. See #19756 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rely on the generated sample output? without a test? I am not sure I understand.

I think we need a test, either in TypeScriptNodeClientCodegenTest or in a workflow verifying the samples (those tests can be run locally, not only by the workflow).

Copy link
Contributor

Choose a reason for hiding this comment

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

@macjohnny can correct me if I am wrong, but our current setup is using the generated samples as a form of snapshot test.
A diff in them will either show up in the pull request or fail on CI.

Think of it like this:

  • You introduce the new logic to add a deprecated marker to generated code
  • In the future someone else makes a PR that accidentally changes that logic, so the markers get lost
  • Their PR would fail CI
  • They would either fix the logic or regenerate the samples
  • If they regenerate the samples it would show as a diff
  • A reviewer would decide whether the removal of the marker is intended, or not
  • If not, they'll prompt the code to be updated to not remove the markers, possibly with a reference to this pull request

Copy link
Member

Choose a reason for hiding this comment

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

@joscha @gcatanese often automated tests are implemented as integration tests that run the generated code and make certain assertions, like

openapi-generator/samples/client/petstore/typescript-angular-v13-provided-in-root/tests/default/src/test/api.spec.ts

Line 70 in 90bc100

it(`should add a pet`, waitForAsync(() => {

in some cases unit tests like https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/test/java/org/openapitools/codegen/typescript/fetch/TypeScriptFetchClientCodegenTest.java can make very specific assertions that are either not covered by the generated samples (one needs to balance number of different samples vs covering each variation in configuration) or are hard to catch and thus merit a specific assertion in a unit test.

Copy link
Member

Choose a reason for hiding this comment

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

so usually adding a unit test is a good thing

Copy link
Contributor

Choose a reason for hiding this comment

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

TIL. Would it make sense to fail on any diff that is produced by generating from anything under config/ ?

Copy link
Member

Choose a reason for hiding this comment

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

You're saying the diff produced in the generated samples would not fail the CI checks if you didn't update them?

no, the CI will fail if the samples are outdated, i.e. the changed code produces a diff in samples and the PR does not add this diff means the CI fails. so when the diff is added to the PR, the reviewer needs to manually check whether this is fine. this does not mean it would actually compile, or sometimes a reviewer might just overlook a specific aspect (especially if the diffs are large), which is why additional assertions in unit tests can make sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. I personally think the generated fixtures are sufficient. Unit tests string.contains() assertions seem quite fragile in comparison.

}

private OperationsMap createPostProcessOperationsMapWithImportName(String importName) {
OperationMap operations = new OperationMap();
operations.setClassname("Pet");
Expand Down
Loading
Loading