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

lukeautry / tsoa

Build OpenAPI-compliant REST APIs using TypeScript and Node

MIT License

3.45k stars 494 forks source link

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

Closed LucianBuzzo closed 1 year ago

LucianBuzzo commented 1 year ago

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

[x] Have you followed the guidelines in our Contributing document?
[x] 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)?
[x] 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.

WoH commented 1 year ago

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

WoH commented 1 year ago

That said, I think https://github.com/lukeautry/tsoa/pull/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 commented 1 year ago

@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 commented 1 year ago

LGTM

amrbahaa360 commented 12 months ago

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

LucianBuzzo commented 11 months ago

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

WoH commented 11 months ago

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

LucianBuzzo commented 11 months ago

Awesome thanks @WoH !