android: create persistent SharedPreferences-based KV store

[goaway]

Description: Updates the exposed KeyValueStore type to be a more traditional implementable interface, and provides a simple persisting implementation based on Android SharedPreferences.
Risk: Low
Testing: Application

Signed-off-by: Mike Schore mike.schore@gmail.com

[jpsim]

Looks straightforward enough. Should we expose a builder API to enable this?

[goaway]

@jpsim Setting a KV store is already supported in the Kotlin EngineBuilder, with Swift to follow. :)

[Augustyniak]

should we have some docs for this?

[goaway]

Added docstring, and the interface is documented - would be great to generate docs from these.

[goaway]

(Looks like @jpsim has made some great progress here!)

[Augustyniak]

Should we add some unit tests specific to SharedPreferencesStore class?

[goaway]

We don't today have a unit test setup that loads the Android platform APIs, because we don't today have a lot of Android-only or Android-specific code (by design). If/as we add more, we probably do want such a test suite, but this is mostly meant to be a trivial example implementation that guides others in using the API. That said, once we have a place for such tests to live, I agree we should cover this at the unit level for completeness' sake.

[Augustyniak]

I see, thank you for the context.

I would argue that we should have a test even for this trivial example since otherwise we cannot really tell whether it works or not the way we expect it to work - as in, I think that we are past the threshold when we can justify having an Android tests suite (if we have a need for Android code we have a need for Android tests).

Could we create a GH issue for this if you do not want to do this as part of this PR? I would love us to start investing in testing more, otherwise it's going to become increasingly hard to continue to move forward without leaving broken stuff behind us.

[jpsim]

Roboelectric supports this today, and we use it in quite a few tests: https://github.com/envoyproxy/envoy-mobile/search?q=robolectric

[jpsim]

Should we expose a builder API to enable this?

Setting a KV store is already supported in the Kotlin EngineBuilder

Sorry I wasn't clear. Right now to use this I think users would have to configure the builder like this:

.addKeyValueStore("some name the user has to choose", SharedPreferencesStore(...))

Why do we have to expose the SharedPreferencesStore in the public API at all? Could we not do this?

.useDefaultPersistentKeyValueStore(true)

And configure SharedPreferencesStore entirely internally to the library?

[goaway]

@jpsim a name for the store is required in the builder API, because more than one key-value store may be registered. The SharedPreferencesStore implementation requires an instance of SharedPreferences which is create/provided by the Android API. We could create our own, but that would then limit the user's ability to supply one of their choosing. (There are also limitations to creating our own - it would require even more general access to the application context, of which EM is currently agnostic.)

https://developer.android.com/training/data-storage/shared-preferences

Anyways, that's the reasoning behind the method signatures. :)

[goaway]

As an update to my comment above, I misremembered - EM does have access to some derivation of the application context at least at creation, since that's needed to initialize DNS. So we could in theory use that to create our own default SharedPreferences instance. But I still have some concerns: we don't really know much about that context and whether it's appropriate to use it for that purpose (there's several flavors of Android context and we're not picky about which we receive), we don't really have any notion of a reserved namespace for creating our store, there's a potential for leakiness in usage of a persistent store if it's not somehow managed, and finally I'm just not certain of what the right defaults should be.

So my inclination is to say that I while think this is a good idea, we perhaps ought to opt for simplicity and flexibility at this point until we have a better notion of where and how this gets used.

[jpsim]

a name for the store is required in the builder API, because more than one key-value store may be registered

I understand that, but we could have a reserved/default name for this persistent store.

Noted on the complexity and responsibility creep involved in accessing the application context internally in Envoy Mobile. Requiring this to be wired explicitly is fine with me for now, but ideally we'd improve on this pattern so that Envoy Mobile's default configuration gets this benefit out of the box.

Also, having reasonable/good defaults doesn't mean that we shouldn't expose ways to change them in case they want to refine/disable/change it for their use case.

[Augustyniak]

Also, having reasonable/good defaults doesn't mean that we shouldn't expose ways to change them in case they want to refine/disable/change it for their use case.

That would be my preference as well -> make some defaults that work just fine for most users and allow them to override it when they need something custom. System level networking libraries on iOS/Android do not require you to provide any caching configuration to get basic caching up and running.

If we want to experiment with this first that's fine but I would argue that the final API needs to have some notions of defaults implemented.

[goaway]

I think we're all in agreement here? The only thing I'm proposing is we hold off on making assumptions around what a default should be until we understand see a bit better how it's being used (alt-svc caching, http caching, etc.).

[jpsim]

I think we're all in agreement here?

To recap some offline discussions, yes in agreement as a first version, which requires consumers to wire this through the addKeyValueStore builder API.

Ideally before merging this would still have the following:

1. changelog entry
2. doc updates
3. usage in the experimental Kotlin app

I still think a test using roboelectric to simulate Android's shared preferences would be good to have but understand if that's not something you want to tackle here.

[jpsim]

Nice work