[client, android] Type the split tunnelling mode instead of storing a string (#7387)

gomobile carries only basic types, so the typed constants stay unexported and
the exported SplitTunnelMode* ints are what the Java side gets.

client/android/split_tunnel.go

@@ -1,17 +1,29 @@
 package android
 
-// Split tunnelling modes, stored as strings so an unknown value written by a
+// SplitTunnelMode is which of the two selections, if either, the tunnel applies.
-// newer build degrades to "off" rather than to some other mode's behaviour.
+// Its values land in the profile's stored preferences, so the constants below
+// are append-only and must never be reordered.
+type SplitTunnelMode int
+
 const (
-	SplitTunnelModeOff     = "off"
+	modeOff SplitTunnelMode = iota
-	SplitTunnelModeExclude = "exclude"
+	modeExclude
-	SplitTunnelModeInclude = "include"
+	modeInclude
+)
+
+// The same modes as basic ints. gomobile drops a constant whose type is not a
+// basic one, so these are what reaches the generated Java bindings, and they
+// keep the Android side tied to the values above instead of repeating 0, 1, 2.
+const (
+	SplitTunnelModeOff     = int(modeOff)
+	SplitTunnelModeExclude = int(modeExclude)
+	SplitTunnelModeInclude = int(modeInclude)
 )
 
 type splitTunnelSection struct {
-	Mode     string   `json:"mode"`
+	Mode     SplitTunnelMode `json:"mode"`
-	Excluded []string `json:"excluded"`
+	Excluded []string        `json:"excluded"`
-	Included []string `json:"included"`
+	Included []string        `json:"included"`
 }
 
 // PackageList wraps []string for gomobile compatibility.
@@ -49,8 +61,12 @@ func (l *PackageList) Get(i int) string {
 // carries. The two selections are kept apart because the platform applies one
 // or the other and never both, and so that switching mode does not throw away
 // the picks made in the other one.
+//
+// Mode is an int rather than a SplitTunnelMode because gomobile carries only
+// basic types across the binding. It holds one of the SplitTunnelMode*
+// constants.
 type SplitTunnelSettings struct {
-	Mode     string
+	Mode     int
 	Excluded *PackageList
 	Included *PackageList
 }
@@ -73,18 +89,21 @@ func packagesOf(list *PackageList) []string {
 	return out
 }
 
-func normalizeSplitTunnelMode(mode string) string {
+// normalizeSplitTunnelMode maps anything outside the known set to off, so a mode
+// written by a newer build degrades to carrying every application rather than to
+// some other mode's behaviour.
+func normalizeSplitTunnelMode(mode SplitTunnelMode) SplitTunnelMode {
 	switch mode {
-	case SplitTunnelModeExclude, SplitTunnelModeInclude:
+	case modeExclude, modeInclude:
 		return mode
 	default:
-		return SplitTunnelModeOff
+		return modeOff
 	}
 }
 
 func settingsFromSection(section splitTunnelSection) *SplitTunnelSettings {
 	out := NewSplitTunnelSettings()
-	out.Mode = normalizeSplitTunnelMode(section.Mode)
+	out.Mode = int(normalizeSplitTunnelMode(section.Mode))
 	for _, pkg := range section.Excluded {
 		out.Excluded.Add(pkg)
 	}
@@ -99,7 +118,7 @@ func sectionFromSettings(settings *SplitTunnelSettings) splitTunnelSection {
 		settings = NewSplitTunnelSettings()
 	}
 	return splitTunnelSection{
-		Mode:     normalizeSplitTunnelMode(settings.Mode),
+		Mode:     normalizeSplitTunnelMode(SplitTunnelMode(settings.Mode)),
 		Excluded: packagesOf(settings.Excluded),
 		Included: packagesOf(settings.Included),
 	}

client/android/split_tunnel_test.go

@@ -1,6 +1,7 @@
 package android
 
 import (
+	"encoding/json"
 	"reflect"
 	"testing"
 )
@@ -8,34 +9,48 @@ import (
 func TestNormalizeSplitTunnelMode(t *testing.T) {
 	tests := []struct {
 		name string
-		mode string
+		mode SplitTunnelMode
-		want string
+		want SplitTunnelMode
 	}{
-		{name: "exclude is kept", mode: SplitTunnelModeExclude, want: SplitTunnelModeExclude},
+		{name: "exclude is kept", mode: modeExclude, want: modeExclude},
-		{name: "include is kept", mode: SplitTunnelModeInclude, want: SplitTunnelModeInclude},
+		{name: "include is kept", mode: modeInclude, want: modeInclude},
-		{name: "off is kept", mode: SplitTunnelModeOff, want: SplitTunnelModeOff},
+		{name: "off is kept", mode: modeOff, want: modeOff},
-		{name: "empty falls back to off", mode: "", want: SplitTunnelModeOff},
+		{name: "a mode from a newer build falls back to off", mode: SplitTunnelMode(7), want: modeOff},
-		{name: "a mode from a newer build falls back to off", mode: "only-work-apps", want: SplitTunnelModeOff},
+		{name: "a negative mode falls back to off", mode: SplitTunnelMode(-1), want: modeOff},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			if got := normalizeSplitTunnelMode(tt.mode); got != tt.want {
-				t.Errorf("normalizeSplitTunnelMode(%q) = %q, want %q", tt.mode, got, tt.want)
+				t.Errorf("normalizeSplitTunnelMode(%d) = %d, want %d", tt.mode, got, tt.want)
 			}
 		})
 	}
 }
 
+// The constants the Android side reads must stay the values the store writes:
+// gomobile carries the ints below, not the typed constants they mirror.
+func TestSplitTunnelModeConstantsMirrorTheTypedOnes(t *testing.T) {
+	if SplitTunnelModeOff != int(modeOff) {
+		t.Errorf("off = %d, want %d", SplitTunnelModeOff, modeOff)
+	}
+	if SplitTunnelModeExclude != int(modeExclude) {
+		t.Errorf("exclude = %d, want %d", SplitTunnelModeExclude, modeExclude)
+	}
+	if SplitTunnelModeInclude != int(modeInclude) {
+		t.Errorf("include = %d, want %d", SplitTunnelModeInclude, modeInclude)
+	}
+}
+
 func TestSettingsFromSection(t *testing.T) {
 	got := settingsFromSection(splitTunnelSection{
-		Mode:     SplitTunnelModeExclude,
+		Mode:     modeExclude,
 		Excluded: []string{"com.example.a", "com.example.b"},
 		Included: []string{"com.example.c"},
 	})
 
 	if got.Mode != SplitTunnelModeExclude {
-		t.Errorf("mode = %q, want %q", got.Mode, SplitTunnelModeExclude)
+		t.Errorf("mode = %d, want %d", got.Mode, SplitTunnelModeExclude)
 	}
 	if got.Excluded.Size() != 2 || got.Excluded.Get(0) != "com.example.a" {
 		t.Errorf("excluded = %v, want the two stored packages", packagesOf(got.Excluded))
@@ -52,7 +67,7 @@ func TestSettingsFromEmptySectionCarriesEverything(t *testing.T) {
 	got := settingsFromSection(splitTunnelSection{})
 
 	if got.Mode != SplitTunnelModeOff {
-		t.Errorf("mode = %q, want %q", got.Mode, SplitTunnelModeOff)
+		t.Errorf("mode = %d, want %d", got.Mode, SplitTunnelModeOff)
 	}
 	if got.Excluded == nil || got.Included == nil {
 		t.Fatal("both selections must be usable lists, not nil")
@@ -62,6 +77,23 @@ func TestSettingsFromEmptySectionCarriesEverything(t *testing.T) {
 	}
 }
 
+// The section is what the profile's preference file holds, so the mode has to
+// survive a JSON round trip as the number the constants name.
+func TestSectionEncodesTheModeAsItsNumber(t *testing.T) {
+	raw, err := json.Marshal(sectionFromSettings(&SplitTunnelSettings{Mode: SplitTunnelModeInclude}))
+	if err != nil {
+		t.Fatalf("marshal section: %v", err)
+	}
+
+	var back splitTunnelSection
+	if err := json.Unmarshal(raw, &back); err != nil {
+		t.Fatalf("unmarshal section: %v", err)
+	}
+	if back.Mode != modeInclude {
+		t.Errorf("mode = %d, want %d, from %s", back.Mode, modeInclude, raw)
+	}
+}
+
 func TestSectionFromSettingsRoundTrip(t *testing.T) {
 	settings := NewSplitTunnelSettings()
 	settings.Mode = SplitTunnelModeInclude
@@ -72,7 +104,7 @@ func TestSectionFromSettingsRoundTrip(t *testing.T) {
 	back := settingsFromSection(section)
 
 	if back.Mode != SplitTunnelModeInclude {
-		t.Errorf("mode = %q, want %q", back.Mode, SplitTunnelModeInclude)
+		t.Errorf("mode = %d, want %d", back.Mode, SplitTunnelModeInclude)
 	}
 	if !reflect.DeepEqual(packagesOf(back.Included), []string{"com.example.a"}) {
 		t.Errorf("included = %v, want [com.example.a]", packagesOf(back.Included))
@@ -84,11 +116,21 @@ func TestSectionFromSettingsRoundTrip(t *testing.T) {
 	}
 }
 
+// A mode the Java side never sets, such as one left by a newer build, must not
+// reach the stored section either.
+func TestSectionFromSettingsNormalizesAnUnknownMode(t *testing.T) {
+	section := sectionFromSettings(&SplitTunnelSettings{Mode: 7})
+
+	if section.Mode != modeOff {
+		t.Errorf("mode = %d, want %d", section.Mode, modeOff)
+	}
+}
+
 func TestSectionFromNilSettings(t *testing.T) {
 	section := sectionFromSettings(nil)
 
-	if section.Mode != SplitTunnelModeOff {
+	if section.Mode != modeOff {
-		t.Errorf("mode = %q, want %q", section.Mode, SplitTunnelModeOff)
+		t.Errorf("mode = %d, want %d", section.Mode, modeOff)
 	}
 	if len(section.Excluded) != 0 || len(section.Included) != 0 {
 		t.Errorf("selections = %v/%v, want both empty", section.Excluded, section.Included)