Encode OpenAPI refs and remove concept of "ref-safe" names#463
Encode OpenAPI refs and remove concept of "ref-safe" names#463nguerrera merged 5 commits intomicrosoft:mainfrom
Conversation
* Correctly URI-encode OpenAPI refs when necessary. Delete broken code that attempted to address this with name mangling. * Remove all regexes from how we decide what to inline and what to ref, and how to name things. The reasoning is now done on the type objects, and not on their serialized names. * The current choices are preserved modulo obvious bugs, but I will open an issue to revisit some of them. It will be easier to change after this refactoring. * Centralize the logic in the shared openapi lib for reuse by cadl-autorest. * Fix issues with the regex-based stripping of Cadl and service namespaces by replacing that with a namespace filter callback on Checker.getTypeName. * Defend against parameter key and type name collisions with diagnostics.
nguerrera
requested review from
allenjzhang,
bterlson,
daviwil,
markcowl and
timotheeguerin
as code owners
|
You can try these changes at https://cadlplayground.z22.web.core.windows.net/prs/463/ |
|
cc @xirzec |
| getLiteralType(node: LiteralNode): LiteralType; | ||
| getTypeName(type: Type): string; | ||
| getNamespaceString(type: NamespaceType | undefined): string; | ||
| getTypeName(type: Type, options?: TypeNameOptions): string; |
There was a problem hiding this comment.
Not really in the scope of this PR but couldn't getTypeName be moved out of the checker. Doesn't feel like they depend on it.
There was a problem hiding this comment.
Good call, I can look at doing that as part of some other work I'm doing nearby next.
| getFriendlyName(program, type, options) ?? program.checker!.getTypeName(type, options); | ||
|
|
||
| if (existing && existing[name] !== undefined) { | ||
| reportDiagnostic(program, { |
There was a problem hiding this comment.
Should we report both instances conflicting. That's what we have for duplicate-symbol or duplicate routes.
There was a problem hiding this comment.
We should. It will take some more refactoring to do the bookkeeping. If you don't mind, I'd rather add this to the list to review on #464 because it will be easier to review more refactoring separately, I think.
There was a problem hiding this comment.
We weren't handling the collisions at all before so this is a strict improvement already.
daviwil
left a comment
daviwil
left a comment
There was a problem hiding this comment.
Overall, I like it! It is a little weird to see Rest.Resource.Something instead of Cadl.Rest.Resource.Something in some places, but it's not a huge issue.
It should be the case that you don't see Cadl.Rest in the OpenAPI anymore anywhere. It used to be rather haphazard when we'd remove Cadl. Now we remove it thoroughly and some things look a little weird as a result. I do have a note on #464 to revisit this. |
3 participants
Correctly URI-encode OpenAPI refs when necessary. Delete broken code that attempted to address this with name mangling.
Remove all regexes from how we decide what to inline and what to ref, and how to name things. The reasoning is now done on the type objects, and not on their serialized names.
The current choices are preserved modulo obvious bugs, but I will open an issue to revisit some of them. It will be easier to change after this refactoring. (Edit: filed Review inlining and naming choices in OpenAPI #464)
Centralize the logic in the shared openapi lib for reuse by cadl-autorest.
Fix issues with the regex-based stripping of Cadl and service namespaces by replacing that with a namespace filter callback on Checker.getTypeName.
Defend against parameter key and type name collisions with diagnostics.