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

[http/openapi] Use enum-driven visibility analysis APIs #5416

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

witemple-msft
Copy link
Member

@witemple-msft witemple-msft commented Dec 19, 2024

This PR brings the core enum-driven visibility system to parity with the existing visibility concepts and uses its new analysis methods in the HTTP and OpenAPI libraries.

  • Added lifecycle visibility modifiers for Delete and Query phases. Delete applies when a resource is passed as an argument to a DELETE operation. Query applies when a resource is passed as an argument to a GET or HEAD operation.
  • Added lifecycle transform templates for Delete and Query.
  • Made the logic of applying a visibility filter a little bit more robust. It will consider the any constraint of the filter to be satisfied if it is missing OR if some modifier in the constraint is present. If the any constraint is present, but empty, it will be unsatisfiable.
  • Deprecated getParameterVisibility and getReturnTypeVisibility in favor of getParameterVisibilityFilter and getReturnTypeVisibilityFilter which return VisibilityFilter objects instead of string[]. Like the modifier analysis methods, the filter versions of these methods are infallible.
  • getParameterVisibilityFilter and getReturnTypeVisibilityFilter accept a VisibilityProvider object that provides default visibility filters for parameters and return types if the parameter/returnType visibilities are not set explicitly. TypeSpec core exports an EmptyProvider that always return filters with no constraints, suitable as a default. HTTP exports HttpVisibilityProvider, which must be used when working in HTTP, as it provides the HTTP-specific default visibility logic that determines visibility by verb.
  • HttpVisibilityProvider can be instantiated with either an HttpVerb directly, in which case that exact verb will be used; HttpOperationOptions, in which case the verbSelector will be used if present, otherwise the fallback logic will be used; or no argument in which case the fallback logic will always be used. This gives it parity with getHttpOperation.
  • HTTP now marshals conversions between core visibility and HTTP Visibility through visibility filters instead of visibility string arrays. The HTTP Visibility concept is unchanged.
  • Used new forms of visibility analysis APIs in openapi to detect read-only properties.
  • Fixed a bug in http-server-csharp that was surfaced by these changes where a visibility constraint Visibility.Create & Visibility.Update was provided, resulting in Visibility.None and properties being made invisible, affecting effective type calculation for anonymous models. (invalidated by merge)
  • Fixed a bug in which hasVisibility and getVisibilityForClass were improperly initializing the visibility state. This didn't affect the new APIs, but it would cause the state of the legacy APIs to be corrupted if visibility is queried from the new APIs before the legacy APIs.

Closes #5119
Closes #5120

@azure-sdk
Copy link
Collaborator

azure-sdk commented Dec 19, 2024

All changed packages have been documented.

  • @typespec/compiler
  • @typespec/http
  • @typespec/openapi
Show changes

@typespec/http - internal ✏️

Updated the OpenAPI3 and HTTP libraries to use the new visibility analysis APIs.

@typespec/openapi - internal ✏️

Updated the OpenAPI3 and HTTP libraries to use the new visibility analysis APIs.

@typespec/compiler - feature ✏️

Added APIs for getting parameterVisibility and returnTypeVisibility as VisibilityFilter objects.

@azure-sdk
Copy link
Collaborator

azure-sdk commented Dec 19, 2024

You can try these changes here

🛝 Playground 🌐 Website 📚 Next docs 🛝 VSCode Extension

@witemple-msft witemple-msft added deprecation A previously supported feature will now report a warning and eventually be removed lib:http labels Jan 7, 2025
@markcowl markcowl self-assigned this Jan 9, 2025
Copy link
Contributor

@markcowl markcowl left a comment

Choose a reason for hiding this comment

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

Also, one question: should we change the @parameterVisibility and @returnTypeVisibility decorators to accept enum visibility (or have I missed this being done previously?). Should we be clear that these are any filters?

Also, does @parameterVisibility() now effectively create an undefined any filter, meaning that all properties are allowed?

@@ -180,6 +181,9 @@ export const $parameterVisibility: ParameterVisibilityDecorator = (
/**
* Returns the visibilities of the parameters of the given operation, if provided with `@parameterVisibility`.
*
* @deprecated Use `getParameterVisibilityFilter` instead.
*
* @see {@link getParameterVisibilityFilter}
Copy link
Contributor

Choose a reason for hiding this comment

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

The two deprecations on the operation visibility decorators will have to be approved by the breaking change review board. I will be surprised if anyone is using these filters, other than possibly the typespec-autorest emitter.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are no occurrences of getReturnTypeVisibility or getParameterVisibility within the packages folder of typespec-azure. I doubt anything other than typespec itself was using these APIs.

@@ -218,6 +286,9 @@ export const $returnTypeVisibility: ReturnTypeVisibilityDecorator = (
/**
* Returns the visibilities of the return type of the given operation, if provided with `@returnTypeVisibility`.
*
* @deprecated Use `getReturnTypeVisibilityFilter` instead.
*
* @see {@link getReturnTypeVisibilityFilter}
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as above

},
};
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want the default here to be to return a filter, so that this should be deprecated as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this question about HttpVisibilityProvider or resolveRequestVisibility? If former, it will always return a filter. If latter, it would be severely breaking. I did consider it, but don't think we can really swallow deprecating the whole HTTP Visibility concept right now. I think instead of preemptively deprecating the HTTP-only Visibility, my hope is that if the core APIs are used in the next couple of months (i.e., people switch to using getReturnTypeVisibilityFilter(..., HttpVisibilityProvider) instead of resolveRequestVisibility or MetadataInfo, then HTTP's Visibility will become a vestigial feature that we can deprecate once some emitters have really proved that this analysis API works for their use case (I'm going to do this for the JS server emitter).

@witemple-msft
Copy link
Member Author

@markcowl @returnTypeVisibility and @parameterVisibility were changed to accept EnumValue in the first patch. The core system does the same kind of normalization for them that it does for visibility modifiers on properties.

I can clarify that returnType/parameter visibility are specifying any constraints. The documentation for those decorators isn't clear about that right now. I considered adding an @returnTypeVisibilityFilter and @parameterVisibilityFilter decorator, but HTTP's visibility concept isn't expressive enough for that. Strictly speaking, the VisibilityProvider can return a visibility filter of any form, but HTTP only understands the any constraint (and only yields filters with the any constraint from its provider). If we decide to make HTTP work with other forms of visibility filters, or add the ability to put a generic filter on returnType/parameters in an operation, we will be compelled to rip Visibility out of the HTTP core or change it very significantly.

Good question on @parameterVisibility() with no arguments. It will effectively create a VisibilityFilter that has { any: new Set() }, which will actually allow nothing rather than everything (VisibilityFilter#any has the same semantics as Array.some, false for an empty array). I'll have to check what we did before, but I can easily make it so that if the list of modifiers was set to empty in the decorator it will return an empty filter rather than a filter with an empty any constraint. An empty filter with no constraints passes all properties, where a filter with an empty any constraint rejects all properties.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation A previously supported feature will now report a warning and eventually be removed lib:http
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[openapi] Adopt enum-driven visibility [http] Adopt enum-driven visibility
4 participants