Fix missing token bug for App Events #1670
Conversation
ptxmac
requested review from
jingping2015,
joesus,
kalendae and
robtimp
as code owners
ptxmac
force-pushed
the
ptx/correct-token
branch
from
4e056b3
to
f8b34a3
Compare
There was a problem hiding this comment.
@joesus has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@ptxmac Thanks for the contribution! I looked at this a bit and I think it's too risky to merge. Basically the situation is this. One of the main reasons to override the app identifier for logging is to enable analysis of a how an app is performing in different regions/countries. A single application can be tied in this way to multiple Facebook App IDs. App IDs and Client Tokens are a strict 1:1 relationship. Making a call with a mismatched App ID / Client Token pair will result in an exception. The risk this change presents is that a developer may not be aware of this relationship. They'll override the App ID for logging at runtime and not realize that they also needed to change the Client Token. This would be really bad. I think a better (though more involved) change would be to make it impossible for this bug to exist by disallowing setting the logging override in isolation. This would involve deprecating |
|
Happy to review any future contributions!!! Feedback is very welcome. :D |
|
Made some changes that handled the disastrous edge case so seemed safe to merge with those. Still happy to review any followup work that actually solves the structural issue outlined above. :D |
Summary: Thanks for proposing a pull request! To help us review the request, please complete the following: - [x] sign [contributor license agreement](https://developers.facebook.com/opensource/cla) - [x] I've ensured that all existing tests pass and added tests (when/where necessary) - [x] I've updated the documentation (when/where necessary) and [Changelog](CHANGELOG.md) (when/where necessary) ## Pull Request Details App Events utils had bug where it wouldn't provide the `app|client` token unless a token was partially specified either as input or in the settings. Pull Request resolved: #1670 Test Plan: I've added a test to verify that `tokenStringToUseFor` returns the correct token Reviewed By: KylinChang Differential Revision: D26876326 Pulled By: joesus fbshipit-source-id: 2cfd7330ad596d008f7dd82c27304c0acb719787
Thanks for proposing a pull request!
To help us review the request, please complete the following:
Pull Request Details
App Events utils had bug where it wouldn't provide the
app|clienttoken unless a token was partially specified either as input or in the settings.Test Plan
I've added a test to verify that
tokenStringToUseForreturns the correct token