Added new specification for chat - release 2020-11-01-preview3#11716
Added new specification for chat - release 2020-11-01-preview3#11716tjprescott merged 10 commits intoAzure:masterfrom
Conversation
Swagger Validation Report
|
| Rule | Message |
|---|---|
| OperationId has a noun that conflicts with one of the model names in definitions section. The model name will be disambiguated to 'ChatThreadModel'. Consider using the plural form of 'ChatThread' to avoid this. Note: If you have already shipped an SDK on top of this spec, fixing this warning may introduce a breaking change. New: Microsoft.CommunicationServicesChat/preview/2020-11-01-preview3/communicationserviceschat.json#L477 |
|
| OperationId has a noun that conflicts with one of the model names in definitions section. The model name will be disambiguated to 'ChatThreadModel'. Consider using the plural form of 'ChatThread' to avoid this. Note: If you have already shipped an SDK on top of this spec, fixing this warning may introduce a breaking change. New: Microsoft.CommunicationServicesChat/preview/2020-11-01-preview3/communicationserviceschat.json#L398 |
|
| OperationId has a noun that conflicts with one of the model names in definitions section. The model name will be disambiguated to 'ChatThreadModel'. Consider using the plural form of 'ChatThread' to avoid this. Note: If you have already shipped an SDK on top of this spec, fixing this warning may introduce a breaking change. New: Microsoft.CommunicationServicesChat/preview/2020-11-01-preview3/communicationserviceschat.json#L170 |
|
| OperationId has a noun that conflicts with one of the model names in definitions section. The model name will be disambiguated to 'ChatThreadModel'. Consider using the plural form of 'ChatThread' to avoid this. Note: If you have already shipped an SDK on top of this spec, fixing this warning may introduce a breaking change. New: Microsoft.CommunicationServicesChat/preview/2020-11-01-preview3/communicationserviceschat.json#L245 |
|
| OperationId has a noun that conflicts with one of the model names in definitions section. The model name will be disambiguated to 'ChatThreadModel'. Consider using the plural form of 'ChatThread' to avoid this. Note: If you have already shipped an SDK on top of this spec, fixing this warning may introduce a breaking change. New: Microsoft.CommunicationServicesChat/preview/2020-11-01-preview3/communicationserviceschat.json#L328 |
|
| OperationId has a noun that conflicts with one of the model names in definitions section. The model name will be disambiguated to 'ChatThreadModel'. Consider using the plural form of 'ChatThread' to avoid this. Note: If you have already shipped an SDK on top of this spec, fixing this warning may introduce a breaking change. New: Microsoft.CommunicationServicesChat/preview/2020-11-01-preview3/communicationserviceschat.json#L988 |
|
| OperationId has a noun that conflicts with one of the model names in definitions section. The model name will be disambiguated to 'ChatThreadModel'. Consider using the plural form of 'ChatThread' to avoid this. Note: If you have already shipped an SDK on top of this spec, fixing this warning may introduce a breaking change. New: Microsoft.CommunicationServicesChat/preview/2020-11-01-preview3/communicationserviceschat.json#L96 |
|
| OperationId has a noun that conflicts with one of the model names in definitions section. The model name will be disambiguated to 'ChatThreadModel'. Consider using the plural form of 'ChatThread' to avoid this. Note: If you have already shipped an SDK on top of this spec, fixing this warning may introduce a breaking change. New: Microsoft.CommunicationServicesChat/preview/2020-11-01-preview3/communicationserviceschat.json#L546 |
|
| OperationId has a noun that conflicts with one of the model names in definitions section. The model name will be disambiguated to 'ChatThreadModel'. Consider using the plural form of 'ChatThread' to avoid this. Note: If you have already shipped an SDK on top of this spec, fixing this warning may introduce a breaking change. New: Microsoft.CommunicationServicesChat/preview/2020-11-01-preview3/communicationserviceschat.json#L608 |
|
| OperationId has a noun that conflicts with one of the model names in definitions section. The model name will be disambiguated to 'ChatThreadModel'. Consider using the plural form of 'ChatThread' to avoid this. Note: If you have already shipped an SDK on top of this spec, fixing this warning may introduce a breaking change. New: Microsoft.CommunicationServicesChat/preview/2020-11-01-preview3/communicationserviceschat.json#L689 |
️️✔️Avocado succeeded [Detail] [Expand]
Validation passes for Avocado.
️️✔️ModelValidation succeeded [Detail] [Expand]
Validation passes for ModelValidation.
️️✔️SemanticValidation succeeded [Detail] [Expand]
Validation passes for SemanticValidation.
️️✔️[Staging] Cross Version BreakingChange (Base on preview version) succeeded [Detail] [Expand]
There are no breaking changes.
️️✔️[Staging] Cross Version BreakingChange (Base on stable version) succeeded [Detail] [Expand]
There are no breaking changes.
Swagger Generation Artifacts
|
...crosoft.CommunicationServicesChat/preview/2020-11-01-preview3/communicationserviceschat.json
Outdated
Show resolved
Hide resolved
|
I see some build issues. You probably need to update the cSpell file and run prettier. |
@RezaJooyandeh It is complaining about This seems to be a new check, should we sluggify compound words? i.e. |
|
@amrElroumy good point. The recommendation is to use camelCase so maybe that actually solve the problem |
...crosoft.CommunicationServicesChat/preview/2020-11-01-preview3/communicationserviceschat.json
Outdated
Show resolved
Hide resolved
| "Threads" | ||
| ], | ||
| "summary": "Creates a chat thread.", | ||
| "operationId": "CreateChatThread", |
There was a problem hiding this comment.
It is good that we have added "Chat" to the types (eg. ChatMessage, ChatThread, etc.), but for the method names, it becomes redundant: ChatClient.CreateChatThread. I think we should remove the duplicated "Chat" from the operation ids so we would have ChatClient.CreateThread
There was a problem hiding this comment.
This one was discussed during private preview (for the method names included), and 3 of the architects were strong about adding Chat, so the decision was to with the prefix across the board. Their argument was that CreateThread or GetThread was too generic and Thread had a strong connotation in software.
Also just to be clear, the 'public' methods on the Clients are not driven by the swagger. They are all hand crafted.
...crosoft.CommunicationServicesChat/preview/2020-11-01-preview3/communicationserviceschat.json
Outdated
Show resolved
Hide resolved
...crosoft.CommunicationServicesChat/preview/2020-11-01-preview3/communicationserviceschat.json
Show resolved
Hide resolved
...crosoft.CommunicationServicesChat/preview/2020-11-01-preview3/communicationserviceschat.json
Outdated
Show resolved
Hide resolved
| ], | ||
| "responses": { | ||
| "200": { | ||
| "description": "Request successful. The action returns a `Thread` resource.", |
There was a problem hiding this comment.
We probably should remove "`" from the description as it is not necessarily going to be used in a markup.
There was a problem hiding this comment.
Thanks for fixing this. I just noticed there are few more with the "`" as well
...crosoft.CommunicationServicesChat/preview/2020-11-01-preview3/communicationserviceschat.json
Outdated
Show resolved
Hide resolved
...crosoft.CommunicationServicesChat/preview/2020-11-01-preview3/communicationserviceschat.json
Outdated
Show resolved
Hide resolved
| "description": "A chat message read receipt indicates the time a chat message was read by a recipient.", | ||
| "type": "object", | ||
| "properties": { | ||
| "senderId": { |
There was a problem hiding this comment.
@DominikMe we should standardized the format for identifiers in REST APIs based on what we talked about to make sure all our services are consistent.
There was a problem hiding this comment.
Yes, this should use the common CommunicationIdentifier type instead of string.
...crosoft.CommunicationServicesChat/preview/2020-11-01-preview3/communicationserviceschat.json
Outdated
Show resolved
Hide resolved
...crosoft.CommunicationServicesChat/preview/2020-11-01-preview3/communicationserviceschat.json
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
| }, | ||
| "ChatMessagePriority": { |
There was a problem hiding this comment.
Are we planning to remove this in a separate iteration? I talked to @ashwinder during public preview release, but it was too last-minute of a change. We agreed to hide this and when we have a more generalized approach bring it back (not necessarily, but maybe something like a property bag that developers can add key-values). "Priority" by itself is too specific to be a property and does not scale. Let's make sure we do it before we get to close to finalization
There was a problem hiding this comment.
There is a feature we have not started yet for tagging which will have something like that.
I have seen 'Priority' in other vendors though, so not sure how expected could be to have an explicit priority. Good to loop some product folks ..
| "example": "123456789" | ||
| }, | ||
| "type": { | ||
| "description": "Type of the chat message.\r\n \r\nPossible values:\r\n - Text\r\n - ThreadActivity/TopicUpdate\r\n - ThreadActivity/AddMember\r\n - ThreadActivity/DeleteMember", |
There was a problem hiding this comment.
We should probably remove "\r" and "\n"s from here.
There was a problem hiding this comment.
It is an unordered list that looks like this:
Type of the chat message.
Possible values:
- Text
- ThreadActivity/TopicUpdate
- ThreadActivity/AddMember
- ThreadActivity/DeleteMember
Wouldn't removing the new lines, make it hard to grok?
There was a problem hiding this comment.
Maybe it should become an extensible enum?
| "readOnly": true, | ||
| "example": "123456789" | ||
| }, | ||
| "type": { |
There was a problem hiding this comment.
Let's also talk about potentially removing the thread activities from here. We should not really return the xml responses. Specially if they are being delivered through eventgrid and real time channel
There was a problem hiding this comment.
This is also a feature on its own. I initially thought also a message should be just a message (always), but after hearing some of the requirements from other P1 clients (dynamics / rave), for them a message includes all types (system level messages, text user messages, etc..), so we probably want to account for those before going ahead.
There is also this underlying issue with pagination, so if we go with a separate end point, I think the right way of doing is is asking MFE to implement it, so we just expose it.
...crosoft.CommunicationServicesChat/preview/2020-11-01-preview3/communicationserviceschat.json
Outdated
Show resolved
Hide resolved
...crosoft.CommunicationServicesChat/preview/2020-11-01-preview3/communicationserviceschat.json
Outdated
Show resolved
Hide resolved
@amrElroumy The guidance is to use lowerCamelCase everywhere, so |
| }, | ||
| { | ||
| "in": "query", | ||
| "name": "maxPageSize", |
There was a problem hiding this comment.
guidelines name it differently, $maxpagesize
| "CreateChatThreadResult": { | ||
| "type": "object", | ||
| "properties": { | ||
| "chatThread": { |
There was a problem hiding this comment.
"description": "The thread created."
If we don't add descriptions, the sdks side complains with warnings.
Same for errors
There was a problem hiding this comment.
This is odd, the type already has xml documentation there.
There was a problem hiding this comment.
This is fixed now, the description is under the "ChatThread" definition.
…hat-2020-11-01-preview3
DominikMe
left a comment
DominikMe
left a comment
There was a problem hiding this comment.
Some comments around common definitions and property flags.
| "401": { | ||
| "description": "Unauthorized.", | ||
| "schema": { | ||
| "$ref": "#/definitions/Error" |
There was a problem hiding this comment.
We should reference the Error definition from the common.json that we checked in.
| }, | ||
| "x-ms-pageable": { | ||
| "nextLinkName": "nextLink", | ||
| "itemName": "value" |
There was a problem hiding this comment.
fyi this is not needed as value is the default. but doesn't hurt
| "description": "A chat message read receipt indicates the time a chat message was read by a recipient.", | ||
| "type": "object", | ||
| "properties": { | ||
| "senderId": { |
There was a problem hiding this comment.
Yes, this should use the common CommunicationIdentifier type instead of string.
| "readOnly": true, | ||
| "example": "2020-10-30T10:50:50Z" | ||
| }, | ||
| "senderId": { |
There was a problem hiding this comment.
this should be the common CommunicationIdentifier type
| "items": { | ||
| "$ref": "#/definitions/ChatMessage" | ||
| }, | ||
| "readOnly": true |
There was a problem hiding this comment.
Instead of readOnly we should mark properties add properties to required. Otherwise AutoREST generates poorly usable code. Properties that are not required, can then be marked readonly where applicable
There was a problem hiding this comment.
value for example should be required here
| "description": "The participants that failed to be added to the chat thread.", | ||
| "type": "array", | ||
| "items": { | ||
| "$ref": "#/definitions/Error" |
| "ChatThread": { | ||
| "description": "Chat thread.", | ||
| "type": "object", | ||
| "properties": { |
There was a problem hiding this comment.
Everything that's readonly here should be required instead.
| "readOnly": true, | ||
| "example": "19:uni01_uy5ucb66ugp3lrhe7pxso6xx4hsmm3dl6eyjfefv2n6x3rrurpea@thread.v2" | ||
| }, | ||
| "topic": { |
There was a problem hiding this comment.
I think we should also do a pass and mark everything as x-nullable: false where applicable
DominikMe
left a comment
DominikMe
left a comment
There was a problem hiding this comment.
Approving now to merge and iterate.
Let's reference this PR in a follow up to carry over and address remaining comments.
|
@DominikMe you need to hit merge too :( |
|
@tjprescott Travis, could you give us a hand merging this one ? Apparently all the folks with permissions are on vacations, and this is an incremental merge so we will need to do it few more times in the next week or so. |
…#11716) * Added new specification for chat - release 2020-11-01-preview3 * Addressing feedback * Updating max page size parameter name * update `maxpagesize` parameter name in example file. * Runn prettier on examples * Removing obsolete example files, and adding missing description properties. * Update `maxpagesize` query parameter name for `ListChatThreads` * rename $maxpagesize to `maxPageSize` Co-authored-by: Camilo Ramirez <juramir@microsoft.com> Co-authored-by: Amr Elroumy <amr.elroumy@microsoft.com>
…#11716) * Added new specification for chat - release 2020-11-01-preview3 * Addressing feedback * Updating max page size parameter name * update `maxpagesize` parameter name in example file. * Runn prettier on examples * Removing obsolete example files, and adding missing description properties. * Update `maxpagesize` query parameter name for `ListChatThreads` * rename $maxpagesize to `maxPageSize` Co-authored-by: Camilo Ramirez <juramir@microsoft.com> Co-authored-by: Amr Elroumy <amr.elroumy@microsoft.com>
6 participants
Added latest API for chat.