-
-
Notifications
You must be signed in to change notification settings - Fork 736
Upgrade Parse Push to GCM v4 #452
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
Upgrade Parse Push to GCM v4 #452
Conversation
This change: ---------- * Moves Parse back to using public APIs (open [GitHub discussion](ParsePlatform#445)) * Cleans up a lot of code in `GcmRegistrar` that is redundant with GCM APIs or written before Bolts * Fixes a typo in manifest instructions that used a literal `bool` instead of `"bool"` * Fixes a bug where ParseInstallation did not save the GcmSenderId, causing Parse to not use the developer's secrets. * Fixes a bug where Parse incorrectly blames a manifest error when GCM is unavailable because the device doesn't have Play Services. * Add a compatibility shim that lets `ParsePushBroadcastReceiver` correctly handle the standard payloads expected by [com.android.gms.gcm.GcmReceiver](https://developers.google.com/android/reference/com/google/android/gms/gcm/GcmReceiver). This lets customers who previously used another push provider use the `ParsePushBroadcastReceiver` instead. * Add support for GCMv4, including a new optional intent to notify the app when the InstanceID is invalidated. GCM v4 has a number of benefits: --------------- * GCM v4 is based on a device-owned InstanceID. Push tokens are oauth tokens signed by the device, so this fixes double-send bugs that Parse Push has never been able to fix. * If we used the InstanceID as the ParseInstallation.InstallationId, we would also increase stability of the Installation record, which fixes some cases where Installations are wiped & replaced (related to the above bug for senderId stability). * This API has a callback in case the InstanceID is invalidated, which should reduce client/server inconsistencies. * These tokens support new server-side APIs like push-to-topic, which are _dramatically_ faster than the normal ParsePush path. * When a device upgrades to GCMv4, the device keeps GCM topics in sync with channels. This paves the way to implement push-to-channels on top of topics. It also allows the customer to keep some of their targeting info regardless of which push provider they choose to use. This has two possibly controversial requirements: ---------------- * The new API issues one token per sender ID rather than one token that works with all sender IDs. To avoid an invasive/breaking server-side change, we are _no longer requesting tokens for the Parse sender ID_ if the developer provided their own. We will also only support at most one custom sender ID. I've had a number of conversations about this and nobody seems concerned. * This change introduces a dependency on the Google Mobile Services SDK. The dependency is just the GCM .jar and does _not_ limit the Parse SDK to devices with Play Services (tested on an ICS emulator w/o Google APIs). I originally tried doing this without the dependency, but the new API has a large amount of crypto and incredible care for compat shims on older API levels. I assume my hand-crafted copy would be worse quality. Open questions ----------------- * Should Parse use the GMS InstanceID over the InstallationId when available? This makes the server-side Installation deduplication code work better, but could break systems that assume InstallationId is a UUID. * Google workflows provide a `google-services.json` file that GMS uses to auto-initialize various Google products (including GCM). Should we allow the Parse SDK to initialize the developer's sender ID with this file in addition to the Parse-specific way?
By analyzing the blame information on this pull request, we identified @grantland, @daisuke-nomura and @wangmengyan95 to be potential reviewers. |
@inlined updated the pull request. |
Good to have you looking at this code again. I haven't had a chance to look at the code yet but here are some thoughts off the top of my head:
Seems like we shouldn't do that because everywhere else installationId is a uuid.
Maybe we could avoid thinking about this entirely by having some API to turn off auto-registration and delegating the registration process to the client? Other thoughts:
Anyway I'll try to take a look sometime this week and hopefully we can get some other folks to chime in too. |
Good to be back. As I joked, with Héctor, this was basically my reaction when I refamiliarized myself with the codebase: Some quick responses:
|
@inlined updated the pull request. |
1 similar comment
@inlined updated the pull request. |
I outlined the reasons I remembered why we didn't do this in #445. I haven't looked at your code in detail, but it seems like you'll need to update the Travis-CI config to get tests to pass. |
@bnham said,
If a developer is not yet ready to migrate their API to Parse Server and they need to release an update to their app, they can continue using the same version of the Parse Android SDK that their app is already using. Is there any compelling reason to update the Parse Android SDK version in a parse.com app and not take advantage of Parse Server? The last Parse Android SDK release was on January 22, before the shutdown was announced. Have we landed any significant bug fixes to master since then? |
@hramos there have been only 2 minor PRs that have been accepted |
@inlined updated the pull request. |
Additionally, I don't think continuing to hardcode GCM in the SDK is the right way nor having a hard dependency due to backwards compatibility limitations as well as not forcing another dependency for those not utilizing GCM. |
@grantland It's impossible for me to be truly unbiased here, so I ask both as a Parse contributor and a Google employee who wants to understand Android developers' mindsets: is this a philosophic or pragmatic concern? The GCM library isn't a GMS monolith anymore; the jar has 42 classes and is 62KB. I often hear 10MB as the magic number to stay under in emerging markets, so this is 0.6% of the file limit and 0.06% of the DEX limit. Other splits in the Parse SDK made sense. We split out Parse-UI because it made more sense for Parse-UI to be an open source template than to expose a knob for every design customization a developer might want. We split out Parse-Social plugins because social JARs were less likely used and were pretty bulky. The latest Facebook SDK is 883KB and 502 classes for the core .aar plus an additional 225KB and 256 classes for the Audience Network (totaling 10% of the file quota and 1.2% of the DEX quota), so I understand why it should be optional. GCM is both more fundamental to most modern apps and has dramatically less requirements. The backwards compatibility concerns may be valid, as long as we're talking about Parse backwards compatibility and not Android compatibility; the GCM v4 libraries are fully compatible with older versions of GMS |
DEX limit actually limits the number of methods, not classes, and has 594 methods which is ~%1 of the DEX limit. This also accounts for the library as it is now, but not any changes in the future which can go up or down. WRT backwards compatibility, I'm mainly talking about Parse SDK backwards compatibility. Would you or @bnham know if there's a set of devices that would be come unsupported by moving to GCM lib as opposed to our solution? I can't remember if there was something around that or not. |
Ah good catch RE the DEX; not sure why I was counting classes. Sorry about that. It looks like the client library requires API level 9, the same level as Parse. The GMS client library gracefully handles a device not having the Google Play Services installed. I verified this on an ICS emulator w/o play services, which is how I discovered one the bugs I fixed in ManifestInfo.java |
# The first commit's message is: Fix .travis.yml # The 2nd commit message will be skipped: # Add missing dependencies
67a8e12
to
b6937df
Compare
@inlined updated the pull request. |
Unfortunately we won't be accepting this change as is since adding a hard dependency on GCM has side affects such as breaking backwards compatibility for users using raw JARs and increasing the APK size and method count developers who do not use GCM. With this in mind, adding support for the latest GCM library would be beneficial and we would gladly accept a PR that does not require a hard dependency on the GCM library. |
Sounds good. I/O is tomorrow, so I have to be heads down prepping on my talk. We might need to collaborate on the best solution. Locking Parse forever on v3 doesn't feel like the best idea and v4+ is based on client-generated keys. We'll either need to depend on undocumented locations for where these keys are stored, or the app could end up in some weird split-brained mode if Parse generates one key pair and the official library generates another. Thanks for taking the time to do the review & helping me understand where everyone is coming from. To help me better understand everyone's stance:
|
Now that I/O is done, we can have a more candid conversation about where Google is going/has gone with FCM. At very least, we can apply the manifest fixes and add the additional payload types for the GcmPushService. I think these will go further to at least make sure Parse doesn't interfere with a customer trying to use FCM. |
@inlined yeah that sounds like a good stopgap for now, thanks! |
Does v4 not support multiple sender IDs? The bigger question would be how many users are using more than one sender ID (excluding the Parse sender ID). Unfortunately it's not a number I can pull up.
Could you elaborate on this? I'm not too familiar with that Intent structure and don't have anything against it, I just don't want to enforce a hard dependency.
This could be useful, but I'm not entirely sure if the tradeoff is worth it. It might be more worthwhile to just switch to topics if that's more reliable/efficient. I'm not even sure if |
v4 supports multiple sender IDs, but it generates a different token for each sender ID. The Parse infrastructure makes really deep assumptions that the token works with every sender ID, so it'd be a more invasive server-side change to fix that. I can't imagine this change getting applied to parse.com so... ¯_(ツ)_/¯ I know you don't have stats on how many people are actually using 3rd party credentials (esp since the Parse SDK doesn't track it without this bugfix), but there is info on the number of GCM credentials each app has attached to it. You use a query to group gcm creds by parse app id, count the records per group, select by count > 1, and see if that returns any results. If not, nobody has ever done any registration that would allow them to use the feature we're talking about removing.
Sure, these are the changes in GCMService.java. Parse creates a local intent where the notification payloads are in a map that includes alert, title, etc. The Google way puts these in different extras in the intent bundle. Supporting both doesn't require compile-time dependencies, just new magic strings.
ParseServer supports push notifications as an extension. Eventually you probably want to use topics over channels where possible, but you'll need the legacy code for devices which use an SDK before topic support and for push-to-query with a clause that can't be articulated with FCM. This is also a feature that would require compile-time dependencies on FCM. I could put this type of feature in mygration |
@hermanliang @natario1 @Jawnnypoo do any of you have bandwidth to figure out what we should do to get to GCM v4? |
At this point, it would probably be best to focus on getting it toward FCM instead, since GCM is not going to get new features https://developers.google.com/cloud-messaging/faq |
I know nothing about it, but it should be doable.. What seems not easy is doing proper testing after. Requires test server, firebase project, firebase keys, test app, possibly more than one device, and many things can go wrong. Sad this PR was not considered. |
This PR was honestly not the right long-term approach. I couldn't warn anyone that FCM was coming and wanted to get Parse/ParseServer upgraded with the wire formats necessary to work automagically on day one. The new Firebase SDKs though make this much easier to do non-invasively. To build this, I would consider ripping out the PPNS code if it hasn't been already. You then would just make the ParsePushBroadcastReceiver also listen to the (probably undocumented but not very complex) FCM intents and handle their payloads (including new push IDs as well as push payloads). The only non-standard thing you'd advise Parse developers is to not add the FCM broadcast receiver (though admittedly I'm not sure if that's broken now that manifest fragments are automatically injected). If this all works, Parse could integrate well with the Firebase SDK without actually depending on it or GMS Core. Regarding testing, CI across ParseServer and the Android SDK may be a PITA, but I can at least give you some tips that would help with manual testing during development at least. Firebase will give you a generous free tier in Cloud Functions for Firebase that will not only support the server-side execution but also automatically configure for the current project. Just have your index.js say const admin = require('firebase-admin');
const functions = require('firebase-functions');
admin.initializeApp(functions.config().firebase); from here you can use Since Cloud Functions for Firebase has the explicit design goal of never requiring any project-specific magic strings, you could theoretically even check this in as a systems test that can be called in CI if the CI has the right config variables. (Disclaimer: I was the former tech lead for Parse Push and am the current tech lead for Cloud Functions for Firebase) |
Thanks @inlined for following up! I'm pretty sure we'll be able to cook something up! |
This change:
GcmRegistrar
that is redundant with GCM APIs or written before Boltsbool
instead of"bool"
ParsePushBroadcastReceiver
correctly handle the standard payloads expected by com.android.gms.gcm.GcmReceiver. This lets customers who previously used another push provider use theParsePushBroadcastReceiver
instead.GCM v4 has a number of benefits:
This has two possibly controversial requirements:
Open questions
google-services.json
file that GMS uses to auto-initialize various Google products (including GCM). Should we allow the Parse SDK to initialize the developer's sender ID with this file in addition to the Parse-specific way?