[typescript] fix: escaped multiline comments; implementation option 1 #19528

joscha · 2024-09-04T15:11:43Z

Currently, generated Typescript-code has escaped 's, etc. in the generated comment parts. This is due to the fact, that we currently use escapeText

openapi-generator/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java

Line 1100 in e914c40

public String escapeText(String input) {

everywhere, which has varying implementations for each target language. For Typescript it has:

openapi-generator/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractTypeScriptClientCodegen.java

Lines 1073 to 1074 in e914c40

    
           // replace ' with \' 
        
           return super.escapeText(input).replace("\'", "\\\'");

which results in the aforementioned escaped sequence.

Example:

(origin here)

This pull request only escapes unsafe characters (currently multiline comments) for the descriptions of parameters, etc.

Note: The current implementation of escapeUnsafeCharacters does not guard for null, which is a possibility for any string passed. escapeText has the null guard. There are about a dozen @Overrides for escapeUnsafeCharacters, none of them guarding null (as it hasn't been used directly before, but only through escapeText, which has the guard). There were two options:

Add null guards to all existing implementations

Introduce a wrapper function like escapeText that guards for null and calls to escapeUnsafeCharacters

I chose 2) as it seemed more resilient, testable and also makes sure that other implementations out there and new ones in pull requests keep on working as expected. Let me know if you prefer 1) instead.

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

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: @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)

closes #19553, #19554

joscha · 2024-09-04T15:13:31Z

modules/openapi-generator/src/main/resources/typescript/api/api.mustache

@@ -39,7 +39,7 @@ export class {{classname}}RequestFactory extends BaseAPIRequestFactory {
     * {{&summary}}
     {{/summary}}
     {{#allParams}}
-     * @param {{paramName}} {{description}}
+     * @param {{paramName}} {{{description}}}


I noticed here, that we HTML-escape the description. We don't anywhere else, so I fixed this on the way, but it is only tangentially related to the pull request. Please let me know if you'd like to revert this. There are currently no sample fixtures which are covering this codepath.

joscha · 2024-09-04T15:13:50Z

modules/openapi-generator/src/main/resources/typescript/model/model.mustache

@@ -6,23 +6,23 @@ import { {{classname}} } from '{{filename}}{{importFileExtension}}';
 {{/tsImports}}


This file contains one half of the meaningful changes of this PR.

joscha · 2024-09-04T15:14:27Z

...generator/src/test/java/org/openapitools/codegen/typescript/TypeScriptClientCodegenTest.java

+    public void multilineCommentsAreEscaped() {
+        final DefaultCodegen codegen = new TypeScriptClientCodegen();
+        final String multilineComment = "/* This is a multiline comment */";
+        final String escapedComment = codegen.escapeUnsafeCharacters(multilineComment);


noticed escapeUnsafeCharacters was untested.

joscha · 2024-09-04T15:14:46Z

modules/openapi-generator/src/test/java/org/openapitools/codegen/DefaultCodegenTest.java

+    public void testEscapeUnsafeCharactersNullGuard() {
+        final DefaultCodegen codegen = new DefaultCodegen();
+
+        // allow null


this asserts that the type guard exists in the newly introduced function.

joscha · 2024-09-04T15:15:28Z

modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java

@@ -1167,6 +1167,19 @@ public String escapeUnsafeCharacters(String input) {
        return input;


This file contains one half of the meaningful changes of this PR.

macjohnny

thanks for your contribution!

joscha · 2024-09-04T15:22:34Z

modules/openapi-generator/src/main/java/org/openapitools/codegen/CodegenConfig.java

@@ -100,6 +100,8 @@ public interface CodegenConfig {

    String escapeUnsafeCharacters(String input);

+    String escapeUnsafeCharactersNullGuard(String input);


I chose this name as that is what it currently does. Most implementations of escapeUnsafeCharacters are not actually about unsafe characters but about multiline comments, so we'd have the chance to name this escapeMultilineComment if we wanted, which would be more obvious, but has the caveat that the function it calls out to (escapeUnsafeCharacters) might not be aligned, so I left it with this slightly awkward suffix.

joscha · 2024-09-04T15:24:37Z

samples/openapi3/client/petstore/typescript/builds/explode-query/models/Capitalization.ts

@@ -19,7 +19,8 @@ export class Capitalization {
    'capitalSnake'?: string;
    'sCAETHFlowPoints'?: string;
    /**
-    * Name of the pet 
+    * Name of the pet
+


I didn't add trimming to the escapeUnsafeCharacters implementation. escapeText has it, hence why this now has an additional newline. Let me know if you prefer trimming leading and trailing whitespace for this and if yes, whether you prefer it for the Typescript generator in specific or generally for all implementations.

wing328 · 2024-09-09T02:59:14Z

Add null guards to all existing implementations

if it's not a lot of work, can you please file another PR to add null guards for the typescript generator(s)?

to me this approach seems more straightforward as I don't think we need 2 functions doing the same thing: one with null check and another one without null check.

joscha · 2024-09-09T07:27:28Z

Add null guards to all existing implementations

if it's not a lot of work, can you please file another PR to add null guards for the typescript generator(s)?

It's not too much. Do you want me to guard for null in any other generator as well, if need be, or wait until the issue occurs and we fix it then?

to me this approach seems more straightforward as I don't think we need 2 functions doing the same thing: one with null check and another one without null check.

Agree.

joscha · 2024-09-09T10:05:16Z

if it's not a lot of work, can you please file another PR to add null guards for the typescript generator(s)?

I opened three alternatives:

[typescript] fix: escaped multiline comments; implementation option 1 #19528 (this PR), introduces a escapeUnsafeCharactersNullGuard
[typescript] fix: escaped multiline comments; implementation option 2 #19553 moves the check into each individual generator
[typescript] fix: escaped multiline comments; implementation option 3 #19554 pulls the null guard up into the parent codegen

Let me know what you think @wing328.

I think after seeing the three options side by side, my order of preference would be 3, 1, 2

wing328 · 2024-09-09T10:10:48Z

thanks for creating these PRs. i'll take a look later this week

wing328 · 2024-09-13T09:22:12Z

@joscha I might have missed it. Why not simply use {{{description}}} which is escaped to prevent code injection to address this particular problem?

joscha · 2024-09-13T09:40:56Z

@joscha I might have missed it. Why not simply use {{{description}}} which is escaped to prevent code injection to address this particular problem?

description has

openapi-generator/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractTypeScriptClientCodegen.java

Line 1074 in e914c40

return super.escapeText(input).replace("\'", "\\\'");

applied, which is doing the ' to \' transformation.

wing328 · 2024-09-13T09:43:34Z

applied, which is doing the ' to ' transformation.

If it doesn't replace ' with \', it will meet your requirement, right?

joscha · 2024-09-13T09:45:49Z

If it doesn't replace ' with \', it will meet your requirement, right?

yes, hence why in each of the fix PRs escapeUnsafeCharacters() is applied but not escapeText().

joscha · 2024-09-17T10:50:15Z

@wing328 @macjohnny any more thoughts on this? I think after seeing the three options side by side, my order of preference would be 3, 1, 2 (see #19528 (comment))

macjohnny · 2024-09-17T11:44:14Z

@joscha I am fine with your suggestion. @wing328 can you approve?

joscha added 2 commits September 4, 2024 15:58

[typescript] fix: Java-escaped multiline comments

4dfb2ce

chore: update generated code samples

92f3c1c

joscha changed the title ~~[typescript] fix: Java-escaped multiline comments~~ [typescript] fix: escaped multiline comments Sep 4, 2024

joscha commented Sep 4, 2024

View reviewed changes

macjohnny approved these changes Sep 4, 2024

View reviewed changes

macjohnny requested a review from wing328 September 4, 2024 15:16

joscha commented Sep 4, 2024

View reviewed changes

joscha mentioned this pull request Sep 4, 2024

docs(V2): no unnecessary escaped sequences planet-a-ventures/affinity-node#40

Open

1 task

This was referenced Sep 9, 2024

[typescript] fix: escaped multiline comments; implementation option 2 #19553

Open

[typescript] fix: escaped multiline comments; implementation option 3 #19554

Open

joscha changed the title ~~[typescript] fix: escaped multiline comments~~ [typescript] fix: escaped multiline comments; implementation option 1 Sep 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[typescript] fix: escaped multiline comments; implementation option 1 #19528

[typescript] fix: escaped multiline comments; implementation option 1 #19528

joscha commented Sep 4, 2024 •

edited

Loading

joscha Sep 4, 2024

joscha Sep 4, 2024

joscha Sep 4, 2024

joscha Sep 4, 2024

joscha Sep 4, 2024

macjohnny left a comment

joscha Sep 4, 2024

joscha Sep 4, 2024

wing328 commented Sep 9, 2024

joscha commented Sep 9, 2024

joscha commented Sep 9, 2024 •

edited

Loading

wing328 commented Sep 9, 2024

wing328 commented Sep 13, 2024

joscha commented Sep 13, 2024

wing328 commented Sep 13, 2024

joscha commented Sep 13, 2024 •

edited

Loading

joscha commented Sep 17, 2024

macjohnny commented Sep 17, 2024

	// replace ' with \'
	return super.escapeText(input).replace("\'", "\\\'");

		@@ -6,23 +6,23 @@ import { {{classname}} } from '{{filename}}{{importFileExtension}}';
		{{/tsImports}}

		@@ -1167,6 +1167,19 @@ public String escapeUnsafeCharacters(String input) {
		return input;

		@@ -100,6 +100,8 @@ public interface CodegenConfig {

		String escapeUnsafeCharacters(String input);

		String escapeUnsafeCharactersNullGuard(String input);

[typescript] fix: escaped multiline comments; implementation option 1 #19528

Are you sure you want to change the base?

[typescript] fix: escaped multiline comments; implementation option 1 #19528

Conversation

joscha commented Sep 4, 2024 • edited Loading

PR checklist

joscha Sep 4, 2024

Choose a reason for hiding this comment

joscha Sep 4, 2024

Choose a reason for hiding this comment

joscha Sep 4, 2024

Choose a reason for hiding this comment

joscha Sep 4, 2024

Choose a reason for hiding this comment

joscha Sep 4, 2024

Choose a reason for hiding this comment

macjohnny left a comment

Choose a reason for hiding this comment

joscha Sep 4, 2024

Choose a reason for hiding this comment

joscha Sep 4, 2024

Choose a reason for hiding this comment

wing328 commented Sep 9, 2024

joscha commented Sep 9, 2024

joscha commented Sep 9, 2024 • edited Loading

wing328 commented Sep 9, 2024

wing328 commented Sep 13, 2024

joscha commented Sep 13, 2024

wing328 commented Sep 13, 2024

joscha commented Sep 13, 2024 • edited Loading

joscha commented Sep 17, 2024

macjohnny commented Sep 17, 2024

joscha commented Sep 4, 2024 •

edited

Loading

joscha commented Sep 9, 2024 •

edited

Loading

joscha commented Sep 13, 2024 •

edited

Loading