Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

preserve tile query params #6432

Merged
merged 6 commits into from
Oct 3, 2016
Merged

preserve tile query params #6432

merged 6 commits into from
Oct 3, 2016

Conversation

mapsam
Copy link
Contributor

@mapsam mapsam commented Sep 22, 2016

Preserves any custom parameters for the Tile URL, strip out the first parameter (access token) and append the rest back into the tile URL, if they exist.

cc @jakepruitt @jfirebaugh @incanus

@mention-bot
Copy link

@mapsam, thanks for your PR! By analyzing this pull request, we identified @jfirebaugh, @tmcw and @yhahn to be potential reviewers.

@mapsam
Copy link
Contributor Author

mapsam commented Sep 22, 2016

Btw, this assumes an access token will always be first, which is probably not a good thing.

@mapsam
Copy link
Contributor Author

mapsam commented Sep 22, 2016

Updated the PR to search specifically for access_token anywhere in the query, remove it, and rebuild the query to be added to the final tile URL.

Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

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

Have you had a look at the test failures? Looks like this is not quite doing what you expect.

const auto query = url.substr(queryIdx);
// get access token index
const auto accessTokenIdx = (query.find("access_token") == 0) ? 0 : query.find("access_token") - 1; // if it's not the first param, grab the & as well
const auto accessTokenEndIdx = (accessTokenIdx == 0) ? query.find("&", accessTokenIdx) + 1 : query.find("&", accessTokenIdx);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line assumes there is a second parameter by checking for query.find("&", accessTokenIdx). If the response from this is 0, we need to grab the end of the query for accessTokenEndIdx.

@mapsam
Copy link
Contributor Author

mapsam commented Sep 26, 2016

@jfirebaugh yep, tests were indeed failing. I've updated the URL work to use a std::regex instead as well as added some tests to Mapbox.CanonicalURL to check for different orders of URL parameters.

@mapsam
Copy link
Contributor Author

mapsam commented Oct 3, 2016

@jfirebaugh any chance I could get a review here? Much appreciated!

@jfirebaugh jfirebaugh merged commit 280fd8c into master Oct 3, 2016
@jfirebaugh jfirebaugh deleted the tile-url-query-preservation branch October 3, 2016 17:08
@mapsam
Copy link
Contributor Author

mapsam commented Oct 3, 2016

Thank you @jfirebaugh!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants