Skip to content

Commit

Permalink
Attend to review comments
Browse files Browse the repository at this point in the history
* setWithCompactRepr -> setWithCompactRepresentation
* testGetValueAsPeriodDelimitedString -> testGetCompactRepresentation
  • Loading branch information
brizental committed Jul 20, 2020
1 parent 040f7b1 commit 27dccb5
Show file tree
Hide file tree
Showing 15 changed files with 50 additions and 49 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import mozilla.telemetry.glean.testing.ErrorType
* Instances of this class type are automatically generated by the parsers at build time,
* allowing developers to record values that were previously registered in the metrics.yaml file.
*
* The JWE API exposes the [set] and [setWithCompactRepr] methods,
* The JWE API exposes the [set] and [setWithCompactRepresentation] methods,
* which take care of validating the input data.
*/
class JweMetricType internal constructor(
Expand Down Expand Up @@ -53,14 +53,14 @@ class JweMetricType internal constructor(
*
* @param value The compact representation of a JWE value.
*/
fun setWithCompactRepr(value: String) {
fun setWithCompactRepresentation(value: String) {
if (disabled) {
return
}

@Suppress("EXPERIMENTAL_API_USAGE")
Dispatchers.API.launch {
LibGleanFFI.INSTANCE.glean_jwe_set_with_compact_repr(this@JweMetricType.handle, value)
LibGleanFFI.INSTANCE.glean_jwe_set_with_compact_representation(this@JweMetricType.handle, value)
}
}

Expand Down Expand Up @@ -102,8 +102,9 @@ class JweMetricType internal constructor(
@Suppress("EXPERIMENTAL_API_USAGE")
Dispatchers.API.assertInTestingMode()

val res = LibGleanFFI.INSTANCE.glean_jwe_test_has_value(this.handle, pingName)
return res.toBoolean()
return LibGleanFFI.INSTANCE
.glean_jwe_test_has_value(this.handle, pingName)
.toBoolean()
}

/**
Expand Down Expand Up @@ -140,7 +141,7 @@ class JweMetricType internal constructor(
*/
@VisibleForTesting(otherwise = VisibleForTesting.NONE)
@JvmOverloads
fun testGetValueAsPeriodDelimitedString(pingName: String = sendInPings.first()): String {
fun testGetCompactRepresentation(pingName: String = sendInPings.first()): String {
@Suppress("EXPERIMENTAL_API_USAGE")
Dispatchers.API.assertInTestingMode()

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -569,7 +569,7 @@ internal interface LibGleanFFI : Library {

fun glean_destroy_jwe_metric(handle: Long)

fun glean_jwe_set_with_compact_repr(metric_id: Long, value: String)
fun glean_jwe_set_with_compact_representation(metric_id: Long, value: String)

fun glean_jwe_set(
metric_id: Long,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,12 @@ class JweMetricTypeTest {

// Check that data was properly recorded.
assertTrue(jweMetric.testHasValue())
assertEquals(JWE, jweMetric.testGetValueAsPeriodDelimitedString())
assertEquals(JWE, jweMetric.testGetCompactRepresentation())

jweMetric.set(HEADER, "", "", CIPHER_TEXT, "")
// Check that data was properly recorded.
assertTrue(jweMetric.testHasValue())
assertEquals(MINIMUM_JWE, jweMetric.testGetValueAsPeriodDelimitedString())
assertEquals(MINIMUM_JWE, jweMetric.testGetCompactRepresentation())
}

@Test
Expand Down Expand Up @@ -111,12 +111,12 @@ class JweMetricTypeTest {

// Check that data was properly recorded in the second ping.
assertTrue(jweMetric.testHasValue("store2"))
assertEquals(JWE, jweMetric.testGetValueAsPeriodDelimitedString("store2"))
assertEquals(JWE, jweMetric.testGetCompactRepresentation("store2"))

jweMetric.set(HEADER, "", "", CIPHER_TEXT, "")
// Check that data was properly recorded in the second ping.
assertTrue(jweMetric.testHasValue("store2"))
assertEquals(MINIMUM_JWE, jweMetric.testGetValueAsPeriodDelimitedString())
assertEquals(MINIMUM_JWE, jweMetric.testGetCompactRepresentation())
}

@Test
Expand All @@ -135,7 +135,7 @@ class JweMetricTypeTest {
assertEquals(1, jweMetric.testGetNumRecordedErrors(ErrorType.InvalidOverflow))

// Invalid compact string representation yield a InvalidValue error
jweMetric.setWithCompactRepr("")
jweMetric.setWithCompactRepresentation("")
assertEquals(1, jweMetric.testGetNumRecordedErrors(ErrorType.InvalidValue))
}
}
2 changes: 1 addition & 1 deletion glean-core/csharp/Glean/LibGleanFFI.cs
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ string auth_tag
);

[DllImport(SharedGleanLibrary, ExactSpelling = true, CallingConvention = CallingConvention.Cdecl)]
internal static extern void glean_jwe_set_with_compact_repr(UInt64 metric_id, string value);
internal static extern void glean_jwe_set_with_compact_representation(UInt64 metric_id, string value);

[DllImport(SharedGleanLibrary, ExactSpelling = true, CallingConvention = CallingConvention.Cdecl)]
internal static extern StringAsReturnValue glean_jwe_test_get_value(UInt64 metric_id, string storage_name);
Expand Down
6 changes: 3 additions & 3 deletions glean-core/csharp/Glean/Metrics/JweMetricType.cs
Original file line number Diff line number Diff line change
Expand Up @@ -53,15 +53,15 @@ string[] sendInPings
/// Set a JWE value.
/// </summary>
/// <param name="value"> The [`compact representation`](https://tools.ietf.org/html/rfc7516#appendix-A.2.7) of a JWE value.</param>
public void SetWithCompactRepr(string value)
public void setWithCompactRepresentation(string value)
{
if (disabled)
{
return;
}

Dispatchers.LaunchAPI(() => {
LibGleanFFI.glean_jwe_set_with_compact_repr(this.handle, value);
LibGleanFFI.glean_jwe_set_with_compact_representation(this.handle, value);
});
}

Expand Down Expand Up @@ -136,7 +136,7 @@ public string TestGetValue(string pingName = null)
/// Defaults to the first value in `sendInPings`</param>
/// <returns>value of the stored metric</returns>
/// <exception cref="System.NullReferenceException">Thrown when the metric contains no value</exception>
public string TestGetValueAsPeriodDelimitedString(string pingName = null)
public string testGetCompactRepresentation(string pingName = null)
{
Dispatchers.AssertInTestingMode();

Expand Down
10 changes: 5 additions & 5 deletions glean-core/csharp/GleanTests/Metrics/JweMetricTypeTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,13 @@ public void APISavesToStorage()

// Check that data was properly recorded.
Assert.True(jweMetric.TestHasValue());
Assert.Equal(this.jwe, jweMetric.TestGetValueAsPeriodDelimitedString());
Assert.Equal(this.jwe, jweMetric.testGetCompactRepresentation());

jweMetric.Set(this.header, "", "", this.cipherText, "");

// Check that data was properly recorded.
Assert.True(jweMetric.TestHasValue());
Assert.Equal(this.minimumJwe, jweMetric.TestGetValueAsPeriodDelimitedString());
Assert.Equal(this.minimumJwe, jweMetric.testGetCompactRepresentation());
}

[Fact]
Expand Down Expand Up @@ -108,12 +108,12 @@ public void APISavesToSecondaryPings()

// Check that data was properly recorded in the second ping.
Assert.True(jweMetric.TestHasValue("store2"));
Assert.Equal(this.jwe, jweMetric.TestGetValueAsPeriodDelimitedString("store2"));
Assert.Equal(this.jwe, jweMetric.testGetCompactRepresentation("store2"));

jweMetric.Set(this.header, "", "", this.cipherText, "");
// Check that data was properly recorded in the second ping.
Assert.True(jweMetric.TestHasValue("store2"));
Assert.Equal(this.minimumJwe, jweMetric.TestGetValueAsPeriodDelimitedString("store2"));
Assert.Equal(this.minimumJwe, jweMetric.testGetCompactRepresentation("store2"));
}

[Fact]
Expand All @@ -132,7 +132,7 @@ public void SettingALongStringRecordsAnError()
Assert.Equal(1, jweMetric.TestGetNumRecordedErrors(ErrorType.InvalidOverflow));

// Invalid compact string representation yield a InvalidValue error
jweMetric.SetWithCompactRepr("");
jweMetric.setWithCompactRepresentation("");
Assert.Equal(1, jweMetric.TestGetNumRecordedErrors(ErrorType.InvalidValue));
}
}
Expand Down
2 changes: 1 addition & 1 deletion glean-core/ffi/glean.h
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,7 @@ void glean_jwe_set(uint64_t metric_id,
FfiStr cipher_text,
FfiStr auth_tag);

void glean_jwe_set_with_compact_repr(uint64_t metric_id, FfiStr value);
void glean_jwe_set_with_compact_representation(uint64_t metric_id, FfiStr value);

int32_t glean_jwe_test_get_num_recorded_errors(uint64_t metric_id,
int32_t error_type,
Expand Down
4 changes: 2 additions & 2 deletions glean-core/ffi/src/jwe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@ define_metric!(JweMetric => JWE_METRICS {
});

#[no_mangle]
pub extern "C" fn glean_jwe_set_with_compact_repr(metric_id: u64, value: FfiStr) {
pub extern "C" fn glean_jwe_set_with_compact_representation(metric_id: u64, value: FfiStr) {
with_glean_value(|glean| {
JWE_METRICS.call_with_log(metric_id, |metric| {
let value = value.to_string_fallible()?;
metric.set_with_compact_repr(&glean, value);
metric.set_with_compact_representation(&glean, value);
Ok(())
})
})
Expand Down
2 changes: 1 addition & 1 deletion glean-core/ios/Glean/GleanFfi.h
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,7 @@ void glean_jwe_set(uint64_t metric_id,
FfiStr cipher_text,
FfiStr auth_tag);

void glean_jwe_set_with_compact_repr(uint64_t metric_id, FfiStr value);
void glean_jwe_set_with_compact_representation(uint64_t metric_id, FfiStr value);

int32_t glean_jwe_test_get_num_recorded_errors(uint64_t metric_id,
int32_t error_type,
Expand Down
8 changes: 4 additions & 4 deletions glean-core/ios/Glean/Metrics/JweMetric.swift
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import Foundation
/// Instances of this class type are automatically generated by the parsers at build time,
/// allowing developers to record values that were previously registered in the metrics.yaml file.
///
/// The JWE API exposes the `JweMetricType.set(_:)` and `JweMetricType.setWithCompactRepr(_:)` methods,
/// The JWE API exposes the `JweMetricType.set(_:)` and `JweMetricType.setWithCompactRepresentation(_:)` methods,
/// which take care of validating the input data.
public class JweMetricType {
let handle: UInt64
Expand Down Expand Up @@ -43,11 +43,11 @@ public class JweMetricType {
///
/// - parameters:
/// * header: value The [`compact representation`](https://tools.ietf.org/html/rfc7516#appendix-A.2.7) of a JWE value.
public func setWithCompactRepr(_ value: String) {
public func setWithCompactRepresentation(_ value: String) {
guard !self.disabled else { return }

Dispatchers.shared.launchAPI {
glean_jwe_set_with_compact_repr(self.handle, value)
glean_jwe_set_with_compact_representation(self.handle, value)
}
}

Expand Down Expand Up @@ -118,7 +118,7 @@ public class JweMetricType {
/// Defaults to the first value in `sendInPings`.
///
/// - returns: value of the stored metric
public func testGetValueAsPeriodDelimitedString(_ pingName: String? = nil) throws -> String {
public func testGetCompactRepresentation(_ pingName: String? = nil) throws -> String {
Dispatchers.shared.assertInTestingMode()

let pingName = pingName ?? self.sendInPings[0]
Expand Down
12 changes: 6 additions & 6 deletions glean-core/ios/GleanTests/Metrics/JweMetricTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,12 @@ class JweMetricTests: XCTestCase {
jweMetric.set(self.header, self.key, self.initVector, self.cipherText, self.authTag)

XCTAssert(jweMetric.testHasValue())
XCTAssertEqual(self.jwe, try jweMetric.testGetValueAsPeriodDelimitedString())
XCTAssertEqual(self.jwe, try jweMetric.testGetCompactRepresentation())

jweMetric.set(self.header, "", "", self.cipherText, "")

XCTAssert(jweMetric.testHasValue())
XCTAssertEqual(self.minimumJwe, try jweMetric.testGetValueAsPeriodDelimitedString())
XCTAssertEqual(self.minimumJwe, try jweMetric.testGetCompactRepresentation())
}

func testJweMustNotRecordIfDisabled() {
Expand All @@ -55,7 +55,7 @@ class JweMetricTests: XCTestCase {

XCTAssertFalse(jweMetric.testHasValue())

jweMetric.setWithCompactRepr(self.jwe)
jweMetric.setWithCompactRepresentation(self.jwe)

XCTAssertFalse(jweMetric.testHasValue(), "JWEs must not be recorded if they are disabled")
}
Expand Down Expand Up @@ -86,12 +86,12 @@ class JweMetricTests: XCTestCase {
jweMetric.set(self.header, self.key, self.initVector, self.cipherText, self.authTag)

XCTAssert(jweMetric.testHasValue("store2"))
XCTAssertEqual(jwe, try jweMetric.testGetValueAsPeriodDelimitedString())
XCTAssertEqual(jwe, try jweMetric.testGetCompactRepresentation())

jweMetric.set(self.header, "", "", self.cipherText, "")

XCTAssert(jweMetric.testHasValue())
XCTAssertEqual(self.minimumJwe, try jweMetric.testGetValueAsPeriodDelimitedString())
XCTAssertEqual(self.minimumJwe, try jweMetric.testGetCompactRepresentation())
}

func testSettingInvalidValuesRecordsErrors() {
Expand All @@ -108,7 +108,7 @@ class JweMetricTests: XCTestCase {
XCTAssertEqual(1, jweMetric.testGetNumRecordedErrors(ErrorType.invalidOverflow))

// Invalid compact string representation yield a InvalidValue error
jweMetric.setWithCompactRepr("")
jweMetric.setWithCompactRepresentation("")
XCTAssertEqual(1, jweMetric.testGetNumRecordedErrors(ErrorType.invalidValue))
}
}
8 changes: 4 additions & 4 deletions glean-core/python/glean/metrics/jwe.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ class JweMetricType:
previously registered in the metrics.yaml file.
The string API exposes the `JweMetricType.set` and
`JweMetricType.setWithCompactRepr` methods,
`JweMetricType.setWithCompactRepresentation` methods,
which take care of validating the input data
and making sure that limits are enforced.
"""
Expand Down Expand Up @@ -52,7 +52,7 @@ def __del__(self):
if getattr(self, "_handle", 0) != 0:
_ffi.lib.glean_destroy_jwe_metric(self._handle)

def set_with_compact_repr(self, value: str):
def set_with_compact_representation(self, value: str):
"""
Set to the specified JWE value.
Expand All @@ -64,7 +64,7 @@ def set_with_compact_repr(self, value: str):

@Dispatcher.launch
def set():
_ffi.lib.glean_jwe_set_with_compact_repr(
_ffi.lib.glean_jwe_set_with_compact_representation(
self._handle, _ffi.ffi_encode_string(value)
)

Expand Down Expand Up @@ -142,7 +142,7 @@ def test_get_value(self, ping_name: Optional[str] = None) -> str:
)
)

def test_get_value_as_period_delimited_string(
def test_get_compact_representation(
self, ping_name: Optional[str] = None
) -> str:
"""
Expand Down
10 changes: 5 additions & 5 deletions glean-core/python/tests/metrics/test_jwe.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,12 @@ def test_the_api_saves_to_its_storage_engine():
jwe_metric.set(header, key, init_vector, cipher_text, auth_tag)

assert jwe_metric.test_has_value()
assert jwe == jwe_metric.test_get_value_as_period_delimited_string()
assert jwe == jwe_metric.test_get_compact_representation()

jwe_metric.set(header, "", "", cipher_text, "")

assert jwe_metric.test_has_value()
assert minimum_jwe == jwe_metric.test_get_value_as_period_delimited_string()
assert minimum_jwe == jwe_metric.test_get_compact_representation()


def test_disabled_jwes_must_not_record_data():
Expand Down Expand Up @@ -63,12 +63,12 @@ def test_the_api_saves_to_secondary_pings():
jwe_metric.set(header, key, init_vector, cipher_text, auth_tag)

assert jwe_metric.test_has_value("store2")
assert jwe == jwe_metric.test_get_value_as_period_delimited_string("store2")
assert jwe == jwe_metric.test_get_compact_representation("store2")

jwe_metric.set(header, "", "", cipher_text, "")

assert jwe_metric.test_has_value("store2")
assert minimum_jwe == jwe_metric.test_get_value_as_period_delimited_string("store2")
assert minimum_jwe == jwe_metric.test_get_compact_representation("store2")


def test_setting_invalid_values_record_errors():
Expand All @@ -85,5 +85,5 @@ def test_setting_invalid_values_record_errors():
testing.ErrorType.INVALID_OVERFLOW
)

jwe_metric.set_with_compact_repr("")
jwe_metric.set_with_compact_representation("")
assert 1 == jwe_metric.test_get_num_recorded_errors(testing.ErrorType.INVALID_VALUE)
2 changes: 1 addition & 1 deletion glean-core/src/metrics/jwe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ impl JweMetric {
///
/// * `glean` - the Glean instance this metric belongs to.
/// * `value` - the [`compact representation`](https://tools.ietf.org/html/rfc7516#appendix-A.2.7) of a JWE value.
pub fn set_with_compact_repr<S: Into<String>>(&self, glean: &Glean, value: S) {
pub fn set_with_compact_representation<S: Into<String>>(&self, glean: &Glean, value: S) {
if !self.should_record(glean) {
return;
}
Expand Down
Loading

0 comments on commit 27dccb5

Please sign in to comment.