[openapi3] Fix bugs related to getRefSafeName in OpenAPI emitter
#456
Conversation
…nitized. This change addresses two separate issues with `getRefSafeName` in the OpenAPI emitter: First, the result of calling `getRefSafeName` was never used inside `getParameterKey` as it would always be overriden before being read. Second, `getRefSafeName` returned incorrect results, replacing the first *valid* character instead of all *invalid* characters due to a typo in the RegEx used.
xirzec
requested review from
allenjzhang,
bterlson,
daviwil,
markcowl,
nguerrera and
timotheeguerin
as code owners
|
Yikes, the regex typo was introduced a long time ago by me. 😳At the point where I broke the regex, it seems I also made other changes that meant we weren't needing it in common cases anymore. Originally, before the bug here, when we mangled the name, we also checked for collisions (getRefSafeName is not 1:1) and appended a counter. I see the collision handling code seems to be gone now, and I'm worried it was removed because as you're observing we weren't using it so we didn't observe the need for it. Could you try the following test case: https://cadlplayground.z22.web.core.windows.net/?c=aW1wb3J0ICJAY2FkbC1sYW5nL3Jlc3QiOwoKQHNlcnZpY2VUaXRsZSgiV2lkZ2V0IFPGFSIpCm5hbWVzcGFjZSBSZXBybzsKCnVzaW5nIENhZGwuSHR0cDsKCm1vZGVsIFBhcmFtcyB7CiAgQHF1ZXJ5ICJCYWQkTmFtZSI6IHN0cmluZzvOHSHPHX0KCm9wIHRlc3QoLi4uxlEpOiB7fTs%3D |
|
/azp run Try it |
|
I think the better fix here might be to url encode the refs and stop mangling to make parameters ref-able. The original isRefSafe/getRefSafe thing was from my own misunderstanding that the names couldn't have these characters. We have an issue tracking that. Though, it isn't a straight fix because we also are relying on isRefSafeName to decide inlining of models in cases where we don't have a "good" name and likely will want to preserve that to some extent. |
|
No pipelines are associated with this pull request. |
|
/azp run Cadl Pull Request Try It |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@timotheeguerin did it not run automatically because @xirzec is not considered a "contributor"? Just curious. |
|
Yeah for security reason as there is secret involved, when a pr is from a fork(when from the main repo it does it auto), it only build automatically for contributors otherwise a contributor need to make that comment |
|
https://github.com/Azure/cadl-azure/issues/510 is the related issue. I'm making other changes for metadata around how we choose the best name, and I'm inclined to tackle this first now. |
|
Seems it saw other comments and didn't post https://cadlplayground.z22.web.core.windows.net/prs/456/ |
|
Yes, the collision is happening: @xirzec I propose holding this back and I'll work on https://github.com/Azure/cadl-azure/issues/510 starting now. It should fix this and unblock something else I'm working on. I should be able to have something up today. Thank you for finding this! |
nguerrera
left a comment
nguerrera
left a comment
There was a problem hiding this comment.
See above, I think we should fix https://github.com/Azure/cadl-azure/issues/510 to fix this.
Sounds good to me! I'm going to close this PR in favor of your solution to #510 |
|
#463 and https://github.com/Azure/cadl-azure/pull/1403 are out with the fix. |
3 participants
This change addresses two separate issues with
getRefSafeNamein the OpenAPI emitter:First, the result of calling
getRefSafeNamewas never used insidegetParameterKeyas it would always be overridden before being read.Second,
getRefSafeNamereturned incorrect results, replacing the first valid character instead of all invalid characters due to a typo in the RegEx used.