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.

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

Escape backtick and template variable from type name #1469

Closed
wants to merge 2 commits into from

Conversation

MacFJA
Copy link

@MacFJA MacFJA commented Aug 20, 2023

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you written unit tests?
  • Have you written unit tests that cover the negative cases (i.e.: if bad data is submitted, does the library respond properly)?
  • This PR is associated with an existing issue?

This PR solve the issue with Swagger that can't handle $ schema name.

My use case is the following:

type MyModel = {
  normalProp: string;
  [key: `variable${number}`]: string;
}

TSOA can't use the type MyModal which is understandable as one of the key is dynamic.
My approach what to create a TSOA specific type like this:

import { MyModal as SourceMyModel } from "./types";
type MyModel = Omit<SourceMyModel, `variable${number}`> & { variable1: string, variable2: string };

With this, TSOA is able to generate the OpenApi file,
but the generated schemas are:

"Pick_SourceMyModel.Exclude_keyofSourceMyModel.%60variable%24_number_%60__": {
	"properties": {
		"normalProp": {
			"type": "string"
		}
	},
	"required": [
		"normalProp"
	],
	"type": "object",
	"description": "From T, pick a set of properties whose keys are in the union K"
},
"Omit_SourceMyModel.%60variable%24_number_%60_": {
	"$ref": "#/components/schemas/Pick_SourceMyModel.Exclude_keyofSourceMyModel.%60variable%24_number_%60__",
	"description": "Construct a type with the properties of T except for those in type K."
},
"MyModel": {
	"allOf": [
		{
			"$ref": "#/components/schemas/Omit_SourceMyModel.%60variable%24_number_%60_"
		},
		{
			"properties": {
				"variable2": {
					"type": "string"
				},
				"variable1": {
					"type": "string"
				}
			},
			"type": "object"
		}
	]
}

With my PR the output will be:

"Pick_SourceMyModel.Exclude_keyofSourceMyModel.variable_number__": {
	"properties": {
		"normalProp": {
			"type": "string"
		}
	},
	"required": [
		"normalProp"
	],
	"type": "object",
	"description": "From T, pick a set of properties whose keys are in the union K"
},
"Omit_SourceMyModel.variable_number_": {
	"$ref": "#/components/schemas/Pick_SourceMyModel.Exclude_keyofSourceMyModel.variable_number__",
	"description": "Construct a type with the properties of T except for those in type K."
},
"MyModel": {
	"allOf": [
		{
			"$ref": "#/components/schemas/Omit_SourceMyModel.variable_number_"
		},
		{
			"properties": {
				"variable2": {
					"type": "string"
				},
				"variable1": {
					"type": "string"
				}
			},
			"type": "object"
		}
	]
}

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hello there MacFJA 👋

Thank you and congrats 🎉 for opening your first PR on this project.✨

We will review the following PR soon! 👀

@WoH
Copy link
Collaborator

WoH commented Aug 22, 2023

Looks reasonable, can you please add the example you provided as a test?

@MacFJA
Copy link
Author

MacFJA commented Aug 27, 2023

@WoH Sorry for the delay.
I added the test, I hope I put it in the right place

@@ -770,9 +770,11 @@ export class TypeResolver {
.replace(/,/g, '.')
.replace(/'([^']*)'/g, '$1')
.replace(/"([^"]*)"/g, '$1')
.replace(/`([^`]*)`/g, '$1')
Copy link
Collaborator

Choose a reason for hiding this comment

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

This concern is valid for both this and the other escape, but I think we can construct scenarios where this could lead to a clash (2 different references ending up with the same name), right?

Copy link
Author

Choose a reason for hiding this comment

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

Yes it could, and whenever char are replaced or removed.

l TS Type Escaped value Data group
Omit<MyType, "_foo"> Omit_MyType._foo_ The type MyType minus the key _foo (1)
Omit<MyType, '_foo'> Omit_MyType._foo_ The type MyType minus the key _foo (1)
Omit<MyType, `_foo`> Omit_MyType._foo_ The type MyType minus the key _foo (1)
Omit<MyType, `${foo}`> Omit_MyType._foo_ The type MyType minus the key of type foo (2)
Omit<MyType, `${fo}o`> Omit_MyType._foo_ The type MyType minus the key of (type fo + text o) (3)
Omit<MyType, `${"foo"}`> Omit_MyType._foo_ The type MyType minus the key foo (4)
many more possibilities

For the first 3, they are the same data, so it's not a big deal.


There is maybe another case (I don't tested it yet): classes, interfaces, types, enums are not "unique".
In TSOA, refType don't mention their source file, so:

// fileA.ts
import { MyType } from "./somewhere/in/the/project"
// fileB.ts
import { MyType } from "./in/another/place"

will be declared as the same schema in the OpenAPI file (to be confirmed), and so create a clash

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it's possible that's a bug

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we find a way to rewrite that so it does not clash with other, but different type names?

@amrbahaa360
Copy link

Is the PR merged to version 5.1.1?
The issue still exists and has not been solved.

@WoH
Copy link
Collaborator

WoH commented Oct 8, 2023

Is the PR merged to version 5.1.1? The issue still exists and has not been solved.

Would you like to improve on this PR and address the points I raised during review? Remember to add tests.

Copy link

github-actions bot commented Nov 8, 2023

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days

@github-actions github-actions bot added the Stale label Nov 8, 2023
@WoH WoH removed the Stale label Nov 10, 2023
Copy link

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days

@github-actions github-actions bot added the Stale label Dec 11, 2023
@github-actions github-actions bot closed this Dec 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants