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

fix: Don't URI encode schema keys when using $ref #1468

Merged

Conversation

LucianBuzzo
Copy link
Contributor

@LucianBuzzo LucianBuzzo commented Aug 17, 2023

This fixes an isssue where using $ in type names leads to invalid OpenAPI specs. This would happen because the refName property that is used as the object key for refs and to create $ref attributes is URI encoded on creation, when it only needs to be URI encoded when used in a $ref attribute.

Fixes #1461

  • 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?

Test plan

I had a hard time configuring a unit test to check this behaviour, if anyone can provide some guidance on how to do it please let me know, as the test cases in this repo are hard to follow.

This fixes an isssue where using `$` in type names leads to invalid
OpenAPI specs. This would happen because the `refName` property that is
used as the object key for refs and to create `$ref` attributes is URI
encoded on creation, when it only needs to be URI encoded when used in
a `$ref` attribute.

Fixes lukeautry#1461

Signed-off-by: Lucian Buzzo <lucian.buzzo@gmail.com>
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 LucianBuzzo 👋

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 27, 2023

Does this only affect the OpenAPI v2 generator? I think we should be consistent and also adapt the OpenAPI v3 Spec Generator.

@WoH
Copy link
Collaborator

WoH commented Aug 27, 2023

That said, I think #1469 would also address this.

I'm not sure if you can URI Encode the ref value, but use the unencoded name as the components/definition key, is that specified somewhere?

@LucianBuzzo
Copy link
Contributor Author

@WoH I updated my PR to also fix the problem for the v3 generator. AFAICT, what happens is that when the $ref value is resolved, it decodes the URI, resulting in %24 being decoded to $ - however, because we use the encoded value as a key, it can't find it. The resolution lookup would be for $variable, but what's in the schema is %24variable.
By only URI encoding the $ref value, we avoid this particular issue, without resorting to special case escaping of the keys.
URI encoding only the $ref URI value is the canonically correct approach as far as I can see!

@WoH
Copy link
Collaborator

WoH commented Sep 4, 2023

LGTM

@WoH WoH merged commit d7cf119 into lukeautry:master Sep 4, 2023
27 checks passed
@amrbahaa360
Copy link

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

@LucianBuzzo
Copy link
Contributor Author

@amrbahaa360 It looks like v5.1.1 was released in February, which won't include this change. A new version needs to be released. @WoH would you be able to cut a new version that includes this fix? v5.1.2 perhaps?

@LucianBuzzo LucianBuzzo deleted the lucianbuzzo/1461-urlencode-in-type-names branch September 28, 2023 12:51
@WoH
Copy link
Collaborator

WoH commented Sep 28, 2023

https://github.com/lukeautry/tsoa/releases/tag/v6.0.0-rc.4

@LucianBuzzo
Copy link
Contributor Author

Awesome thanks @WoH !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Usage of $ in type names leads to invalid OpenAPI specs
3 participants