-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Adding Communication CallingServer API preview 0. #14543
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. |
fce422a
to
d9fab6e
Compare
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>
cf30c22
to
4f3481d
Compare
d5a347f
to
1d36d17
Compare
* Removed Model and Internal postfix, added events in swagger * update the title
* 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we will fix this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a long-running operation, you can annotate it as LRO e.g. here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this is correct. We will revisit this post preview.
} | ||
], | ||
"responses": { | ||
"202": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will fix
"required": [ | ||
"targets", | ||
"source", | ||
"callbackUri" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
* 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>
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.