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
protocol JSON schema: required fields of ConversationAccount don't match written specification #6541
Comments
@johnataylor this is related to the issue you are assigned to here: microsoft/BotFramework-Emulator#2397 |
@drmason13 thanks for highlighting the inconsistencies. I'm trying to determine what would be the most effective approach to resolving this, perhaps you could provide some input to the process. I'm wondering whether it could be resolved in the implementation or the spec. I don't think there are other implementations outside of the Azure Bot Service and the Bot Framework SDK. But we have to avoid shipping any breaking changes to implementation on a minor release. At the same time, we would want to be mindful of the original intentions captured in the spec. For example, how is this impacting your particular scenario? |
Thanks, the impact on me personally is minimal. I have edited a local copy of the JSON schema and used that to generate client API bindings, and I have been editing those by hand since. I think the implementations are actually correct, according to the spec and it would be unwise to change them now. I think the JSON schema (as opposed to the spec) is simply incorrect, it seems safe to change from my perspective. Is it a breaking change to change the schema in such a way that increases the set of valid activities? It's definitely the friendlier kind of breaking change. Nothing should suddenly start erroring on previously valid inputs as a result of the change I proposed. |
Oh, the breaking part would be that clients that previously assumed those required fields were always there would break if/when they stopped being there... even if that isn't true in practice now (for the emulator at least), it could become true. Safest to consider it for the next major release I expect. |
Thanks, we'll consider this in the next major release. |
Github issues should be used for bugs and feature requests. Use Stack Overflow for general "how-to" questions.
Version
commit 0fa7c51 of the JSON schema (itself unversioned as far as I'm aware)
Describe the bug
The JSON schema doesn't match the written specification at https://github.com/microsoft/botframework-sdk/blob/main/specs/botframework-activity/botframework-activity.md#conversation.
ConversationAccount
requries all of id, name, isGroup and conversationType in the JSON schema.Whereas the written specification explicitly states:
name being a required field doesn't match "The conversation.name field is optional", for a start.
To Reproduce
Steps to reproduce the behavior:
This can be reproduced using the emulator, see microsoft/BotFramework-Emulator#2397
Expected behavior
Only id field in ConversationAccount should be required in the JSON Schema definition.
The following JSON should be a valid ConversationAccount:
The JSON schema should be able to validate activities that meet the specification generated by trusted/well behaved clients such as the emulator.
Screenshots
If applicable, add screenshots to help explain your problem.
Additional context
Add any other context about the problem here.
Tracking Status
Dotnet SDK TODO
Javascript SDK TODO
Python SDK TODO
Java SDK TODO
Samples TODO
Docs TODO
Tools TODO
The text was updated successfully, but these errors were encountered: