microsoftgraph / msgraph-metadata

Microsoft Graph metadata captured and used for generating client library code files.
https://graph.microsoft.com
MIT License
105 stars 31 forks source link

`teamsApps` and `appDefinitions` should be marked as `HasStream="true"` #163

Open peombwa opened 2 years ago

peombwa commented 2 years ago

teamsApps and appDefinitions should be marked as HasStream="true" with a MediaType annotation of application/zip. See publish-an-app-to-the-app-catalog and update-an-application-previously-published-to-the-microsoft-teams-app-catalog.

The way the API is currently represented in the metadata results in missing SDK snippets in the API reference docs.

irvinesunday commented 1 year ago

@peombwa do you mean adding the attribute HasStream="true" to the entity types defining the mentioned navigation properties, teamsApps and appDefinitions, i.e. teamsApp and teamsAppDefinition respectively? And add the Org.OData.Core.V1.AcceptableMediaTypes to those respective entity types?

Reference image

peombwa commented 1 year ago

@irvinesunday, that was the original thought - https://github.com/microsoftgraph/microsoft-graph-devx-api/issues/336#issuecomment-708629024. However, I'm not entirely sure if HasStream="true" is the appropriate attribute to use since the API is now described as shown below:

POST https://graph.microsoft.com/v1.0/appCatalogs/teamsApps
Content-type: application/zip

[Zip file containing a Teams app package]

The API also returns a JSON object on upload - https://docs.microsoft.com/graph/api/teamsapp-publish?view=graph-rest-1.0&tabs=http#example-1-publish-an-app-to-the-app-catalog.

irvinesunday commented 1 year ago

Yeah, this is a tricky one. I am thinking (and this might be unconventional) we can annotate the nav. property with a MediaType annotation as below:

<EntityType Name="appCatalogs" BaseType="graph.entity">
  <NavigationProperty Name="teamsApps" Type="Collection(graph.teamsApp)" ContainsTarget="true">
    <Annotation Term="Org.OData.Core.V1.MediaType" String="application/zip" />
  </NavigationProperty>
</EntityType>

Then we can update the conversion lib to override the default response and request body content types with the string value provided if that annotation is present.

Thoughts @baywet, @darrelmiller

baywet commented 1 year ago

This API is a bit of a mess. Not only the creation is based off a zip, which gets posted to a collection, but the update is also a POST (instead of PUT/PATCH) that gets posted to https://graph.microsoft.com/v1.0/appCatalogs/teamsApps/{id}/appDefinitions. To me it doesn't follow the OData semantics, hence our difficulty to model it in the service definition and for the conversion library to handle it properly. Maybe they should have relied on actions instead?

It's not the only case like that, places have a weird implementation, messages (email) attachments as well, etc... I'd argue that for those edge cases, we'd need a "OpenAPI description transform step". Like we have the XSLT today for the CSDL, this file would contain jq queries (or equivalent) and an object description for insert/update/removal. This file would only be used for things that are impossible to model with CSDL semantics. What do you think?

In case we still want to try to model it using CSDL, I think this annotation is ambiguous. As far as I know this annotation impacts BOTH the request and response body, where we'd need an annotation that only impacts the request body to model things properly. Do you know whether we could add additional terms? like Org.OData.Core.V1.Post.RequestBody.MediaType or so? (and all the declinations for other verbs and response/request body)

irvinesunday commented 1 year ago

I like the idea of having a sort of "OpenAPI transform step" where we can supply a subset OpenAPI doc (in YAML/JSON) that has the specific paths (and operations) that we'd want to substitute with the ones present in the converted doc. This can apply some sort of "layering-over" the main document. This YAML doc. will obviously be deserialized before being used to replace the target paths (and constituent operations). The YAML OpenAPI doc. can be supplied via a string property in the convert settings in the OpenAPI.NET.OData lib.

baywet commented 1 year ago

I'd keep it a separate process, this need is very specific to our edge cases. And layering patches over the conversion result is not the concern of the conversion library. As a general direction, moonwalk (OpenAPI v4) is supposed to add layering natively to the standard, which would mean we would be able to use that patch file "as is" in the future.