Large scale https upgrades #314
Conversation
|
Note Bitrise is struggling with the cpp submodule so I am regenerating the CI job to better support submodules. See https://devcenter.bitrise.io/faq/adding-projects-with-submodules/ or more information about the issue. |
subsymbolic
force-pushed
the
feature/large_scale_https_upgrades
branch
from
121f1e5 to
4741a65
Compare
* Remove old component and replace with new * Download https data, validate and persist * Add migration including deleting old records * Unit tests
Also move other 3rd party libs into correct folder
subsymbolic
force-pushed
the
feature/large_scale_https_upgrades
branch
from
4741a65 to
832c700
Compare
subsymbolic
changed the title
|
Bitrise Update: The Bitrise submodule issue was fixed by using the github https (rather than ssh) url for the submodule. |
| val md = MessageDigest.getInstance("SHA-256") | ||
| md.reset() | ||
| val digest = md.digest(this) | ||
| return String.format("%0" + digest.size * 2 + "x", BigInteger(1, digest)) |
There was a problem hiding this comment.
Is this doing more manual string concatenation than necessary? Or is there a reason why digest.size *2 isn't provided as a param?
There was a problem hiding this comment.
I'm generating the format specifier there (not the string itself). I'm open to alternatives.
| System.loadLibrary("https-bloom-lib") | ||
| } | ||
|
|
||
| constructor(maxItems: Int, targetProbability: Double) { |
There was a problem hiding this comment.
Should we mark this as @VisibleForTesting if it's not needed from production code?
There was a problem hiding this comment.
It is an appropriate part of the API for this class, matching the C++ code. I don't feel a need to hide it but am happy to if you feel strongly about it.
There was a problem hiding this comment.
Just because it's in the C++ code, does that mean we need to have it exposed in Kotlin? If yes, then happy to leave it.
But if not, it seems we only call it from testing and it might not exist otherwise, hence why I was thinking that annotation might help explain its existence.
But i don't feel strongly about it.
| return true | ||
| } | ||
| httpsBloomFilter?.let { | ||
| val shouldUpgrade = it.contains(uri.host) |
There was a problem hiding this comment.
maybe we should assign uri.host to a local variable in case it's in any way expensive to parse out the host from the Uri (and we use it twice in quick succession)
|
|
||
| override fun create(): BloomFilter? { | ||
|
|
||
| val specificationWrapper = Observable.just(dao) |
There was a problem hiding this comment.
As discussed IRL, maybe we need to get this off the main thread.
There was a problem hiding this comment.
Great feedback @CDRussell. I've made this update but used a Rx io Scheduler (not thread {}) to allow for testing
| import javax.inject.Inject | ||
|
|
||
| interface HttpsBloomFilterFactory { | ||
| fun create(): BloomFilter? |
There was a problem hiding this comment.
Related to other comment about threading, does this need to be asynchronous so work can be avoided on the main thread? or if it's to be called on a worker thread, should we annotate it as such?
There was a problem hiding this comment.
Definitely! Thanks :)
| if (response.isSuccessful) { | ||
| val whitelist = response.body()!! | ||
| Timber.d("Updating https whitelist with new data") | ||
| whitelistDao.deleteAll() |
There was a problem hiding this comment.
Should we make a method on the Dao which both deletes and inserts, so that we can mark it with @Transaction to ensure the deletion and insertion both either happen or neither happens?
| fun count(): Int | ||
| fun insert(specification: HttpsBloomFilterSpec) | ||
|
|
||
| @Query("select * from https_bloom_filter_spec limit 1") |
There was a problem hiding this comment.
This isn't a pattern we currently use in our codebase (nearly all the DAOs are lowercase). How do you feel about discussing the convention in our next FDRI meeting and then updating the code universally according to the decision we make?
There was a problem hiding this comment.
You're right; we use a mishmash of upper and lower case, even in the same query 😞
Cool, ignore it for this PR
| @Insert(onConflict = OnConflictStrategy.REPLACE) | ||
| fun insertAll(domains: List<HttpsWhitelistedDomain>) | ||
|
|
||
| @Query("select count(1) > 0 from https_whitelisted_domain where domain = :domain") |
There was a problem hiding this comment.
See my previous comment.
| - avd-manager@0.9.2: {} | ||
| - avd-manager@0.9.2: | ||
| inputs: | ||
| - version: '27' |
|
I see an issue when upgrading from The app is left in a broken state from here it seems, but restarting the app resolves the issue. It isn't clear if the migration steps succeeded or failed as a result. Updating to Thoughts @subsymbolic ?
|
| val initialTime = System.nanoTime() | ||
| val shouldUpgrade = it.contains(host) | ||
| val totalTime = System.nanoTime() - initialTime | ||
| Timber.d("${host} ${if (shouldUpgrade) "is" else "is not"} upgradable, lookup in ${totalTime/NANO_TO_MILLIS_DIVISOR}ms") |
There was a problem hiding this comment.
Is it nicer to use TimeUnit here for the conversion rather than doing it manually?
TimeUnit.NANOSECONDS.toMillis(totalTime) ?
There was a problem hiding this comment.
Sadly not. TimeUnit conversions only return whole numbers, so would always spit out zero.
| return null | ||
| } | ||
|
|
||
| val initialTimestamp = Date().time |
There was a problem hiding this comment.
Given this isn't performance-critical code path, this probably doesn't matter but....
creating a new Date object here is more overhead than grabbing the System.currentTimeMillis() directly
in fact, the constructor of Date does just that anyway
public Date() {
this(System.currentTimeMillis());
}
so we'd be better replacing Date().time with System.currentTimeMillis() I think.
This fixes room migration bug. 1.1.1 See May 16 2018 release notes (1.1.1 is identical to 1.1.1-rc1) https://developer.android.com/jetpack/docs/release-notes
…ords) of https whitelist and tracker entries
|
Thanks for the review @CDRussell! |
2 participants
Task/Issue URL: https://app.asana.com/0/72649045549333/721626625236432
Tech Design URL: https://app.asana.com/0/72649045549333/727813825116580
Description:
Add large scale https support to android
Steps to test this PR:
TEST ONE:
TEST TWO:
TEST THREE:
Internal references:
Software Engineering Expectations
Technical Design Template