Support Link Header pagination in WebCmdlets #3828
Conversation
…e end user implementing: https://github.com/PowerShell/PowerShell-RFC/blob/master/2-Draft-Accepted/RFC0021-Link-header-based-pagination-for-WebCmdlets.md When the response includes a Link Header (https://tools.ietf.org/html/rfc5988#page-6), for Invoke-WebRequest we create a RelationLink property that is a Dictionary representing the URLs and rel attributes and ensure the URLs are absolute to make it easier for the developer to use. For Invoke-RestMethod, we expose a -FollowRelLink switch to automatically follow 'next' rel links to the end until we hit the optional -MaxRelLink parameter value.
| /// </summary> | ||
| [Parameter] | ||
| [Alias("ML")] | ||
| [ValidateRange(0, Int32.MaxValue)] |
There was a problem hiding this comment.
Isn't -FL -ML 0 contradictory? If you agree, I would use 1 for the minimum.
| if (followedRelLink > 0) | ||
| { | ||
| string linkVerboseMsg = string.Format(CultureInfo.CurrentCulture, | ||
| "Following rel link {0}", |
There was a problem hiding this comment.
This string should be in a resx for localization.
There was a problem hiding this comment.
And I see you just followed the pattern in the rest of this method - you should fix all of them at the same time.
There was a problem hiding this comment.
Since there's only 3 I see in this file, I'll fix them with this PR
| /// <summary> | ||
| /// gets the RelationLink property | ||
| /// </summary> | ||
| public Dictionary<string, string> RelationLink { get; set; } |
There was a problem hiding this comment.
Does it make sense to have a public setter?
Also note how the Headers property is get only and returns a copy of the internal Headers. This isn't ideal because people write code like:
if (obj.Headers.ContainsKey("Something"))
{
obj.Headers["Something"] ...
}
And this usage would create 2 copies of the dictionary. It's better to use ReadOnlyDictionary.
There was a problem hiding this comment.
Created #3842 for the Headers issue
There was a problem hiding this comment.
Will make the setter internal
|
|
||
| // we only support the URL in angle brackets and `rel`, other attributes are ignored | ||
| // user can still parse it themselves via the Headers property | ||
| Regex regex = new Regex("<(?<url>.*?)>;\\srel=\"(?<rel>.*?)\""); |
There was a problem hiding this comment.
You should not create a new Regex like this, instead you should prefer the static method, see the best practices.
| { | ||
| return; | ||
| } | ||
| Uri = new Uri(_relationLink["next"]); |
There was a problem hiding this comment.
I think it we be better to use a local variable instead of modifying the cmdlet parameter.
| ThrowTerminatingError(er); | ||
| } | ||
|
|
||
| ParseLinkHeader(response, Uri); |
There was a problem hiding this comment.
Should you skip parsing the header when we're not following pages?
|
Docs update PR? |
|
@TravisEz13 added link to doc PR |
Implements https://github.com/PowerShell/PowerShell-RFC/blob/master/2-Draft-Accepted/RFC0021-Link-header-based-pagination-for-WebCmdlets.md
When the response includes a Link Header (https://tools.ietf.org/html/rfc5988#page-6), for Invoke-WebRequest we create a RelationLink property that is a Dictionary representing the URLs and rel attributes and ensure the URLs are absolute to make it easier for the developer to use. For Invoke-RestMethod, we expose a -FollowRelLink switch to automatically follow 'next' rel links to the end until we hit the optional -MaximumFollowRelLink parameter value.
Since we will eventually remove the FullClr code, I didn't make the effort to add the same changes to the CoreClr sources to the FullClr sources.
Fix #3041
Doc update MicrosoftDocs/PowerShell-Docs#1232