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

Allow transforming URLs before requesting #8084

Merged
merged 7 commits into from
Feb 20, 2017
Merged

Allow transforming URLs before requesting #8084

merged 7 commits into from
Feb 20, 2017

Conversation

kkaefer
Copy link
Contributor

@kkaefer kkaefer commented Feb 16, 2017

An alternative to #7485. This implements a way of modifying the URLs that get requested from the internet. Potential use cases are altering URLs or hostnames and supporting use cases as described in #7455

This implementation invokes the supplied callback on the main thread (where the DefaultFileSource:: setResourceTransform happens) to avoid threading issues. I benchmarked the time it takes to transform the URL, and typically got times <1ms roundtrip (for scheduling the callback on the main thread and returning the result to the OnlineFileSource thread), but it can sometimes be higher when the main thread is busy rendering the map.

To do:

  • Android implementation
  • Unit tests

Caveats:

  • Does not allow transforming asset:// and file:// URLs; the transform is only called on requests that go out to the internet
  • When a request is scheduled, the internet connection goes offline for a long time, and then goes back online, it'll use the URL that was transformed during initialization; it does not attempt to transform the URL again when the request is attempted again

When using this feature, note that requests will still be cached by their canonical path, and not the transformed path. This means that the transformed URL should point to the same resources (e.g. on a different server, or with an time-limited access token) to avoid a caching mismatch. This is up to the server operator and application developer to ensure.

@1ec5 1ec5 added Core The cross-platform C++ core, aka mbgl feature labels Feb 16, 2017
Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

This API looks good, and ensuring main thread execution will make it much harder for a developer to mess up. In addition to the comments below, please add a note to the iOS and macOS changelogs, since MGLOfflineStorageDelegate is fashioned as a public API.


/// Initializes the run loop shim that lives on the main thread.
void MGLInitializeRunLoop() {
static mbgl::util::RunLoop mainRunLoop;
Copy link
Contributor

Choose a reason for hiding this comment

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

If the intent is to ensure that this object gets constructed once per process and lives for the lifetime of the process, consider a load method or C++ static initializer, which doesn’t have to get called explicitly. Of the list on that page, +[MGLOfflineStorage load] or an __attribute__(constructor) function seem to be the most desirable options. (We have an example of a +load method in MGLAccountManager.) Avoid a framework initializer, since running C++ code in a framework initializer is a no-no.

@@ -0,0 +1,3 @@
#import "MGLFoundation.h"

void MGLInitializeRunLoop();
Copy link
Contributor

@1ec5 1ec5 Feb 16, 2017

Choose a reason for hiding this comment

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

Moving this method closer to the call site would make its purpose clearer. Otherwise, a brief documentation comment would be helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this method

/**
The receiver’s delegate.

An offline storage sends messages to its delegate to allow it to transform URLs
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: “An offline storage object

@param url The original URL to be transformed.
@return A URL that will now be downloaded.
*/
- (NSURL*)offlineStorage:(MGLOfflineStorage*)storage
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: space before * for consistency.

kind = MGLResourceKindSpriteJSON;
break;
default:
kind = MGLResourceKindUnknown;
Copy link
Contributor

Choose a reason for hiding this comment

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

Initialize kind to MGLResourceKindUnknown where you declare it above, and remove this default case. That way, we’ll get a warning here when a new mbgl::Resource::Kind is added.

encoding:NSUTF8StringEncoding]];
MGLResourceKind kind;
switch (res.kind) {
case mbgl::Resource::Kind::Style:
Copy link
Contributor

Choose a reason for hiding this comment

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

If you have autoindentation turned off in Xcode, you might want to select this code and hit ⌃I to reindent it. Otherwise, anyone touching this code will end up reindenting individual lines haphazardly when they hit : etc.

@@ -85,6 +85,19 @@ typedef NS_OPTIONS(NSUInteger, MGLMapDebugMaskOptions) {
#endif
};

/** TODO docs */
typedef NS_ENUM(NSUInteger, MGLResourceKind) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In general, we keep enumeration definitions close to the classes that use them, which in this case would be in MGLOfflineStorage.h. MGLMapDebugMaskOptions above was moved here in order to avoid duplication between iOS and macOS, while MGLUserTrackingMode is a mistake – it doesn’t even make sense for macOS.

typedef NS_ENUM(NSUInteger, MGLResourceKind) {
/** Unknown type */
MGLResourceKindUnknown,
/** TODO: docs */
Copy link
Contributor

Choose a reason for hiding this comment

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

As you document these values, you might want to link to relevant portions of the style specification (example), since we don’t document these resources anywhere else in the SDK.

@kkaefer
Copy link
Contributor Author

kkaefer commented Feb 17, 2017

Here's how to transform URLs on Android:

diff --git a/platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/MapboxApplication.java b/platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/MapboxApplication.java
index a10c6ea..80877e0 100644
--- a/platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/MapboxApplication.java
+++ b/platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/MapboxApplication.java
@@ -4,13 +4,16 @@ import android.app.Application;
 import android.os.StrictMode;
 
 import com.mapbox.mapboxsdk.Mapbox;
+import com.mapbox.mapboxsdk.storage.Resource;
+import com.mapbox.mapboxsdk.offline.OfflineManager;
+import com.mapbox.mapboxsdk.offline.OfflineManager.ResourceTransformCallback;
 import com.squareup.leakcanary.LeakCanary;
 
 import timber.log.Timber;
 
 import static timber.log.Timber.DebugTree;
 
-public class MapboxApplication extends Application {
+public class MapboxApplication extends Application implements ResourceTransformCallback {
 
   @Override
   public void onCreate() {
@@ -38,6 +41,11 @@ public class MapboxApplication extends Application {
       .build());
 
     Mapbox.getInstance(getApplicationContext(), getString(R.string.mapbox_access_token));
+    OfflineManager.getInstance(getApplicationContext()).setResourceTransform(this);
+  }
+
+  public String onURL(@Resource.Kind int kind, String url) {
+    return url;
   }
 
   private void initializeLogger() {

and here's how to do it on iOS:

diff --git a/platform/ios/app/MBXAppDelegate.h b/platform/ios/app/MBXAppDelegate.h
index 7ff321b..f70973e 100644
--- a/platform/ios/app/MBXAppDelegate.h
+++ b/platform/ios/app/MBXAppDelegate.h
@@ -1,8 +1,8 @@
-#import <UIKit/UIKit.h>
+#import <Mapbox/Mapbox.h>
 
 extern NSString * const MBXMapboxAccessTokenDefaultsKey;
 
-@interface MBXAppDelegate : UIResponder <UIApplicationDelegate>
+@interface MBXAppDelegate : UIResponder <UIApplicationDelegate, MGLOfflineStorageDelegate>
 
 @property (strong, nonatomic) UIWindow *window;
 
diff --git a/platform/ios/app/MBXAppDelegate.m b/platform/ios/app/MBXAppDelegate.m
index c2834bf..b8cfd56 100644
--- a/platform/ios/app/MBXAppDelegate.m
+++ b/platform/ios/app/MBXAppDelegate.m
@@ -23,7 +23,16 @@ - (BOOL)application:(UIApplication *)application didFinishLaunchingWithOptions:(
         [MGLAccountManager setAccessToken:accessToken];
     }
 
+    [[MGLOfflineStorage sharedOfflineStorage] setDelegate:self];
+
     return YES;
 }
 
+- (NSURL *)offlineStorage:(MGLOfflineStorage *)storage
+    URLForResourceOfKind:(MGLResourceKind)kind
+                 withURL:(NSURL *)url {
+    // Change the URL here if required.
+    return url;
+}
+
 @end

The approach is very similar in both SDKs: implement the delegate/interface and provide a method that gets called whenever a URL needs transforming. The URL is called on the main thread.

@kkaefer
Copy link
Contributor Author

kkaefer commented Feb 17, 2017

This PR also includes a bare-bones version of #4386 for Android. Long term, we should align the iOS and Android implementations and move the ownership of DefaultFileSource to OfflineManager.

@kkaefer
Copy link
Contributor Author

kkaefer commented Feb 17, 2017

Instead of passing a Resource&&, we could also pass a const Resource&, and expect a std::string as a return value. This would reflect the fact that the url is the only thing that can be changed (changes to all other values are ignored, since their values have been used prior to this call to construct this URL). I.e. it's not meaningful to change kind, necessity, or tileData, and the user probably shouldn't change priorModified, priorExpires and priorEtag either.

Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

iOS and macOS changes look great. Thanks for all the iterations on this feature.

MGLResourceKindUnknown,
/** Style sheet JSON file */
MGLResourceKindStyle,
/** TileJSON file as specified in https://www.mapbox.com/mapbox-gl-js/style-spec/#root-sources */
Copy link
Contributor

Choose a reason for hiding this comment

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

Use an <a> tag with the text "Mapbox Style Specification", so that the documentation looks cleaner in the jazzy documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose we could alternatively refer to the TileJSON file as the file specified by the configurationURL parameter in MGLTileSource's initializers.

@return A URL that will now be downloaded.
*/
- (NSURL *)offlineStorage:(MGLOfflineStorage *)storage
URLForResourceOfKind:(MGLResourceKind)kind
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: adding the space before the asterisk caused these colons to become misaligned. 😅

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.

Looks good!

Instead of passing a Resource&&, we could also pass a const Resource&, and expect a std::string as a return value.

This seems like a good idea. The SDK bindings enforce this already, which is good.

@kkaefer kkaefer merged commit f8ab327 into master Feb 20, 2017
@kkaefer kkaefer deleted the url-transform branch February 20, 2017 13:36
Copy link
Contributor

@ivovandongen ivovandongen left a comment

Choose a reason for hiding this comment

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

LGTM. Although I'd prefer having the setResourceTransform on a separate class from OfflineManager since it's not specific to offline. But we can pick that up in: #8083

@kkaefer
Copy link
Contributor Author

kkaefer commented Feb 20, 2017

Longer term, we should follow the iOS model, which has a wrapper class for DefaultFileSource (which is essentially what OfflineManager is). On iOS, it's called MGLOfflineStorage, but it's kind of a misnomer since it's also used in online environments. A better name would've been MGLStorageManager. On Android, we could e.g. rename OfflineManager to StorageManager, and have a Map object take a reference to a StorageManager on construction.

@1ec5 1ec5 mentioned this pull request Feb 20, 2017
@1ec5
Copy link
Contributor

1ec5 commented Feb 20, 2017

On iOS, it's called MGLOfflineStorage, but it's kind of a misnomer since it's also used in online environments. A better name would've been MGLStorageManager. On Android, we could e.g. rename OfflineManager to StorageManager, and have a Map object take a reference to a StorageManager on construction.

Reopened #4250 (comment) to track the renaming discussion.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants