GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — [Feature] Add Multiple Link Header Support by markekraus · Pull Request #5265 · PowerShell/PowerShell

Conversation

Mark Kraus (markekraus) commented Oct 28, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

fixes #5257

Adds support for the following response Header scenario:

Link: <https://MYDEVORG.okta.com/api/v1/users?limit=200&filter=status+eq+%22STAGED%22>; rel="self"
Link: <https://MYDEVORG.okta.com/api/v1/users?after=OKTAID&limit=200&filter=status+eq+%22STAGED%22>; rel="next"

CoreFX treats multiple header responses as separate array elements. Unless each array element is parsed, only the first one would be accepted. The current implimentation not only returns an incomplete RelationLink dictionary, but also breaks -FollowRelLink when a sever responds with multiple Link headers. This PR corrects both the pagination and dictionary.

The test can currently only be done on Windows as we currently lack a native server that supports returning multiple headers with the same name (see #4639). The test would only result in a false pass on Linux and macOS.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          [Feature] Add multiple link header support

57df0c2

Mark Kraus (markekraus) requested review from James Truher (JamesWTruher), Paul Higinbotham (PaulHigin) and Dan Travison (dantraMSFT) as code owners

October 28, 2017 15:31

Ilya (iSazonov) assigned Travis Plunk (TravisEz13)

Oct 30, 2017

Ilya (iSazonov) reviewed

Oct 30, 2017

View reviewed changes

Ilya (iSazonov) left a comment

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1 Outdated

		$result.Output.RelationLink["last"] \| Should BeExactly "http://localhost:8080/PowerShell?test=linkheader&maxlinks=5&linknumber=5"
		}

		# Test pending support for multiple header capable server on Linux/macOS ses issue #4639

Ilya (iSazonov) Oct 30, 2017

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Typo - ses.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mark Kraus (markekraus) Oct 30, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          fix typo

5c052d9

Ilya (iSazonov) reviewed

Oct 31, 2017

View reviewed changes

Comment thread

...PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs

		_relationLink.Add(rel, absoluteUri.AbsoluteUri.ToString());
		string url = match.Groups["url"].Value;
		string rel = match.Groups["rel"].Value;
		if (url != String.Empty && rel != String.Empty && !_relationLink.ContainsKey(rel))

Ilya (iSazonov) Oct 31, 2017

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I don't know if match.Success is true enough to exclude an exception. It seems safer to check String.IsNullOrEmpty() for url and rel.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mark Kraus (markekraus) Oct 31, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I don't know if match.Success

In this instance, it probably is. We are assured a string for both link and pattern so long as we don't mess with the pattern Regex.Match will never except and it true it the 2 indexes will exist in matches.

I agree on the String.IsNullOrEmpty() but the point of this PR is to address the Multiple Link Header issue, not to change this logic. Should that be done in a separate PR?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) Oct 31, 2017

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Doc says that there is no null.

Closed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Paul Higinbotham (PaulHigin) approved these changes

Oct 31, 2017

View reviewed changes

Travis Plunk (TravisEz13) merged commit 57f3c85 into PowerShell:master

Nov 1, 2017

dddugan mentioned this pull request

Nov 22, 2017

Invoke-RestMethod -FollowRelLink should return a single array of objects #5526

Closed

Chris Ruscio (cruscio) mentioned this pull request

Dec 11, 2017

Link Header pagination fails when there's no space between ';' and 'rel' #5667

Closed

Mark Kraus (markekraus) deleted the MultipleRelLinkFix branch

January 19, 2018 18:58

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request

Aug 6, 2026


          Add Multiple Link Header Support (PowerShell#5265)

2df3e3c

Reviewers

Ilya (iSazonov)

iSazonov left review comments

Dan Travison (dantraMSFT)

Awaiting requested review from dantraMSFT

James Truher (JamesWTruher)

Awaiting requested review from JamesWTruher

+1 more reviewer

Paul Higinbotham (PaulHigin)

PaulHigin approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

Travis Plunk (TravisEz13)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Link header pagination only parsing first rel

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

Mark Kraus (markekraus) commented Oct 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ilya (iSazonov) left a comment

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) Oct 30, 2017

Choose a reason for hiding this comment

Uh oh!

Mark Kraus (markekraus) Oct 30, 2017

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) Oct 31, 2017

Choose a reason for hiding this comment

Uh oh!

Mark Kraus (markekraus) Oct 31, 2017

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) Oct 31, 2017

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Mark Kraus (markekraus) commented Oct 28, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.