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.

Sign up for GitHub

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

fix the description of queue sendMessage's VisibilityTimeout option's… #11459

Merged
merged 1 commit into from
Oct 30, 2020
Merged

fix the description of queue sendMessage's VisibilityTimeout option's… #11459

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion specification/storage/data-plane/Microsoft.QueueStorage/preview/2018-03-28/queue.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -829,7 +829,7 @@
"$ref": "#/parameters/RequiredQueueMessage"
},
{
"$ref": "#/parameters/VisibilityTimeout"
"$ref": "#/parameters/VisibilityTimeoutForEnqueue"
},
{
"$ref": "#/parameters/MessageTTL"
Expand Down Expand Up @@ -1862,6 +1862,17 @@
"maximum": 604800,
"x-ms-parameter-location": "method",
"description": "Optional. Specifies the new visibility timeout value, in seconds, relative to server time. The default value is 30 seconds. A specified value must be larger than or equal to 1 second, and cannot be larger than 7 days, or larger than 2 hours on REST protocol versions prior to version 2011-08-18. The visibility timeout of a message can be set to a value later than the expiry time."
},
"VisibilityTimeoutForEnqueue": {
"name": "visibilitytimeout",
"x-ms-client-name": "visibilityTimeout",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is using a different casing than previously, visibility**T**imeout vs visibility**t**imeout, is that going to cause an issue with generation?

Copy link
Member Author

@ljian3377 ljian3377 Oct 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I first went without "x-ms-client-name": "visibilityTimeout" but it turned out to be named visibilitytimeout in JavaScript generated code, while before this change, the original name has camel case - visibilityTimeout.

I don't want to change the name here, i.e. want to keep the camel case as it was; so add the x-ms-client-name to enforce it. Worked in JavaScript. May need to verify in other languages.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out we have this transform in JavaScript README.md

Rename visibilitytimeout -> visibilityTimeout

directive:
  - from: swagger-document
    where: $.parameters.VisibilityTimeout
    transform: >
      $["x-ms-client-name"] = "visibilityTimeout";
  - from: swagger-document
    where: $.parameters.VisibilityTimeoutRequired
    transform: >
      $["x-ms-client-name"] = "visibilityTimeout";

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, looks like .NET don't have this transform.
https://github.com/Azure/azure-sdk-for-net/tree/master/sdk/storage/Azure.Storage.Queues/swagger

I will remove "x-ms-client-name": "visibilityTimeout" in the swagger and add another transform in JavaScript.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "name": "visibilityTimeout" is enough x-ms-client-name would be to override it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But to @alzimmermsft 's comment we should fix casing in name. Otherwise compilation may fail in .NET/Java

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry my bad. looks like in other versions of this parameter it's all lower-case. Plz disregard previous comment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my understanding this should use visibilitytimeout as that is what it would previously generate with: https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/storage/Azure.Storage.Queues/src/Generated/QueueRestClient.cs#L1868

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed "x-ms-client-name": "visibilityTimeout" in #11461

"in": "query",
"required": false,
"type": "integer",
"minimum": 0,
"maximum": 604800,
"x-ms-parameter-location": "method",
"description": "Optional. If specified, the request must be made using an x-ms-version of 2011-08-18 or later. If not specified, the default value is 0. Specifies the new visibility timeout value, in seconds, relative to server time. The new value must be larger than or equal to 0, and cannot be larger than 7 days. The visibility timeout of a message cannot be set to a value later than the expiry time. visibilitytimeout should be set to a value smaller than the time-to-live value."
}
}
}