Skip to content
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

Closed
14 tasks
drmason13 opened this issue Sep 10, 2022 · 5 comments · May be fixed by #6542
Closed
14 tasks
Assignees
Labels
Bot Services Required for internal Azure reporting. Do not remove. Do not change color. bug Indicates an unexpected problem or an unintended behavior. customer-reported Issue is created by anyone that is not a collaborator in the repository. needs-triage The issue has just been created and it has not been reviewed by the team.

Comments

@drmason13
Copy link

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:

A2080: Channels, bots, and clients MUST include the conversation and conversation.id fields when generating an activity.

The conversation.name field is optional and represents the display name for the conversation if it exists and is available.

A2081: Channels SHOULD include the conversation.name and conversation.isGroup fields if they are available.

A2082: Bots and clients SHOULD NOT include the conversation.name field unless it is semantically valuable within the channel.

A2083: Bots and clients SHOULD NOT include the conversation.isGroup and conversation.converationType fields in activities they generate.

A2084: Channels SHOULD include the conversation.conversationType field if more than one value is defined for the channel. Channels SHOULD NOT include the field if there is only one possible value.

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:

"conversation": {
    "id": "40f7b490-2609-11ed-a12d-37cd6a1cd89f|livechat"
}

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

  • PR
  • Merged

Javascript SDK TODO

  • PR
  • Merged

Python SDK TODO

  • PR
  • Merged

Java SDK TODO

  • PR
  • Merged

Samples TODO

  • PR
  • Merged

Docs TODO

  • PR
  • Merged

Tools TODO

  • PR
  • Merged
@drmason13 drmason13 added bug Indicates an unexpected problem or an unintended behavior. needs-triage The issue has just been created and it has not been reviewed by the team. labels Sep 10, 2022
@stevkan stevkan added Bot Services Required for internal Azure reporting. Do not remove. Do not change color. customer-reported Issue is created by anyone that is not a collaborator in the repository. labels Sep 12, 2022
@LeeParrishMSFT
Copy link

@johnataylor this is related to the issue you are assigned to here: microsoft/BotFramework-Emulator#2397

@johnataylor
Copy link
Member

@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?

@drmason13
Copy link
Author

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.

@drmason13
Copy link
Author

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.

@johnataylor
Copy link
Member

Thanks, we'll consider this in the next major release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bot Services Required for internal Azure reporting. Do not remove. Do not change color. bug Indicates an unexpected problem or an unintended behavior. customer-reported Issue is created by anyone that is not a collaborator in the repository. needs-triage The issue has just been created and it has not been reviewed by the team.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants