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

Specific to Microsoft Graph (service providers): Re-ordered URL Query Params causes trouble #123

Closed
olihey opened this issue Feb 4, 2018 · 6 comments
Assignees

Comments

@olihey
Copy link

olihey commented Feb 4, 2018

Hej,

I am working with Microsoft Graph and resty. Awesome so far but now I ran into an issue. I get download / upload URLs from the API and using them with resty always fails. The reason is that resty reorders the URLs query parameters.

For example this is the URL I get from Microsoft:

https://mycompany.sharepoint.com/personal/XXXXXXXX_onmicrosoft_com/_layouts/15/download.aspx?UniqueId=ead1d0ed-XXX-XXX-XXX-abb7612b3146&Translate=false&tempauth=eyJ0eXAiOiJKV1QiLC...HZEhwVnJ1d0NSUGVLaUpSaVNLRG5scz0&ApiVersion=2.0

but resty finally sends the following:

https://mycompany.sharepoint.com/personal/XXXXXXXX_onmicrosoft_com/_layouts/15/download.aspx?ApiVersion=2.0&Translate=false&UniqueId=ead1d0ed-XXX-XXX-XXX-abb7612b3146&tempauth=eyJ0eXAiOiJKV1QiLC...HZEhwVnJ1d0NSUGVLaUpSaVNLRG5scz0

I know that "technically" those two URLs express the same but the Microsoft Graph API seems to be picky about them. So after some debugging I found out that I can make this work with resty when I remove the parseRequestURL from the beforeRequest list.

Any chance we can make this an option? There is already an SetDoNotParseResponse. I am happy to contribute a SetDoNotParseRequestURL as PR. Would that be okay?

@olihey olihey changed the title Re-ordered URL causes troubel Re-ordered URL causes trouble Feb 4, 2018
@jeevatkm jeevatkm changed the title Re-ordered URL causes trouble Specific to Microsoft Graph (service providers): Re-ordered URL causes trouble Feb 4, 2018
@jeevatkm jeevatkm self-assigned this Feb 4, 2018
@jeevatkm
Copy link
Member

jeevatkm commented Feb 4, 2018

Microsoft Graph API seems to be picky about them.

@olihey sorry to hear that. Thank you for sharing your experience. Technically SetDoNotParseResponse behavior, we cannot be compare with this.

To bring better experience; let me have a look and get back to you.

@jeevatkm
Copy link
Member

jeevatkm commented Feb 4, 2018

@olihey I have done my analyzes. As you know per RFC there is no order preserve requirement.

Also standard package url.Values.Encode(...) sorts query params alphabetically.

To bring balance and better resty user experience. I have enhanced the resty to preserve query string order partially, it means "resty will preserve query string order on provided URL not on SetQuery* methods". This enhancement is available at branch preserve-query-string-order-partially

Please give it try and share your feedback. Then it will be part of v1.3 release.

@jeevatkm jeevatkm added this to the v1.3 Milestone milestone Feb 4, 2018
@jeevatkm jeevatkm changed the title Specific to Microsoft Graph (service providers): Re-ordered URL causes trouble Specific to Microsoft Graph (service providers): Re-ordered URL Query Params causes trouble Feb 5, 2018
@olihey
Copy link
Author

olihey commented Feb 5, 2018

The fix works for me. Thanks a lot 👍
But I am a bit torn about this. I also read a bit into this and for me the issue actually lies in the Microsoft Graph API and not resty. I opened an issue at Microsoft and looking forward to what they say ( OneDrive/onedrive-api-docs#792 )

If you don't see any issue in merging the changes you made, then please go for it.

@jeevatkm
Copy link
Member

jeevatkm commented Feb 5, 2018

@olihey Thanks I'm glad to merge this enhancement. It is fully compatible with RFC and previous resty versions too.

@olihey
Copy link
Author

olihey commented Feb 5, 2018

Great, thanks again for the quick help!

@jeevatkm
Copy link
Member

jeevatkm commented Feb 6, 2018

Merged 😄

@jeevatkm jeevatkm closed this as completed Feb 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants