Azure / azure-rest-api-specs

The source for REST API specifications for Microsoft Azure.
MIT License
2.63k stars 5.06k forks source link

[IC3 ACS Server Calling ] API Review #24370

Closed azure-sdk closed 1 year ago

azure-sdk commented 1 year ago

New API Review meeting has been requested.

Service Name: IC3 ACS Server Calling Review Created By: Fariba Haghbin Review Date: 07/19/2023 09:00 AM PT Onboarding Record: PR: https://github.com/Azure/azure-rest-api-specs/pull/24482 Hero Scenarios Link: Not Provided Core Concepts Doc Link: Not Provided

Description:

Detailed meeting information and documents provided can be accessed here For more information that will help prepare you for this review, the requirements, and office hours, visit the documentation here

markweitzel commented 1 year ago

API Stewardship Board Review: 5-Jul-23

Consider modeling the kind as an anyOf and drop the Source suffix so that we are consistent in the naming, e.g. text, file, etc..

Next Steps

Fix the missing properties in the API view. Refactor the source for polymorphism & look at the names. Then good for preview.

mikekistler commented 1 year ago

Here's a quick overview of how to refactor PlaySource into a true polymorphic model.

First we change PlaySource into a base schema by removing the properties for the variants and declare "kind" as the discriminator.

    "PlaySource": {
      "required": [
        "kind"
      ],
      "type": "object",
      "discriminator": "kind",
      "properties": {
        "kind": {
          "$ref": "#/definitions/PlaySourceType"
        },
        "playSourceCacheId": {
          "description": "Defines the identifier to be used for caching related media",
          "type": "string"
        }
      }
    },

Then create a new schema for each of the variants. For the existing "file" variant, this looks like:

    "FilePlaySource": {
      "type": "object",
      "properties": {
        "file": {
          "$ref": "#/definitions/FileSource",
        }
      },
      "required": [
        "file"
      ],
      "allOf": [
        {
          "$ref": "#/definitions/PlaySource"
        }
      ],
      "x-ms-discriminator-value": "file"
    },

The "text" variant looks similar:

    "TextPlaySource": {
      "type": "object",
      "properties": {
        "text": {
          "$ref": "#/definitions/TextSource",
        }
      },
      "required": [
        "text"
      ],
      "allOf": [
        {
          "$ref": "#/definitions/PlaySource"
        }
      ],
      "x-ms-discriminator-value": "text"
    },

From there, I think you can see what the "ssml" variant should be -- or you can let CoPilot write it for you ;-).

arupdutta-msft commented 1 year ago

We already have FilePlaySource in GA. I am not sure how to make changes to that without breaking the existing sdk versions.

Also I have seen problem with deserializing polymorphic models over the wire, which would mean we will have to add a custom deserialization logic and maintain it.

Our customer facing SDK models are polymorphic already. I would prefer the API models for play source as they are currently. The current models keep our APIs simpler and reduces maintenance of custom code for deserialization.

mikekistler commented 1 year ago

This commit is a slight modification of the pattern I described above which introduces polymorphism for PlaySource but (I think) has no compatibility issues with the previously GA'ed REST API.