Skip to content
Permalink
Browse files

Issue #432: Fretboard: Kinto delete diffs might lead to crash

Closes #432: Fretboard: Kinto delete diffs might lead to crash
  • Loading branch information...
fercarcedo authored and pocmo committed Jul 18, 2018
1 parent 7e56af9 commit ee164fc8121c1cb338be1e28c00b99d2077a7799
@@ -14,9 +14,9 @@ interface ExperimentSource {
* parsing the response into experiments
*
* @param client Http client to use, provided by Fretboard
* @param experiments list of already downloaded experiments
* @param snapshot list of already downloaded experiments
* (in order to process a diff response, for example)
* @return modified list of experiments
*/
fun getExperiments(experiments: List<Experiment>): List<Experiment>
fun getExperiments(snapshot: ExperimentsSnapshot): ExperimentsSnapshot
}
@@ -14,12 +14,12 @@ interface ExperimentStorage {
*
* @param experiments list of experiments to store
*/
fun save(experiments: List<Experiment>)
fun save(snapshot: ExperimentsSnapshot)

/**
* Reads experiments from disk
*
* @return experiments from disk
*/
fun retrieve(): List<Experiment>
fun retrieve(): ExperimentsSnapshot
}
@@ -0,0 +1,13 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

package mozilla.components.service.fretboard

/**
* Represents an experiment sync result
*/
data class ExperimentsSnapshot(
val experiments: List<Experiment>,
val lastModified: Long?
)
@@ -17,7 +17,7 @@ class Fretboard(
private val storage: ExperimentStorage,
regionProvider: RegionProvider? = null
) {
private var experiments: List<Experiment> = listOf()
private var experimentsResult: ExperimentsSnapshot = ExperimentsSnapshot(listOf(), null)
private var experimentsLoaded: Boolean = false
private val evaluator = ExperimentEvaluator(regionProvider)

@@ -26,7 +26,7 @@ class Fretboard(
*/
@Synchronized
fun loadExperiments() {
experiments = storage.retrieve()
experimentsResult = storage.retrieve()
experimentsLoaded = true
}

@@ -40,8 +40,8 @@ class Fretboard(
loadExperiments()
}
try {
val serverExperiments = source.getExperiments(experiments)
experiments = serverExperiments
val serverExperiments = source.getExperiments(experimentsResult)
experimentsResult = serverExperiments
storage.save(serverExperiments)
} catch (e: ExperimentDownloadException) {
// Keep using the local experiments
@@ -58,7 +58,7 @@ class Fretboard(
* @return true if the user is part of the specified experiment, false otherwise
*/
fun isInExperiment(context: Context, descriptor: ExperimentDescriptor): Boolean {
return evaluator.evaluate(context, descriptor, experiments) != null
return evaluator.evaluate(context, descriptor, experimentsResult.experiments) != null
}

/**
@@ -69,7 +69,7 @@ class Fretboard(
* @param block block of code to be executed if the user is part of the experiment
*/
fun withExperiment(context: Context, descriptor: ExperimentDescriptor, block: (Experiment) -> Unit) {
evaluator.evaluate(context, descriptor, experiments)?.let { block(it) }
evaluator.evaluate(context, descriptor, experimentsResult.experiments)?.let { block(it) }
}

/**
@@ -80,7 +80,7 @@ class Fretboard(
* @return metadata associated with the experiment
*/
fun getExperiment(descriptor: ExperimentDescriptor): Experiment? {
return evaluator.getExperiment(descriptor, experiments)
return evaluator.getExperiment(descriptor, experimentsResult.experiments)
}

/**
@@ -4,9 +4,9 @@

package mozilla.components.service.fretboard.source.kinto

import mozilla.components.service.fretboard.Experiment
import mozilla.components.service.fretboard.ExperimentSource
import mozilla.components.service.fretboard.JSONExperimentParser
import mozilla.components.service.fretboard.ExperimentsSnapshot
import org.json.JSONArray
import org.json.JSONObject

@@ -24,13 +24,13 @@ class KintoExperimentSource(
val collectionName: String,
private val client: HttpClient = HttpURLConnectionHttpClient()
) : ExperimentSource {
override fun getExperiments(experiments: List<Experiment>): List<Experiment> {
val experimentsDiff = getExperimentsDiff(client, experiments)
return mergeExperimentsFromDiff(experimentsDiff, experiments)
override fun getExperiments(snapshot: ExperimentsSnapshot): ExperimentsSnapshot {
val experimentsDiff = getExperimentsDiff(client, snapshot)
return mergeExperimentsFromDiff(experimentsDiff, snapshot)
}

private fun getExperimentsDiff(client: HttpClient, experiments: List<Experiment>): String {
val lastModified = getMaxLastModified(experiments)
private fun getExperimentsDiff(client: HttpClient, snapshot: ExperimentsSnapshot): String {
val lastModified = snapshot.lastModified
val kintoClient = KintoClient(client, baseUrl, bucketName, collectionName)
return if (lastModified != null) {
kintoClient.diff(lastModified)
@@ -39,53 +39,35 @@ class KintoExperimentSource(
}
}

private fun mergeExperimentsFromDiff(experimentsDiff: String, experiments: List<Experiment>): List<Experiment> {
private fun mergeExperimentsFromDiff(experimentsDiff: String, snapshot: ExperimentsSnapshot): ExperimentsSnapshot {
val experiments = snapshot.experiments
val mutableExperiments = experiments.toMutableList()
val experimentParser = JSONExperimentParser()
val diffJsonObject = JSONObject(experimentsDiff)
val data = diffJsonObject.get(DATA_KEY)
if (data is JSONObject) {
if (data.getBoolean(DELETED_KEY)) {
mergeDeleteDiff(data, mutableExperiments)
}
} else {
mergeAddUpdateDiff(experimentParser, data as JSONArray, mutableExperiments)
}
return mutableExperiments
}

private fun mergeDeleteDiff(data: JSONObject, mutableExperiments: MutableList<Experiment>) {
mutableExperiments.remove(mutableExperiments.single { it.id == data.getString(ID_KEY) })
}

private fun mergeAddUpdateDiff(
experimentParser: JSONExperimentParser,
experimentsJsonArray: JSONArray,
mutableExperiments: MutableList<Experiment>
) {
val experimentsJsonArray = data as JSONArray
var maxLastModified: Long? = snapshot.lastModified
for (i in 0 until experimentsJsonArray.length()) {
val experimentJsonObject = experimentsJsonArray[i] as JSONObject
val experiment = mutableExperiments.singleOrNull { it.id == experimentJsonObject.getString(ID_KEY) }
if (experiment != null)
if (experiment != null) {
mutableExperiments.remove(experiment)
mutableExperiments.add(experimentParser.fromJson(experimentJsonObject))
}
}

private fun getMaxLastModified(experiments: List<Experiment>): Long? {
var maxLastModified: Long = -1
for (experiment in experiments) {
val lastModified = experiment.lastModified
if (lastModified != null && lastModified > maxLastModified) {
maxLastModified = lastModified
}
if (!experimentJsonObject.has(DELETED_KEY)) {
mutableExperiments.add(experimentParser.fromJson(experimentJsonObject))
}
val lastModifiedDate = experimentJsonObject.getLong(LAST_MODIFIED_KEY)
if (maxLastModified == null || lastModifiedDate > maxLastModified) {
maxLastModified = lastModifiedDate
}
}
return if (maxLastModified > 0) maxLastModified else null
return ExperimentsSnapshot(mutableExperiments, maxLastModified)
}

companion object {
private const val ID_KEY = "id"
private const val DATA_KEY = "data"
private const val DELETED_KEY = "deleted"
private const val LAST_MODIFIED_KEY = "last_modified"
}
}
@@ -6,6 +6,7 @@ package mozilla.components.service.fretboard.storage.flatfile

import mozilla.components.service.fretboard.Experiment
import mozilla.components.service.fretboard.JSONExperimentParser
import mozilla.components.service.fretboard.ExperimentsSnapshot
import org.json.JSONArray
import org.json.JSONObject

@@ -19,16 +20,17 @@ internal class ExperimentsSerializer {
* Transforms the given list of experiments to
* its json file representation
*
* @param experiments experiments to serialize
* @param snapshot experiments to serialize
* @return json file representation of the given experiments
*/
fun toJson(experiments: List<Experiment>): String {
fun toJson(snapshot: ExperimentsSnapshot): String {
val experimentsJson = JSONObject()
val experimentsJSONArray = JSONArray()
val jsonParser = JSONExperimentParser()
for (experiment in experiments)
for (experiment in snapshot.experiments)
experimentsJSONArray.put(jsonParser.toJson(experiment))
experimentsJson.put(EXPERIMENTS_KEY, experimentsJSONArray)
experimentsJson.put(LAST_MODIFIED_KEY, snapshot.lastModified)
return experimentsJson.toString()
}

@@ -39,16 +41,24 @@ internal class ExperimentsSerializer {
* @param json json file contents
* @return list of experiments
*/
fun fromJson(json: String): List<Experiment> {
val experimentsJsonArray = JSONObject(json).getJSONArray(EXPERIMENTS_KEY)
fun fromJson(json: String): ExperimentsSnapshot {
val experimentsJson = JSONObject(json)
val experimentsJsonArray = experimentsJson.getJSONArray(EXPERIMENTS_KEY)
val experiments = ArrayList<Experiment>()
val jsonParser = JSONExperimentParser()
for (i in 0 until experimentsJsonArray.length())
experiments.add(jsonParser.fromJson(experimentsJsonArray[i] as JSONObject))
return experiments
val lastModified: Long?
if (experimentsJson.has(LAST_MODIFIED_KEY)) {
lastModified = experimentsJson.getLong(LAST_MODIFIED_KEY)
} else {
lastModified = null
}
return ExperimentsSnapshot(experiments, lastModified)
}

companion object {
private const val EXPERIMENTS_KEY = "experiments"
private const val LAST_MODIFIED_KEY = "last_modified"
}
}
@@ -5,25 +5,25 @@
package mozilla.components.service.fretboard.storage.flatfile

import android.util.AtomicFile
import mozilla.components.service.fretboard.Experiment
import mozilla.components.service.fretboard.ExperimentStorage
import mozilla.components.service.fretboard.ExperimentsSnapshot
import java.io.FileNotFoundException
import java.io.File

class FlatFileExperimentStorage(file: File) : ExperimentStorage {
private val atomicFile: AtomicFile = AtomicFile(file)

override fun retrieve(): List<Experiment> {
override fun retrieve(): ExperimentsSnapshot {
try {
val experimentsJson = String(atomicFile.readFully())
return ExperimentsSerializer().fromJson(experimentsJson)
} catch (e: FileNotFoundException) {
return listOf()
return ExperimentsSnapshot(listOf(), null)
}
}

override fun save(experiments: List<Experiment>) {
val experimentsJson = ExperimentsSerializer().toJson(experiments)
override fun save(snapshot: ExperimentsSnapshot) {
val experimentsJson = ExperimentsSerializer().toJson(snapshot)
atomicFile.startWrite().writer().use {
it.append(experimentsJson)
}
@@ -6,7 +6,6 @@ package mozilla.components.service.fretboard

import org.junit.Test
import org.junit.runner.RunWith
import org.mockito.ArgumentMatchers
import org.mockito.Mockito.`when`
import org.mockito.Mockito.mock
import org.mockito.Mockito.times
@@ -38,23 +37,25 @@ class FretboardTest {
@Test
fun testUpdateExperimentsEmptyStorage() {
val experimentSource = mock(ExperimentSource::class.java)
`when`(experimentSource.getExperiments(ArgumentMatchers.anyList())).thenReturn(listOf(Experiment("id")))
val result = ExperimentsSnapshot(listOf(), null)
`when`(experimentSource.getExperiments(result)).thenReturn(ExperimentsSnapshot(listOf(Experiment("id")), null))
val experimentStorage = mock(ExperimentStorage::class.java)
`when`(experimentStorage.retrieve()).thenReturn(result)
val fretboard = Fretboard(experimentSource, experimentStorage)
fretboard.updateExperiments()
verify(experimentSource).getExperiments(listOf())
verify(experimentStorage).save(listOf(Experiment("id")))
verify(experimentSource).getExperiments(result)
verify(experimentStorage).save(ExperimentsSnapshot(listOf(Experiment("id")), null))
}

@Test
fun testUpdateExperimentsFromStorage() {
val experimentSource = mock(ExperimentSource::class.java)
`when`(experimentSource.getExperiments(ArgumentMatchers.anyList())).thenReturn(listOf(Experiment("id")))
`when`(experimentSource.getExperiments(ExperimentsSnapshot(listOf(Experiment("id0")), null))).thenReturn(ExperimentsSnapshot(listOf(Experiment("id")), null))
val experimentStorage = mock(ExperimentStorage::class.java)
`when`(experimentStorage.retrieve()).thenReturn(listOf(Experiment("id0")))
`when`(experimentStorage.retrieve()).thenReturn(ExperimentsSnapshot(listOf(Experiment("id0")), null))
val fretboard = Fretboard(experimentSource, experimentStorage)
fretboard.updateExperiments()
verify(experimentSource).getExperiments(listOf(Experiment("id0")))
verify(experimentStorage).save(listOf(Experiment("id")))
verify(experimentSource).getExperiments(ExperimentsSnapshot(listOf(Experiment("id0")), null))
verify(experimentStorage).save(ExperimentsSnapshot(listOf(Experiment("id")), null))
}
}
Oops, something went wrong.

0 comments on commit ee164fc

Please sign in to comment.
You can’t perform that action at this time.