Skip to content

Update OpenAPI3 emitter for shared routes#1770

Merged
tjprescott merged 19 commits into
mainfrom
emitters/SharedRoutesOpenAPI3
Apr 10, 2023
Merged

Update OpenAPI3 emitter for shared routes#1770
tjprescott merged 19 commits into
mainfrom
emitters/SharedRoutesOpenAPI3

Conversation

@tjprescott

@tjprescott tjprescott commented Mar 30, 2023

Copy link
Copy Markdown
Member

Closes #1198.

  • Emit diagnostic if not all routes that are shared are actually marked as such
  • Emit diagnostic if common params are inherently incompatible
  • Troubleshoot test scenarios for metadata:
    • can make properties optional
    • bubbles up visibility changes to referencers
  • Refactor to not alter the program or type graph
  • Operation ID heuristic for merged operations
  • Additional scenarios?
  • Finish implementation for mulitple request body types.
  • Finish implementation for multiple response types

@github-actions

Copy link
Copy Markdown
Contributor

Changes in this PR will be published to the following url to try(check status of TypeSpec Pull Request Try It pipeline for publish status):
Playground: https://cadlplayground.z22.web.core.windows.net/prs/1770/

Website: https://cadlwebsite.z1.web.core.windows.net/prs/1770/

@azure-pipelines

Copy link
Copy Markdown

You can try these changes at https://cadlplayground.z22.web.core.windows.net/prs/1770/

Check the website changes at https://cadlwebsite.z1.web.core.windows.net/prs/1770/

@tjprescott
tjprescott force-pushed the emitters/SharedRoutesOpenAPI3 branch 4 times, most recently from 3e370a9 to acc6d94 Compare March 31, 2023 20:29
Comment thread packages/openapi3/test/shared-routes.test.ts
Comment thread packages/openapi3/test/shared-routes.test.ts Outdated
@tjprescott
tjprescott force-pushed the emitters/SharedRoutesOpenAPI3 branch 2 times, most recently from 1387dd5 to 6564487 Compare April 3, 2023 20:50
@tjprescott

Copy link
Copy Markdown
Member Author

@johanste I updated the PR to allow params to differ by location. The key logic that is reflected in the tests occurs in openapi.ts lines 422:465 (validateCommonParameters). Please scrutinize that heavily and let me know if there are other scenarios I should handle or if I'm handling existing ones incorrectly.

@mikekistler

Copy link
Copy Markdown
Contributor

Is there documentation for how this works?

@markcowl markcowl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks good, a couple of suggestions for additional tests

Comment thread packages/openapi3/test/shared-routes.test.ts
Comment thread packages/http/src/route.ts Outdated

@nguerrera nguerrera left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Minor comment about sorting. I looked only at the test cases, not the implementation but that was all I found about the test cases.

Comment thread packages/openapi3/test/shared-routes.test.ts Outdated
@tjprescott
tjprescott force-pushed the emitters/SharedRoutesOpenAPI3 branch 6 times, most recently from f314450 to 9ca0c35 Compare April 4, 2023 22:33
Comment thread packages/openapi3/test/shared-routes.test.ts
@mikekistler

Copy link
Copy Markdown
Contributor

It looks like some operations may be silently discarded if routes are shared.

Playground Example where operation "bar" is silently discarded.

@tjprescott

Copy link
Copy Markdown
Member Author

New scenario where signatures are not the same, and bar is still lost.

Playground Example 2

bar is not lost here. Foo and bar are combined in the output spec as foo_bar.

@tjprescott
tjprescott force-pushed the emitters/SharedRoutesOpenAPI3 branch from 9a69838 to 13e0de2 Compare April 5, 2023 23:44
@tjprescott
tjprescott force-pushed the emitters/SharedRoutesOpenAPI3 branch 2 times, most recently from 6e3ecfb to 44f6bd2 Compare April 10, 2023 19:52
@tjprescott
tjprescott force-pushed the emitters/SharedRoutesOpenAPI3 branch from 44f6bd2 to 0f5ef7d Compare April 10, 2023 19:53
* @param path Relative route path. Cannot include query parameters.
* @param parameters Optional set of parameters used to configure the route. Supports `{shared: true}` which indicates that the route may be shared by several operations.
*/
extern dec route(entity: unknown, path: string, parameters?: object);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same comment as on @daviwil PR, parameters should be typed

alias RouteOptions = {
shared?: boolean
}

@tjprescott
tjprescott enabled auto-merge (squash) April 10, 2023 21:10
@tjprescott
tjprescott merged commit 7195605 into main Apr 10, 2023
@tjprescott
tjprescott deleted the emitters/SharedRoutesOpenAPI3 branch April 10, 2023 21:19
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.

OpenAPI and Autorest emitters need updating for shared routes

6 participants