-
Notifications
You must be signed in to change notification settings - Fork 6
feat: dynamic geoip db support #940
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
base: main
Are you sure you want to change the base?
Conversation
composeApp/src/desktopMain/kotlin/org/ooni/engine/DesktopOonimkallBridge.kt
Outdated
Show resolved
Hide resolved
composeApp/src/androidMain/kotlin/org/ooni/probe/net/Http.android.kt
Outdated
Show resolved
Hide resolved
composeApp/src/desktopMain/kotlin/org/ooni/probe/net/Http.desktop.kt
Outdated
Show resolved
Hide resolved
| val ip: String?, | ||
| val asn: String?, | ||
| val countryCode: String?, | ||
| val geoIpdb: String?, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to get the geoIpDb from the engine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To determine which version of geoIPDB the engine is currently working with.
composeApp/src/commonMain/kotlin/org/ooni/engine/models/EnginePreferences.kt
Outdated
Show resolved
Hide resolved
composeApp/src/commonMain/kotlin/org/ooni/probe/domain/GetEnginePreferences.kt
Outdated
Show resolved
Hide resolved
composeApp/src/commonMain/kotlin/org/ooni/probe/di/Dependencies.kt
Outdated
Show resolved
Hide resolved
af697b7 to
eef3dbc
Compare
eef3dbc to
b5322c8
Compare
geoipdb from path
sdsantos
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some other issues:
a) We're not cleaning old version of the geoIp database after we download an update
b) GeoIP databases will count for storage usaged, but they are not cleared when the clean storage (maybe this is what we want, but just wanted to make sure)
c) The fetch is running on app start, and therefore on any instrumented test we have. It should be disabled for most tests I imagine.
composeApp/src/androidMain/kotlin/org/ooni/probe/net/Http.android.kt
Outdated
Show resolved
Hide resolved
composeApp/src/androidMain/kotlin/org/ooni/probe/net/Http.android.kt
Outdated
Show resolved
Hide resolved
| const val GEOIP_DB_VERSION_DEFAULT: String = "20250801" | ||
| const val GEOIP_DB_REPO: String = "aanorbel/oomplt-mmdb" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These constants could be private.
composeApp/src/commonMain/kotlin/org/ooni/probe/domain/FetchGeoIpDbUpdates.kt
Outdated
Show resolved
Hide resolved
| private fun buildGeoIpDbUrl(version: String): String = | ||
| "https://github.com/${GEOIP_DB_REPO}/releases/download/$version/$version-ip2country_as.mmdb" | ||
|
|
||
| private fun normalize(tag: String): Int = tag.removePrefix("v").trim().toInt() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're trusting that the tag will always be an Int, otherwise the app crashes. I think we should fail silently instead if that's the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And why do we need to remove the prefix, if we're the ones in control of the repository? Right now, if there's a v in the tag there maybe be some bugs, for example, will the v also be present here:
"https://github.com/${GEOIP_DB_REPO}/releases/download/$version/$version-ip2country_as.mmdb"?
composeApp/src/commonMain/kotlin/org/ooni/probe/domain/FetchGeoIpDbUpdates.kt
Show resolved
Hide resolved
composeApp/src/commonMain/kotlin/org/ooni/probe/domain/FetchGeoIpDbUpdates.kt
Outdated
Show resolved
Hide resolved
composeApp/src/commonMain/kotlin/org/ooni/probe/domain/FetchGeoIpDbUpdates.kt
Outdated
Show resolved
Hide resolved
| return Success(null) | ||
| } else { | ||
| val url = buildGeoIpDbUrl(latestVersion) | ||
| val target = "$cacheDir/$latestVersion.mmdb" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the cacheDir the place we really want for these files? Is some systems, cache directories can be deleted periodically.
b1d98df to
305a640
Compare
| ) { | ||
| companion object { | ||
| private const val GEOIP_DB_VERSION_DEFAULT: String = "20250801" | ||
| private const val GEOIP_DB_REPO: String = "aanorbel/oomplt-mmdb" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now can we move this to a proper repository under the OONI account?
| val currentTimeMillis = Clock.System.now().toEpochMilliseconds() | ||
| val oneDayInMillis = 24 * 60 * 60 * 1000L | ||
| val timeSinceLastCheck = currentTimeMillis - lastCheckMillis | ||
|
|
||
| if (timeSinceLastCheck < oneDayInMillis) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can take advantage of kotlin.time.Duration to make this more clear:
if (Clock.System.now() - Instant.fromEpochMilliseconds(lastCheckMillis) < 1.days) {
}
|
|
||
| suspend operator fun invoke(): Result<Path?, MkException> { | ||
| // Check if we've already checked today | ||
| val lastCheckMillis = preferencesRepository.getValueByKey(SettingsKey.MMDB_LAST_CHECK).first() as? Long |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we have the check, we can prevent this check from happening in instrumented tests, like I have here with disableRefreshArticles():
Line 16 in 6b3ab71
| suspend fun disableRefreshArticles() { |
Uh oh!
There was an error while loading. Please reload this page.