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

Handle configured API_URLs that have a path: #4995

Merged
merged 2 commits into from
Jul 19, 2017

Conversation

durran
Copy link
Contributor

@durran durran commented Jul 19, 2017

For the case when a user has modified mapboxgl.config.API_URL that
includes a path such as http://test.example.com/api.mapbox.com the
path was getting removed when building the full URL as it was being
overwritten by the original path from the request.

This pull request checks if the generation of the apiUrlObject has a
path other than '/' (which is the default), and if so we need to prepend it to the
original urlObject path.

This fixes the case where users using a reverse proxy to mapbox have
filtered on the specific path set in their API_URL, and when it has been
removed the lack of a match causes the proxy to not forward the requests
.

Debug page:

screen shot 2017-07-19 at 9 34 43 pm

Benchmarks:

benchmark | master 8567504 | api-url-with-path d0feffa
--- | --- | ---
**map-load** | 298 ms  | 304 ms 
**style-load** | 1,048 ms  | 95 ms 
**buffer** | 1,173 ms  | 1,163 ms 
**fps** | 57 fps  | 60 fps 
**frame-duration** | 8.1 ms, 2% > 16ms  | 7.9 ms, 1% > 16ms 
**query-point** | 1.15 ms  | 1.24 ms 
**query-box** | 86.21 ms  | 95.22 ms 
**geojson-setdata-small** | 7 ms  | 4 ms 
**geojson-setdata-large** | 170 ms  | 167 ms 

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page

For the case when a user has modified mapboxgl.config.API_URL that
includes a path such as http://test.example.com/api.mapbox.com the
path was getting removed when building the full URL as it was being
overwritten by the original path from the request.

This pull request checks if the generation of the apiUrlObject has a
path other than '/' (which is the default) we need to prepend it to the
original urlObject path.

This fixes the case where users using a reverse proxy to mapbox have
filtered on the specific path set in their API_URL, and when it has been
removed the lack of a match causes the proxy to not forward the requests
.
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.

Thanks for the contribution @durran!

@@ -17,6 +17,10 @@ function makeAPIURL(urlObject: UrlObject, accessToken: string | null | void): st
urlObject.protocol = apiUrlObject.protocol;
urlObject.authority = apiUrlObject.authority;

if (apiUrlObject.path !== '/') {
urlObject.authority = `${urlObject.authority}${apiUrlObject.path}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this prepend apiUrlObject.path to urlObject.path, rather than appending it to authority? That would be clearer and more robust.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More than happy to make that change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jfirebaugh Changes made - we now prepend to the urlObject.path instead of appending to urlObject.authority.

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.

Thank you!

@jfirebaugh jfirebaugh merged commit ca243c7 into mapbox:master Jul 19, 2017
@durran
Copy link
Contributor Author

durran commented Jul 19, 2017

Thanks!

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

Successfully merging this pull request may close these issues.

2 participants