android: more code review suggestions

updates ENG-2084

Suggestions for PR #179

Signed-off-by: Percy Wegmann <percy@tailscale.com>
oxtoacart/android-nextgen-infra
Percy Wegmann 9 months ago committed by Jonathan Nobels
parent 5204448f1e
commit d5ad4b31d1

1
.gitignore vendored

@ -32,3 +32,4 @@ tailscale.jks
#IDE
.vscode
.idea

@ -6,6 +6,7 @@ DEBUG_APK=tailscale-debug.apk
RELEASE_AAB=tailscale-release.aab
APPID=com.tailscale.ipn
AAR=android_legacy/libs/ipn.aar
AAR_NEXTGEN=android/libs/ipn.aar
KEYSTORE=tailscale.jks
KEYSTORE_ALIAS=tailscale
TAILSCALE_VERSION=$(shell ./version/tailscale-version.sh 200)
@ -129,14 +130,18 @@ androidpath:
toolchain: $(TOOLCHAINDIR)/bin/go
android/libs:
mkdir -p android_legacy/libs
$(AAR): toolchain checkandroidsdk android/libs
$(AAR): toolchain checkandroidsdk
@mkdir -p android_legacy/libs && \
go run gioui.org/cmd/gogio \
-ldflags "-X tailscale.com/version.longStamp=$(VERSIONNAME) -X tailscale.com/version.shortStamp=$(VERSIONNAME_SHORT) -X tailscale.com/version.gitCommitStamp=$(TAILSCALE_COMMIT) -X tailscale.com/version.extraGitCommitStamp=$(OUR_VERSION)" \
-buildmode archive -target android -appid $(APPID) -tags novulkan,tailscale_go -o $@ github.com/tailscale/tailscale-android/cmd/tailscale
$(AAR_NEXTGEN): $(AAR)
@mkdir -p android/libs && \
cp $(AAR) $(AAR_NEXTGEN)
lib: $(AAR_NEXTGEN)
# tailscale-debug.apk builds a debuggable APK with the Google Play SDK.
$(DEBUG_APK): $(AAR)
(cd android_legacy && ./gradlew test assemblePlayDebug)
@ -180,4 +185,4 @@ clean:
-rm -rf android_legacy/build $(DEBUG_APK) $(RELEASE_AAB) $(AAR) tailscale-fdroid.apk
-pkill -f gradle
.PHONY: all clean install android_legacy/lib $(DEBUG_APK) $(RELEASE_AAB) $(AAR) release bump_version dockershell
.PHONY: all clean install android_legacy/lib $(DEBUG_APK) $(RELEASE_AAB) $(AAR) release bump_version dockershell lib

@ -5,23 +5,22 @@
package com.tailscale.ipn.ui.localapi
import android.util.Log
import com.tailscale.ipn.ui.model.*
import com.tailscale.ipn.ui.model.BugReportID
import com.tailscale.ipn.ui.model.Ipn
import com.tailscale.ipn.ui.model.IpnLocal
import com.tailscale.ipn.ui.model.IpnState
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.flow.StateFlow
import kotlinx.coroutines.flow.first
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.launch
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.Job
import java.util.concurrent.ConcurrentHashMap
typealias StatusResponseHandler = (Result<IpnState.Status>) -> Unit
typealias BugReportIdHandler = (Result<BugReportID>) -> Unit
typealias PrefsHandler = (Result<Ipn.Prefs>) -> Unit
class LocalApiClient {
constructor() {
Log.d("LocalApiClient", "LocalApiClient created")
}
class LocalApiClient(private val scope: CoroutineScope) {
companion object {
private val _isReady = MutableStateFlow(false)
@ -29,6 +28,7 @@ class LocalApiClient {
// Called by the backend when the localAPI is ready to accept requests.
@JvmStatic
@Suppress("unused")
fun onReady() {
_isReady.value = true
Log.d("LocalApiClient", "LocalApiClient is ready")
@ -46,73 +46,57 @@ class LocalApiClient {
// body: The body of the request.
// cookie: A unique identifier for this request. This is used map responses to
// the corresponding request. Cookies must be unique for each request.
external fun doRequest(request: String, method: String, body: String, cookie: String)
private external fun doRequest(request: String, method: String, body: ByteArray?, cookie: String)
protected val scope = CoroutineScope(Dispatchers.IO + Job())
fun <T> executeRequest(request: LocalAPIRequest<T>) {
private fun <T> executeRequest(request: LocalAPIRequest<T>) {
scope.launch {
isReady.first { it == true }
isReady.first { it }
Log.d("LocalApiClient", "Executing request:${request.method}:${request.path}")
addRequest(request)
// The jni handler will treat the empty string in the body as null.
val body = request.body ?: ""
doRequest(request.path, request.method, body, request.cookie)
requests[request.cookie] = request
doRequest(request.path, request.method, request.body, request.cookie)
}
}
// This is called from the JNI layer to publish localAPIResponses. This should execute on the
// same thread that called doRequest.
fun onResponse(response: String, cookie: String) {
val request = requests[cookie]
request?.let {
@Suppress("unused")
fun onResponse(response: ByteArray, cookie: String) {
requests.remove(cookie)?.let { request ->
Log.d("LocalApiClient", "Reponse for request:${request.path} cookie:${request.cookie}")
// The response handler will invoked internally by the request parser
request.parser(response)
removeRequest(cookie)
}
?: { Log.e("LocalApiClient", "Received response for unknown request: ${cookie}") }
} ?: { Log.e("LocalApiClient", "Received response for unknown request: $cookie") }
}
// Tracks in-flight requests and their callback handlers by cookie. This should
// always be manipulated via the addRequest and removeRequest methods.
private var requests = HashMap<String, LocalAPIRequest<*>>()
private var requestLock = Any()
fun addRequest(request: LocalAPIRequest<*>) {
synchronized(requestLock) { requests[request.cookie] = request }
}
fun removeRequest(cookie: String) {
synchronized(requestLock) { requests.remove(cookie) }
}
private var requests = ConcurrentHashMap<String, LocalAPIRequest<*>>()
// localapi Invocations
fun getStatus(responseHandler: StatusResponseHandler) {
val req = LocalAPIRequest.status(responseHandler)
executeRequest<IpnState.Status>(req)
executeRequest(req)
}
fun getBugReportId(responseHandler: BugReportIdHandler) {
val req = LocalAPIRequest.bugReportId(responseHandler)
executeRequest<BugReportID>(req)
executeRequest(req)
}
fun getPrefs(responseHandler: PrefsHandler) {
val req = LocalAPIRequest.prefs(responseHandler)
executeRequest<Ipn.Prefs>(req)
executeRequest(req)
}
fun getProfiles(responseHandler: (Result<List<IpnLocal.LoginProfile>>) -> Unit) {
val req = LocalAPIRequest.profiles(responseHandler)
executeRequest<List<IpnLocal.LoginProfile>>(req)
executeRequest(req)
}
fun getCurrentProfile(responseHandler: (Result<IpnLocal.LoginProfile>) -> Unit) {
val req = LocalAPIRequest.currentProfile(responseHandler)
executeRequest<IpnLocal.LoginProfile>(req)
executeRequest(req)
}
// (jonathan) TODO: A (likely) exhaustive list of localapi endpoints required for
@ -141,5 +125,7 @@ class LocalApiClient {
// verifyDeepling
// ping
// setTailFSFileServerAddress
init {
Log.d("LocalApiClient", "LocalApiClient created")
}
}

@ -4,9 +4,15 @@
package com.tailscale.ipn.ui.localapi
import com.tailscale.ipn.ui.model.*
import kotlinx.serialization.decodeFromString
import com.tailscale.ipn.ui.model.BugReportID
import com.tailscale.ipn.ui.model.Errors
import com.tailscale.ipn.ui.model.Ipn
import com.tailscale.ipn.ui.model.IpnLocal
import com.tailscale.ipn.ui.model.IpnState
import kotlinx.serialization.ExperimentalSerializationApi
import kotlinx.serialization.json.Json
import kotlinx.serialization.json.decodeFromStream
import java.util.UUID
private object Endpoint {
const val DEBUG = "debug"
@ -41,9 +47,8 @@ private object Endpoint {
//
// (jonathan) TODO: Audit local API for all of the possible error results and clean
// it up if possible.
enum class APIErrorVals(val rawValue: String) {
UNPARSEABLE_RESPONSE("Unparseable localAPI response"),
NOT_READY("Not Ready");
enum class APIErrorVals(private val rawValue: String) {
UNPARSEABLE_RESPONSE("Unparseable localAPI response"), NOT_READY("Not Ready");
fun toError(): Error {
return Error(rawValue)
@ -51,49 +56,32 @@ enum class APIErrorVals(val rawValue: String) {
}
class LocalAPIRequest<T>(
path: String,
val method: String,
val body: String? = null,
val parser: (String) -> Unit,
path: String,
val method: String,
val body: ByteArray? = null,
val parser: (ByteArray) -> Unit,
) {
val path = "/localapi/v0/$path"
val cookie = UUID.randomUUID().toString()
companion object {
val cookieLock = Any()
var cookieCounter: Int = 0
val decoder = Json { ignoreUnknownKeys = true }
fun <T> get(path: String, body: String? = null, parser: (String) -> Unit) =
fun <T> get(path: String, body: ByteArray? = null, parser: (ByteArray) -> Unit) =
LocalAPIRequest<T>(
method = "GET",
path = path,
body = body,
parser = parser
method = "GET", path = path, body = body, parser = parser
)
fun <T> put(path: String, body: String? = null, parser: (String) -> Unit) =
fun <T> put(path: String, body: ByteArray? = null, parser: (ByteArray) -> Unit) =
LocalAPIRequest<T>(
method = "PUT",
path = path,
body = body,
parser = parser
method = "PUT", path = path, body = body, parser = parser
)
fun <T> post(path: String, body: String? = null, parser: (String) -> Unit) =
private fun <T> post(path: String, body: ByteArray? = null, parser: (ByteArray) -> Unit) =
LocalAPIRequest<T>(
method = "POST",
path = path,
body = body,
parser = parser
method = "POST", path = path, body = body, parser = parser
)
fun getCookie(): String {
synchronized(cookieLock) {
cookieCounter += 1
return cookieCounter.toString()
}
}
fun status(responseHandler: StatusResponseHandler): LocalAPIRequest<IpnState.Status> {
return get(Endpoint.STATUS) { resp ->
responseHandler(decode<IpnState.Status>(resp))
@ -125,33 +113,33 @@ class LocalAPIRequest<T>(
}
// Check if the response was a generic error
fun parseError(respData: String): Error {
try {
val err = Json.decodeFromString<Errors.GenericError>(respData)
return Error(err.error)
@OptIn(ExperimentalSerializationApi::class)
fun parseError(respData: ByteArray): Error {
return try {
val err = Json.decodeFromStream<Errors.GenericError>(respData.inputStream())
Error(err.error)
} catch (e: Exception) {
return Error(APIErrorVals.UNPARSEABLE_RESPONSE.toError())
Error(APIErrorVals.UNPARSEABLE_RESPONSE.toError())
}
}
// Handles responses that are raw strings. Returns an error result if the string
// is empty
fun parseString(respData: String): Result<String> {
return if (respData.length > 0) Result(respData)
private fun parseString(respData: ByteArray): Result<String> {
return if (respData.isNotEmpty()) Result(respData.decodeToString())
else Result(APIErrorVals.UNPARSEABLE_RESPONSE.toError())
}
// Attempt to decode the response into the expected type. If that fails, then try
// parsing as an error.
inline fun <reified T> decode(respData: String): Result<T> {
try {
val message = decoder.decodeFromString<T>(respData)
return Result(message)
@OptIn(ExperimentalSerializationApi::class)
private inline fun <reified T> decode(respData: ByteArray): Result<T> {
return try {
val message = decoder.decodeFromStream<T>(respData.inputStream())
Result(message)
} catch (e: Exception) {
return Result(parseError(respData))
Result(parseError(respData))
}
}
}
val cookie: String = getCookie()
}

@ -4,27 +4,26 @@
package com.tailscale.ipn.ui.service
import android.util.Log
import com.tailscale.ipn.ui.localapi.LocalApiClient
import com.tailscale.ipn.ui.notifier.Notifier
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.SupervisorJob
import kotlinx.coroutines.cancel
class IpnManager {
var notifier = Notifier()
var apiClient = LocalApiClient()
val model: IpnModel
constructor() {
model = IpnModel(notifier, apiClient)
}
private var notifier = Notifier()
private var scope = CoroutineScope(SupervisorJob() + Dispatchers.Main)
private var apiClient = LocalApiClient(scope)
private val model = IpnModel(notifier, apiClient, scope)
// We share a single instance of the IPNManager across the entire application.
companion object {
@Volatile
private var instance: IpnManager? = null
fun getInstance() =
instance ?: synchronized(this) {
instance ?: IpnManager().also { instance = it }
}
fun getInstance() = instance ?: synchronized(this) {
instance ?: IpnManager().also { instance = it }
}
}
}

@ -6,31 +6,22 @@ package com.tailscale.ipn.ui.service
import android.util.Log
import com.tailscale.ipn.ui.localapi.LocalApiClient
import com.tailscale.ipn.ui.model.*
import com.tailscale.ipn.ui.model.Ipn
import com.tailscale.ipn.ui.model.IpnLocal
import com.tailscale.ipn.ui.model.Netmap
import com.tailscale.ipn.ui.notifier.Notifier
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.Job
import kotlinx.coroutines.launch
import kotlinx.coroutines.flow.StateFlow
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.flow.StateFlow
import kotlinx.coroutines.flow.first
import kotlinx.coroutines.launch
class IpnModel {
protected val scope = CoroutineScope(Dispatchers.Default + Job())
var notifierSessions: MutableList<String> = mutableListOf()
val apiClient: LocalApiClient
constructor(notifier: Notifier, apiClient: LocalApiClient) {
Log.d("IpnModel", "IpnModel created")
this.apiClient = apiClient
val session = notifier.watchAll { n -> onNotifyChange(n) }
notifierSessions.add(session)
scope.launch { loadUserProfiles() }
}
class IpnModel(
notifier: Notifier,
private val apiClient: LocalApiClient,
scope: CoroutineScope
) {
private var notifierSessions: MutableList<String> = mutableListOf()
private val _state: MutableStateFlow<Ipn.State?> = MutableStateFlow(null)
private val _netmap: MutableStateFlow<Netmap.NetworkMap?> = MutableStateFlow(null)
@ -43,7 +34,7 @@ class IpnModel {
private val _loggedInUser: MutableStateFlow<IpnLocal.LoginProfile?> = MutableStateFlow(null)
private val _loginProfiles: MutableStateFlow<List<IpnLocal.LoginProfile>?> =
MutableStateFlow(null)
MutableStateFlow(null)
val state: StateFlow<Ipn.State?> = _state
@ -57,7 +48,7 @@ class IpnModel {
val loggedInUser: StateFlow<IpnLocal.LoginProfile?> = _loggedInUser
val loginProfiles: StateFlow<List<IpnLocal.LoginProfile>?> = _loginProfiles
var isUsingExitNode: Boolean = false
val isUsingExitNode: Boolean
get() {
return prefs.value != null
}
@ -66,16 +57,16 @@ class IpnModel {
// Backend Observation
private suspend fun loadUserProfiles() {
LocalApiClient.isReady.first { it == true }
LocalApiClient.isReady.first { it }
apiClient.getProfiles { result ->
result.success?.let { users -> _loginProfiles.value = users }
?: run { Log.e("IpnManager", "Error loading profiles: ${result.error}") }
?: run { Log.e("IpnManager", "Error loading profiles: ${result.error}") }
}
apiClient.getCurrentProfile { result ->
result.success?.let { user -> _loggedInUser.value = user }
?: run { Log.e("IpnManager", "Error loading profiles: ${result.error}") }
?: run { Log.e("IpnManager", "Error loading profiles: ${result.error}") }
}
}
@ -97,4 +88,11 @@ class IpnModel {
notify.Version?.let { version -> _version.value = version }
}
init {
Log.d("IpnModel", "IpnModel created")
val session = notifier.watchAll { n -> onNotifyChange(n) }
notifierSessions.add(session)
scope.launch { loadUserProfiles() }
}
}

@ -5,11 +5,11 @@
package localapiservice
import (
"bytes"
"context"
"encoding/json"
"io"
"log"
"strings"
"time"
"unsafe"
@ -45,7 +45,7 @@ func Java_com_tailscale_ipn_ui_localapi_LocalApiClient_doRequest(
cls C.jclass,
jpath C.jstring,
jmethod C.jstring,
jbody C.jstring,
jbody C.jbyteArray,
jcookie C.jstring) {
jenv := (*jni.Env)(unsafe.Pointer(env))
@ -62,20 +62,20 @@ func Java_com_tailscale_ipn_ui_localapi_LocalApiClient_doRequest(
// The body string. This is optional and may be empty.
bodyRef := jni.NewGlobalRef(jenv, jni.Object(jbody))
bodyStr := jni.GoString(jenv, jni.String(bodyRef))
bodyArray := jni.GetByteArrayElements(jenv, jni.ByteArray(bodyRef))
defer jni.DeleteGlobalRef(jenv, bodyRef)
resp := doLocalAPIRequest(pathStr, methodStr, bodyStr)
resp := doLocalAPIRequest(pathStr, methodStr, bodyArray)
jrespBody := jni.JavaString(jenv, resp)
respBody := jni.Value(jrespBody)
cookie := jni.Value(jcookie)
onResponse := jni.GetMethodID(jenv, shim.clientClass, "onResponse", "(Ljava/lang/String;Ljava/lang/String;)V")
onResponse := jni.GetMethodID(jenv, shim.clientClass, "onResponse", "(Lbyte[];Ljava/lang/String;)V")
jni.CallVoidMethod(jenv, jni.Object(cls), onResponse, respBody, cookie)
}
func doLocalAPIRequest(path string, method string, body string) string {
func doLocalAPIRequest(path string, method string, body []byte) string {
if shim.service == nil {
return "{\"error\":\"Not Ready\"}"
}
@ -84,7 +84,7 @@ func doLocalAPIRequest(path string, method string, body string) string {
defer cancel()
var reader io.Reader = nil
if len(body) > 0 {
reader = strings.NewReader(body)
reader = bytes.NewReader(body)
}
r, err := shim.service.Call(ctx, method, path, reader)

Loading…
Cancel
Save