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

Type base64 encoded FileAttachment.contentBytes as a string #322

Conversation

RobvH
Copy link

@RobvH RobvH commented Jul 14, 2022

Thanks for taking the time to review this PR!

This PR changes the described type of contentBytes from NullableOption<number> to NullableOption<string>.

Why was it mistyped?

The documentation, imprecisely describes it in contradicting terms: Edm.Binary and "The base64-encoded contents of the file." The field cannot be both at the same time. Instead, it is clear from use of the Api that the contents sent and accepted by the Api are base64 encoded (string) of a binary. After decoding, the type would be an Edm.Binary, but not in the FileAttachment object itself.

Impact

Because the type incorrectly describes the contents as a number, test fixtures built using actual FileAttachment responses appear as the wrong shape to Typescript, forcing the test author to cast through unknown and effectively reducing the value of the type.

It was described in this issue: #23 and never resolved. I suspect this happened because the contradiction of the docs wasn't clearly discussed. We all know base64 encoded binaries are strings, the docs simply mis-describe this in the type field while accurately describing it in the description field.

Reference: https://docs.microsoft.com/en-us/graph/api/resources/fileattachment?view=graph-rest-1.0

@nikithauc
Copy link
Contributor

Thank you highlighting this issue @RobvH!

This file is auto generated from the metadata. https://github.com/microsoftgraph/msgraph-metadata/. I will trace this issue and get back on this issue.

@RobvH
Copy link
Author

RobvH commented Aug 22, 2022

Thank you highlighting this issue @RobvH!

This file is auto generated from the metadata. https://github.com/microsoftgraph/msgraph-metadata/. I will trace this issue and get back on this issue.

Thanks, I'll close this one as I see the fix is in #328 . Is there a usual cadence to those PRs getting merged?

@RobvH RobvH closed this Aug 22, 2022
@RobvH RobvH deleted the correct_FileAttachment_contentBytes_type branch August 22, 2022 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants