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: @withVisibility decorator #2544

Merged
merged 7 commits into from
Oct 13, 2023

Conversation

timotheeguerin
Copy link
Member

fix #2517

@github-actions
Copy link
Contributor

github-actions bot commented Oct 6, 2023

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/2544/

Website: https://tspwebsitepr.z22.web.core.windows.net/prs/2544/

Copy link
Member Author

@timotheeguerin timotheeguerin Oct 6, 2023

Choose a reason for hiding this comment

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

Todo need to update the doc to explain this new behavior
Todo add tests

@mikekistler
Copy link
Member

I think there is still a problem. I believe that any model I define in TypeSpec should appear in the OpenAPI with all the properties I defined in them (in the absence of a "withVisibility" decorator).

But this isn't the case, as this playground illustrates. The Dog model is missing the secretName property. Isn't that wrong?

@timotheeguerin
Copy link
Member Author

timotheeguerin commented Oct 6, 2023

yeah I guess, my fix might actually only really fix the other issue Brian thought about. So this remaining fix I guess is about the openapi3 emitter not using visibility for unreachable models.

So the 2 issues are:

  • Unreachable models shouldn't be affected by visibility
  • Model used with @withVisibility and referenced in an operation shouldn't have their visibility reapplied

@timotheeguerin timotheeguerin enabled auto-merge (squash) October 13, 2023 16:32
@timotheeguerin timotheeguerin merged commit f110040 into microsoft:main Oct 13, 2023
12 checks passed
@timotheeguerin timotheeguerin deleted the fix/with-visibility branch October 13, 2023 16:43
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 applies read visibility to unreachable types
3 participants