Adding Communication CallingServer API preview 0.#14543
Conversation
|
Hi, @zihzhan-msft Thanks for your PR. I am workflow bot for review process. Here are some small tips. Any feedback about review process or workflow bot, pls contact swagger and tools team. vsswagger@microsoft.com |
|
[Call for Action] To better understand Azure service dev/test scenario, and support Azure service developer better on Swagger and REST API related tests in early phase, please help to fill in with this survey https://aka.ms/SurveyForEarlyPhase. It will take 5 to 10 minutes. If you already complete survey, please neglect this comment. Thanks. |
Swagger Validation Report
|
|
Swagger pipeline restarted successfully, please wait for status update in this comment. |
zihzhan-msft
force-pushed
the
zihzhan/communication-callingserver-preview1
branch
from
fce422a to
d9fab6e
Compare
zihzhan-msft
force-pushed
the
zihzhan/communication-callingserver-preview1
branch
from
d9fab6e to
c162856
Compare
* Updated the servercalling swagger with playAudio api for out-call * Fixed a typo Co-authored-by: Paresh Arvind Patil <papati@microsoft.com>
zihzhan-msft
force-pushed
the
zihzhan/communication-callingserver-preview1
branch
from
cf30c22 to
4f3481d
Compare
zihzhan-msft
changed the title
zihzhan-msft
force-pushed
the
zihzhan/communication-callingserver-preview1
branch
from
d5a347f to
1d36d17
Compare
* Removed Model and Internal postfix, added events in swagger * update the title
markweitzel
added
the
APIStewardshipBoard-ReviewRequested
* Updated swagger with version 2021-06-15 * Fix the Communication.Common definitions path * Removed old swagger
…into zihzhan/communication-callingserver-preview1
| ], | ||
| "summary": "Create a new call.", | ||
| "description": "Create a new call.", | ||
| "operationId": "CallConnection_CreateCall", |
There was a problem hiding this comment.
The first part of the operationId (i.e. CallConnection) becomes the name of the client exposing APIs, it is recommended to use the plural name CallConnections.
There was a problem hiding this comment.
As we determine the right taxonomy for call/connection etc, we should make sure that the API pattern is:
POSTin to /calling/<term for callConnections in plural> responds with the Location (standard header) to the newly created /calling/<term for callConnections in plural>/<generated "name" of the created callConnection>. The name of the model at that location should be .
E.g.
POST /calling/callConnections -> (Location of created CallConnection) GET /calling/callConnections/{callConnectionName} -> CallConnection
There was a problem hiding this comment.
We use a guid to identify the callConnection and call it callConnectionId. Is this what you are referring to as callConnectionName or is that different? Currently the way it works is:
POST /calling/callConnections returns callConnectionId in the body of the 201 response. The app can do GET /calling/callConnections/callConnectionId Are you suggesting we should return the URL in the response?
There was a problem hiding this comment.
id is generally a fully qualified URL, which is why I suggested a different name for the "last segment".
And, yes, for a POST/201 response, I would expect a Location header indicating where the resource got created.
| } | ||
| ], | ||
| "responses": { | ||
| "201": { |
There was a problem hiding this comment.
This seems like a long-running operation, you can annotate it as LRO e.g. here
There was a problem hiding this comment.
Could you point me to some doc on what the annotation "x-ms-long-running-operation" would do. The APIs for creating the call and hanging up the call rely on events coming back to the callbackUri in CreateCallRequest. The events give the callConnectionState to the application
There was a problem hiding this comment.
Per my understanding, these are not long running operations yet. The only way to get notified of operation completion is (currently) the notification callback.
In practical terms, it means that any generated library will change once the service adds support for polling for completion rather than the existing notification path. But since this is a preview, that is acceptable.
There was a problem hiding this comment.
Yes this is correct. We will revisit this post preview.
| } | ||
| ], | ||
| "responses": { | ||
| "202": { |
There was a problem hiding this comment.
Same comment as 201 LRO, we can use x-ms-long-running-operation
…//github.com/Azure/azure-rest-api-specs into zihzhan/communication-callingserver-preview1
|
Some comments on the API definition:
Style issues
|
| } | ||
| } | ||
| }, | ||
| "/calling/callConnections/{callConnectionId}/:hangup": { |
There was a problem hiding this comment.
I find it very likely that we are going to want to be able to handle the race condition of hanging up/at the same time as the call was being answered. In other words, differentiating between giving up due to no answer and hanging up an active connection. We should make room for this in the API.
There was a problem hiding this comment.
If the callee doesn't answer the call, after a timeout, the call will go to Disconnected state. If the app wants to hangup while the call is still ringing, app can call hangup() and we will handle that internally. Do you see a benefit for the developer in differentiating between these two cases? Could you suggest what they would differently.
There was a problem hiding this comment.
You allow application developers to avoid exposing called parties the annoyance of answering and hearing the other party hang up. It is my recollection (which is about 2 decades old by this time) that all libraries we used distinguished between the two. And as I look at the code CreateCallRequest, I am also missing a timeout parameter that allows the caller to determine for how long they want to wait for the destination to "pick up the phone".
| "description": "The value to identify context of the operation.", | ||
| "type": "string" | ||
| }, | ||
| "audioFileId": { |
There was a problem hiding this comment.
Why do you need this? The audioFileUri should identify the file uniquely, correct? And if you want to enable caching, you can also use conditional requests/if-none-match/if-modified-since to only get the file if it has changed.
There was a problem hiding this comment.
audioFileId is used for caching. The service uses the cached file if the same audioFileId is used. Otherwise it will download it again. We can consider alternatives to this post-public preview
There was a problem hiding this comment.
It is ok to do this after the public preview. I would recommend renaming the field, however. There is nothing in the name audioFieldId that hints at what it is being used for. I suspect that the name had bled out from the internal implementation/the name of the field in the data store you are using.
| "type": "object", | ||
| "properties": { | ||
| "id": { | ||
| "description": "Gets or sets the identifier.", |
There was a problem hiding this comment.
Please avoid "get or sets " comments in the description. It does not provide any value to the customer. You need to document what the id field is.
| "required": [ | ||
| "targets", | ||
| "source", | ||
| "callbackUri" |
There was a problem hiding this comment.
How is the callbackUri called/secured? See the subscriptions section of the API guidelines to learn more about the expected call sequence...
There was a problem hiding this comment.
Currently we are relying on the client secret mechanism as used by EventGrid: https://docs.microsoft.com/en-us/azure/event-grid/security-authentication#using-client-secret-as-a-query-parameter Post-public preview we will be considering other mechanisms such as AAD
There was a problem hiding this comment.
How was the client secret provided? Is the client expected to include the query parameter in the callbackUri when setting up the subscription (e.g. when creating the callConnection)? Or is a property of the ACS resource itself?
There was a problem hiding this comment.
the client should include it in the callbackUri when creating the subscription.
| "$ref": "#/definitions/PhoneNumberIdentifier", | ||
| "description": "The alternate identity of the source of the call if dialing out to a pstn number" | ||
| }, | ||
| "targets": { |
There was a problem hiding this comment.
I see that we are also using the term "participants" when inviting more parties to the call. We should find a consistent term to use (I know I mentioned "to" or "destination" in the in-person review/overview earlier, but that may not make as much sense outside the context of making the call)
There was a problem hiding this comment.
We need to think about this to see if we can find a better name. For public preview can we go with what we have currently? Client Calling SDK is also using "addParticipant" for this. https://docs.microsoft.com/en-us/azure/communication-services/quickstarts/voice-video-calling/calling-client-samples?pivots=platform-web
|
I don't see anything blocking a preview release. However, there are a couple of important questions, the answer to which may have fairly significant impact on the eventual API (e.g. clientCallConnection, serverCallConnection naming, can they be combined etc.). |
…s. Changed invite participant to add participant. Other model name related changes.
|
@anuchandy @mikekistler @johanste Thanks for all the feedback. We will be addressing the feedback post preview and also doing the user studies as we discused to resolve the naming, etc. Could we have this PR approved and merged into master or do we need to do anything else before that? In reply to: 859892523 |
mikekistler
left a comment
mikekistler
left a comment
There was a problem hiding this comment.
I think all my concerns have been addressed.
|
Johan confirmed that pr is good, the naming and combining of operations will be a follow-up (post preview). Merging it. |
* Adding Communication CallingServer API preview1. * Updated the Swagger to add PlayAudio api for out-call scenario (Azure#14578) * Updated the servercalling swagger with playAudio api for out-call * Fixed a typo Co-authored-by: Paresh Arvind Patil <papati@microsoft.com> * Add preview0 swagger for .net sdk. * Update SDK Swagger preview0 * Update swagger api version. * Update swagger api version. * Removed Model and Internal postfix, added events in swagger (Azure#14639) * Removed Model and Internal postfix, added events in swagger * update the title * Add swagger for version 2021-06-15 (Azure#14720) * Updated swagger with version 2021-06-15 * Fix the Communication.Common definitions path * Removed old swagger * Added unhold to list of custom-word * Update readme file accourding to new swagger. * update responses model suffix to result * Removed error responses and consumes/produces values from all the apis. Changed invite participant to add participant. Other model name related changes. * Added examples for all the apis. * Removed error responses from example * Added back error response type, added response body for add participant operation * Fix join call example * Update the examples Co-authored-by: Paresh Arvind Patil <patilaparesh@gmail.com> Co-authored-by: Paresh Arvind Patil <papati@microsoft.com> Co-authored-by: navali-msft <66667092+navali-msft@users.noreply.github.com> Co-authored-by: Naveed Ali <navali@microsoft.com>
czubair
added
APIStewardshipBoard-SignedOff
10 participants
MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.
Changelog
Please ensure to add changelog with this PR by answering the following questions.
Contribution checklist:
If any further question about AME onboarding or validation tools, please view the FAQ.
ARM API Review Checklist
Ensure to check this box if one of the following scenarios meet updates in the PR, so that label “WaitForARMFeedback” will be added automatically to involve ARM API Review. Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs, all “removals” and “adding a new property” no more require ARM API review.
Please ensure you've reviewed following guidelines including ARM resource provider contract and REST guidelines. Estimated time (4 hours). This is required before you can request review from ARM API Review board.
If you are blocked on ARM review and want to get the PR merged with urgency, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.
Breaking Change Review Checklist
If there are following updates in the PR, ensure to request an approval from Breaking Change Review Board as defined in the Breaking Change Policy.
Action: to initiate an evaluation of the breaking change, create a new intake using the template for breaking changes. Addition details on the process and office hours are on the Breaking change Wiki.
Please follow the link to find more details on PR review process.