Skip to content

Commit

Permalink
Revert "update Rust for recorded context to handle event store queries"
Browse files Browse the repository at this point in the history
This reverts commit b8f83af.
  • Loading branch information
DonalMe committed Oct 26, 2024
1 parent f7f0add commit 5468c36
Show file tree
Hide file tree
Showing 18 changed files with 84 additions and 572 deletions.
3 changes: 0 additions & 3 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,6 @@

## ⚠️ Breaking Changes ⚠️

### Nimbus SDK ⛅️🔬🔭
- Added methods to `RecordedContext` for retrieving event queries and setting their values back to the foreign object ([#6322](https://github.com/mozilla/application-services/pull/6322)).

### Remote Settings
- Updated Error hierarchy. We don't need to update consumer code because the only consumer was
Android and it only caught exceptions using the base RemoteSettingsException class.
Expand Down
32 changes: 10 additions & 22 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 1 addition & 3 deletions components/nimbus/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ name = "nimbus"
default=["stateful"]
rkv-safe-mode = ["dep:rkv"]
stateful-uniffi-bindings = []
stateful = ["rkv-safe-mode", "stateful-uniffi-bindings", "dep:remote_settings", "dep:regex"]
stateful = ["rkv-safe-mode", "stateful-uniffi-bindings", "dep:remote_settings"]

[dependencies]
anyhow = "1"
Expand All @@ -39,7 +39,6 @@ unicode-segmentation = "1.8.0"
error-support = { path = "../support/error" }
remote_settings = { path = "../remote_settings", optional = true }
cfg-if = "1.0.0"
regex = { version = "1.10.5", optional = true }

[build-dependencies]
uniffi = { workspace = true, features = ["build"] }
Expand All @@ -50,4 +49,3 @@ viaduct-reqwest = { path = "../support/viaduct-reqwest" }
env_logger = "0.10"
clap = "2.34"
tempfile = "3"
ctor = "0.2.2"
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertNotNull
import org.junit.Assert.assertNull
import org.junit.Assert.assertThrows
import org.junit.Assert.assertTrue
import org.junit.Before
import org.junit.Ignore
Expand All @@ -38,9 +37,7 @@ import org.mozilla.experiments.nimbus.GleanMetrics.NimbusHealth
import org.mozilla.experiments.nimbus.internal.EnrollmentChangeEvent
import org.mozilla.experiments.nimbus.internal.EnrollmentChangeEventType
import org.mozilla.experiments.nimbus.internal.JsonObject
import org.mozilla.experiments.nimbus.internal.NimbusException
import org.mozilla.experiments.nimbus.internal.RecordedContext
import org.mozilla.experiments.nimbus.internal.validateEventQueries
import org.robolectric.RobolectricTestRunner
import java.util.Calendar
import java.util.concurrent.Executors
Expand Down Expand Up @@ -74,7 +71,6 @@ class NimbusTests {
private fun createNimbus(
coenrollingFeatureIds: List<String> = listOf(),
recordedContext: RecordedContext? = null,
block: Nimbus.() -> Unit = {},
) = Nimbus(
context = context,
appInfo = appInfo,
Expand All @@ -84,7 +80,7 @@ class NimbusTests {
observer = null,
delegate = nimbusDelegate,
recordedContext = recordedContext,
).also(block)
)

@get:Rule
val gleanRule = GleanTestRule(context)
Expand Down Expand Up @@ -738,59 +734,23 @@ class NimbusTests {
assertEquals("Event count must match", isReadyEvents.count(), 3)
}

class TestRecordedContext(
private val eventQueries: Map<String, String>? = null,
private var eventQueryValues: Map<String, Double>? = null,
) : RecordedContext {
var recorded = mutableListOf<JSONObject>()

override fun getEventQueries(): Map<String, String> {
return eventQueries?.toMap() ?: mapOf()
}

override fun setEventQueryValues(eventQueryValues: Map<String, Double>) {
this.eventQueryValues = eventQueryValues
}

override fun record() {
recorded.add(this.toJson())
}

override fun toJson(): JsonObject {
val contextToRecord = JSONObject()
contextToRecord.put("enabled", true)
contextToRecord.put("events", JSONObject(eventQueryValues ?: mapOf<String, Double>()))
return contextToRecord
}
}

@Test
fun `Nimbus records context if it's passed in`() {
val context = TestRecordedContext()
val nimbus = createNimbus(recordedContext = context)

suspend fun getString(): String {
return testExperimentsJsonString(appInfo, packageName)
}

val job = nimbus.applyLocalExperiments(::getString)
runBlocking {
job.join()
}
class TestRecordedContext : RecordedContext {
var recordCount = 0

assertEquals(context.recorded.size, 1)
}
override fun record() {
recordCount++
}

@Test
fun `Nimbus recorded context event queries are run and the value is written back into the object`() {
val context = TestRecordedContext(
mapOf(
"TEST_QUERY" to "'event'|eventSum('Days', 1, 0)",
),
)
val nimbus = createNimbus(recordedContext = context) {
recordEvent("event")
override fun toJson(): JsonObject {
val contextToRecord = JSONObject()
contextToRecord.put("enabled", true)
return contextToRecord
}
}
val context = TestRecordedContext()
val nimbus = createNimbus(recordedContext = context)

suspend fun getString(): String {
return testExperimentsJsonString(appInfo, packageName)
Expand All @@ -801,19 +761,7 @@ class NimbusTests {
job.join()
}

assertEquals(context.recorded.size, 1)
assertEquals(context.recorded[0].getJSONObject("events").getDouble("TEST_QUERY"), 1.0, 0.0)
}

@Test
fun `Nimbus recorded context event queries are validated`() {
val context = TestRecordedContext(
mapOf(
"FAILING_QUERY" to "'event'|eventSumThisWillFail('Days', 1, 0)",
),
)

assertThrows("Expected an error to be thrown", NimbusException::class.java, { validateEventQueries(context) })
assertEquals(context.recordCount, 1)
}
}

Expand Down
11 changes: 0 additions & 11 deletions components/nimbus/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,6 @@ pub enum NimbusError {
CirrusError(#[from] CirrusClientError),
#[error("UniFFI callback error: {0}")]
UniFFICallbackError(#[from] uniffi::UnexpectedUniFFICallbackError),
#[cfg(feature = "stateful")]
#[error("Regex error: {0}")]
RegexError(#[from] regex::Error),
}

#[cfg(feature = "stateful")]
Expand All @@ -84,14 +81,6 @@ pub enum BehaviorError {
IntervalParseError(String),
#[error("The event store is not available on the targeting attributes")]
MissingEventStore,
#[error("The recorded context is not available on the nimbus client")]
MissingRecordedContext,
#[error("EventQueryTypeParseError: {0} is not a valid EventQueryType")]
EventQueryTypeParseError(String),
#[error(r#"EventQueryParseError: "{0}" is not a valid EventQuery"#)]
EventQueryParseError(String),
#[error(r#"TypeError: "{0}" is not of type {1}"#)]
TypeError(String, String),
}

#[cfg(not(feature = "stateful"))]
Expand Down
16 changes: 1 addition & 15 deletions components/nimbus/src/nimbus.udl
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,7 @@ typedef extern RemoteSettingsConfig;
[ExternalExport="remote_settings"]
typedef extern RemoteSettingsServer;

namespace nimbus {

/// A test utility used to validate event queries against the jexl evaluator.
///
/// This method should only be used in tests.
[Throws=NimbusError]
void validate_event_queries(RecordedContext recorded_context);
};

namespace nimbus {};
dictionary AppContext {
string app_name;
string app_id;
Expand Down Expand Up @@ -112,7 +104,6 @@ enum NimbusError {
"InvalidPath", "InternalError", "NoSuchExperiment", "NoSuchBranch",
"DatabaseNotReady", "VersionParsingError", "BehaviorError", "TryFromIntError",
"ParseIntError", "TransformParameterError", "ClientError", "UniFFICallbackError",
"RegexError",
};

[Custom]
Expand All @@ -122,10 +113,6 @@ typedef string JsonObject;
interface RecordedContext {
JsonObject to_json();

record<string, string> get_event_queries();

void set_event_query_values(record<string, f64> event_query_values);

void record();
};

Expand Down Expand Up @@ -296,7 +283,6 @@ interface NimbusClient {

[Throws=NimbusError]
void dump_state_to_log();

};

interface NimbusTargetingHelper {
Expand Down
16 changes: 4 additions & 12 deletions components/nimbus/src/stateful/behavior.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ impl PartialEq for Interval {
self.to_string() == other.to_string()
}
}

impl Eq for Interval {}

impl Hash for Interval {
Expand All @@ -85,7 +84,7 @@ impl FromStr for Interval {
_ => {
return Err(NimbusError::BehaviorError(
BehaviorError::IntervalParseError(input.to_string()),
));
))
}
})
}
Expand Down Expand Up @@ -366,7 +365,7 @@ impl EventQueryType {
return Err(NimbusError::TransformParameterError(format!(
"event transform {} requires a positive number as the second parameter",
self
)));
)))
}
} as usize;
let zero = &Value::from(0);
Expand All @@ -376,7 +375,7 @@ impl EventQueryType {
return Err(NimbusError::TransformParameterError(format!(
"event transform {} requires a positive number as the third parameter",
self
)));
)))
}
} as usize;

Expand All @@ -403,7 +402,7 @@ impl EventQueryType {
return Err(NimbusError::TransformParameterError(format!(
"event transform {} requires a positive number as the second parameter",
self
)));
)))
}
} as usize;

Expand All @@ -428,13 +427,6 @@ impl EventQueryType {
})
}

pub fn validate_query(maybe_query: &str) -> Result<bool> {
let regex = regex::Regex::new(
r#"^(?:"[^"']+"|'[^"']+')\|event(?:Sum|LastSeen|CountNonZero|Average|AveragePerNonZeroInterval)\(["'](?:Years|Months|Weeks|Days|Hours|Minutes)["'],\s*\d+\s*(?:,\s*\d+\s*)?\)$"#,
)?;
Ok(regex.is_match(maybe_query))
}

fn error_value(&self) -> f64 {
match self {
Self::LastSeen => f64::MAX,
Expand Down
Loading

0 comments on commit 5468c36

Please sign in to comment.