New API preview version for ACS Email Inline Attachment feature #29699
Conversation
natekimball-msft
requested review from
mikekistler and
marclerwick
and removed request for
a team
Next Steps to Merge✅ All automated merging requirements have been met! To get your PR merged, see aka.ms/azsdk/specreview/merge. |
Swagger Validation Report
|
| Compared specs (v0.10.12) | new version | base version |
|---|---|---|
| CommunicationServicesEmail.json | 2024-07-01-preview(1a44c19) | 2023-03-31(main) |
| CommunicationServicesEmail.json | 2024-07-01-preview(1a44c19) | 2023-01-15-preview(main) |
The following breaking changes are detected by comparison with the latest preview version:
| Rule | Message |
|---|---|
| The new version has a different format 'byte' than the previous one ''. New: Email/preview/2024-07-01-preview/CommunicationServicesEmail.json#L305:9 Old: Email/preview/2023-01-15-preview/CommunicationServicesEmail.json#L305:9 |
️️✔️CredScan succeeded [Detail] [Expand]
There is no credential detected.
️⚠️LintDiff: 0 Warnings warning [Detail]
| Compared specs (v2.2.2) | new version | base version |
|---|---|---|
| package-2024-07-01-preview | package-2024-07-01-preview(1a44c19) | default(main) |
The following errors/warnings exist before current PR submission:
| Rule | Message |
|---|---|
| Path parameter should specify a maximum length (maxLength) and characters allowed (pattern). Location: Email/preview/2024-07-01-preview/CommunicationServicesEmail.json#L20 |
|
Error schema should define code and message properties as required.Location: Email/preview/2024-07-01-preview/CommunicationServicesEmail.json#L40 |
|
The error property in the error response schema should be required.Location: Email/preview/2024-07-01-preview/CommunicationServicesEmail.json#L40 |
|
Error schema should define code and message properties as required.Location: Email/preview/2024-07-01-preview/CommunicationServicesEmail.json#L125 |
|
The error property in the error response schema should be required.Location: Email/preview/2024-07-01-preview/CommunicationServicesEmail.json#L125 |
|
| Booleans properties are not descriptive in all cases and can make them to use, evaluate whether is makes sense to keep the property as boolean or turn it into an enum. Location: Email/preview/2024-07-01-preview/CommunicationServicesEmail.json#L237 |
️️✔️Avocado succeeded [Detail] [Expand]
Validation passes for Avocado.
️️✔️SwaggerAPIView succeeded [Detail] [Expand]
️️✔️TypeSpecAPIView succeeded [Detail] [Expand]
️️✔️ModelValidation succeeded [Detail] [Expand]
Validation passes for ModelValidation.
️️✔️SemanticValidation succeeded [Detail] [Expand]
Validation passes for SemanticValidation.
️️✔️PoliCheck succeeded [Detail] [Expand]
Validation passed for PoliCheck.
️️✔️SpellCheck succeeded [Detail] [Expand]
Validation passes for SpellCheck.
️️✔️Lint(RPaaS) succeeded [Detail] [Expand]
Validation passes for Lint(RPaaS).
️️✔️PR Summary succeeded [Detail] [Expand]
Validation passes for Summary.
️️✔️Automated merging requirements met succeeded [Detail] [Expand]
Swagger Generation Artifacts
|
Generated ApiView
|
…atekimball-msft/azure-rest-api-specs into natekimball/2024-07-01-preview
|
I believe the failure in check |
| "responses": { | ||
| "200": { | ||
| "headers": { | ||
| "retry-after": 100 |
There was a problem hiding this comment.
| "retry-after": 100 | |
| "retry-after": "100" |
| "202": { | ||
| "headers": { | ||
| "Operation-Location": "https://contoso.westus.communications.azure.com//emails/operations/8540c0de-899f-5cce-acb5-3ec493af3800?api-version=2023-03-31", | ||
| "retry-after": 20 |
There was a problem hiding this comment.
| "retry-after": 20 | |
| "retry-after": "20" |
| "headers": { | ||
| "retry-after": { | ||
| "description": "This header will only be present when the status is a non-terminal status. It indicates the minimum amount of time in seconds to wait before polling for operation status again.", | ||
| "type": "integer", |
There was a problem hiding this comment.
Are you sure this should be integer instead of string? The HTTP spec seems to require string to support date-based values.
There was a problem hiding this comment.
I can check the historical reasoning of this choice with the team and get back to you. That being said, I have a couple of follow up questions here:
If it's the spec that is incorrect, I'm curious as to why these errors are only manifesting in our builds now, despite using the same version of autorest? It appears that it wasn't caught in our team's previous PRs. Examples: Add base64 formatting to contentInBase64 property by kagbakpem · Pull Request #24617 · Azure/azure-rest-api-specs (github.com), Azure Communication Services Email API for GA by apattath · Pull Request #22979 · Azure/azure-rest-api-specs (github.com).
If I make changes to the older specs retroactively, are there any considerations here with introducing breaking changes? It seems like in our most recent PR, a tag was added to approve a breaking change (unsure if it was for the same reason). Is that appropriate here?
There was a problem hiding this comment.
For breaking changes, we regard the service behavior as the "source of truth". If the API definition changes to more accurately describe the service behavior, we allow this and actually encourage it.
There was a problem hiding this comment.
The check also failed in PR 22979. For PR 24617, the check was bypassed using label Approved-BreakingChange, and we no longer have the build log, but I suspect it also failed with the same error.
I think the first question here, is how should retry-after be expressed in services, specs, and examples? Always a string, or can each service decide between string and integer?
For instance, even though the HTTP spec allows string dates, if a service has decided it will ever only send integer seconds in the header, would it be appropriate for the service and spec to use integer?
@mikekistler: Do you know the answer?
There was a problem hiding this comment.
We always want the value of retry-after to be "delay-seconds" and not a date, and RFC 7231 says:
A delay-seconds value is a non-negative decimal integer, representing time in seconds.
So integer is the appropriate type for this header value.
Furthermore, it is common that string and integer values are serialized in headers in the same way, so it might be possible to change the API description from type: string to type: integer in a non-breaking way (i.e. without changing the behavior of the service). As long as the service does not wrap the value in quotes, e.g.
Retry-after: "180"
but most services will not do this so it might be fine.
There was a problem hiding this comment.
We always want the value of retry-after to be "delay-seconds" and not a date
@mikekistler: Can you elaborate on this? Is this an Azure guideline above the HTTP spec (which allows either)? Why wouldn't we want to give services the option to send either seconds or a date?
I understand it's valid for a service to choose to always send delay-seconds, and we may have reasons to prefer it (e.g. performance generating the response). But putting this restriction in the spec seems like overkill to me.
The guidance was changed recently in this PR (microsoft/api-guidelines#517), but even here it says SHOULD (not MUST) use delay-seconds:
DO include a
retry-afterheader in the response if the operation is not complete. The value of this header should be an integer number of seconds that the client should wait before polling the status monitor again.
Though it does use MUST in another place:
The response must look like this:
200 OK retry-after: <delay-seconds> (if status not terminal) <JSON Status Monitor Resource in body>
One more question, is the guidance to always use delay-seconds specific to 200 responses, or should it also apply to 202 responses? The spec in this PR has a retry-after example for both response codes.
There was a problem hiding this comment.
Retry-after should always specify delay-seconds in any response.
mikekistler
added
the
APIStewardshipBoard-SignedOff
natekimball-msft
added
the
PublishToCustomers
|
Hi, @natekimball-msft. Your PR has no update for 14 days and it is marked as stale PR. If no further update for over 14 days, the bot will close the PR. If you want to refresh the PR, please remove |
microsoft-github-policy-service
bot
added
the
no-recent-activity
natekimball-msft
removed
the
no-recent-activity
### Packages impacted by this PR azure/communication-email ### Issues associated with this PR N/A ### Describe the problem that is addressed by this PR N/A ### What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen? N/A - update to include contentId property for EmailAttachment to support inline attachments. ### Are there test cases added in this PR? _(If not, why?)_ Yes ### Provide a list of related PRs _(if any)_ [Java SDK PR](Azure/azure-sdk-for-java#41591) [.NET SDK PR](Azure/azure-sdk-for-net#45360) [REST API PR (complete)](Azure/azure-rest-api-specs#29699) ### Command used to generate this PR:**_(Applicable only to SDK release request PRs)_ Git command? git push --set-upstream origin natekimball/update-js-sdk-for-inline-attachments ### Checklists - [X] Added impacted package name to the issue description - [ ] Does this PR needs any fixes in the SDK Generator?** _(If so, create an Issue in the [Autorest/typescript](https://github.com/Azure/autorest.typescript) repository and link it here)_ - [X] Added a changelog (if necessary)
Data Plane API Specification Update Pull Request
Tip
Overwhelmed by all this guidance? See the
Getting helpsection at the bottom of this PR description.PR review workflow diagram
Please understand this diagram before proceeding. It explains how to get your PR approved & merged.
API Info: The Basics
Most of the information about your service should be captured in the issue that serves as your API Spec engagement record.
Is this review for (select one):
Change Scope
This section will help us focus on the specific parts of your API that are new or have been modified.
Please share a link to the design document for the new APIs, a link to the previous API Spec document (if applicable), and the root paths that have been updated.
/emails:send, specifically, the addition ofcontentIdto theEmailAttachmentdefinition, and updated description forattachmentsinEmailMessagedefinition. TheSendEmail.jsonexample has also been updated with sample usage of an inline image.Viewing API changes
For convenient view of the API changes made by this PR, refer to the URLs provided in the table
in the
Generated ApiViewcomment added to this PR. You can use ApiView to show API versions diff.Suppressing failures
If one or multiple validation error/warning suppression(s) is detected in your PR, please follow the
Swagger-Suppression-Process
to get approval.
❔Got questions? Need additional info?? We are here to help!
Contact us!
The Azure API Review Board is dedicated to helping you create amazing APIs. You can read about our mission and learn more about our process on our wiki.
Click here for links to tools, specs, guidelines & other good stuff
Tooling
Guidelines & Specifications
Helpful Links
Getting help
write accessper aka.ms/azsdk/access#request-access-to-rest-api-or-sdk-repositoriesNext Steps to Mergecomment. It will appear within few minutes of submitting this PR and will continue to be up-to-date with current PR state.and https://aka.ms/ci-fix.
queuedstate, please add a comment with contents/azp run.This should result in a new comment denoting a
PR validation pipelinehas started and the checks should be updated after few minutes.