From 27dccb56a6a4dd7e8fb15d5074df54d492f0a0dc Mon Sep 17 00:00:00 2001 From: brizental Date: Mon, 20 Jul 2020 15:55:36 +0200 Subject: [PATCH] Attend to review comments * setWithCompactRepr -> setWithCompactRepresentation * testGetValueAsPeriodDelimitedString -> testGetCompactRepresentation --- .../telemetry/glean/private/JweMetricType.kt | 13 +++++++------ .../mozilla/telemetry/glean/rust/LibGleanFFI.kt | 2 +- .../telemetry/glean/private/JweMetricTypeTest.kt | 10 +++++----- glean-core/csharp/Glean/LibGleanFFI.cs | 2 +- glean-core/csharp/Glean/Metrics/JweMetricType.cs | 6 +++--- .../csharp/GleanTests/Metrics/JweMetricTypeTest.cs | 10 +++++----- glean-core/ffi/glean.h | 2 +- glean-core/ffi/src/jwe.rs | 4 ++-- glean-core/ios/Glean/GleanFfi.h | 2 +- glean-core/ios/Glean/Metrics/JweMetric.swift | 8 ++++---- .../ios/GleanTests/Metrics/JweMetricTests.swift | 12 ++++++------ glean-core/python/glean/metrics/jwe.py | 8 ++++---- glean-core/python/tests/metrics/test_jwe.py | 10 +++++----- glean-core/src/metrics/jwe.rs | 2 +- glean-core/tests/jwe.rs | 8 ++++---- 15 files changed, 50 insertions(+), 49 deletions(-) diff --git a/glean-core/android/src/main/java/mozilla/telemetry/glean/private/JweMetricType.kt b/glean-core/android/src/main/java/mozilla/telemetry/glean/private/JweMetricType.kt index ed7a03d9ec..7ffc292541 100644 --- a/glean-core/android/src/main/java/mozilla/telemetry/glean/private/JweMetricType.kt +++ b/glean-core/android/src/main/java/mozilla/telemetry/glean/private/JweMetricType.kt @@ -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( @@ -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) } } @@ -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() } /** @@ -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() diff --git a/glean-core/android/src/main/java/mozilla/telemetry/glean/rust/LibGleanFFI.kt b/glean-core/android/src/main/java/mozilla/telemetry/glean/rust/LibGleanFFI.kt index 866d74f4a5..a15f12783f 100644 --- a/glean-core/android/src/main/java/mozilla/telemetry/glean/rust/LibGleanFFI.kt +++ b/glean-core/android/src/main/java/mozilla/telemetry/glean/rust/LibGleanFFI.kt @@ -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, diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/JweMetricTypeTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/JweMetricTypeTest.kt index e0260ae724..928f32dc33 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/JweMetricTypeTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/JweMetricTypeTest.kt @@ -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 @@ -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 @@ -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)) } } diff --git a/glean-core/csharp/Glean/LibGleanFFI.cs b/glean-core/csharp/Glean/LibGleanFFI.cs index f07f95eba3..d6d0aedf09 100644 --- a/glean-core/csharp/Glean/LibGleanFFI.cs +++ b/glean-core/csharp/Glean/LibGleanFFI.cs @@ -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); diff --git a/glean-core/csharp/Glean/Metrics/JweMetricType.cs b/glean-core/csharp/Glean/Metrics/JweMetricType.cs index 1054d02327..45ece7760a 100644 --- a/glean-core/csharp/Glean/Metrics/JweMetricType.cs +++ b/glean-core/csharp/Glean/Metrics/JweMetricType.cs @@ -53,7 +53,7 @@ string[] sendInPings /// Set a JWE value. /// /// The [`compact representation`](https://tools.ietf.org/html/rfc7516#appendix-A.2.7) of a JWE value. - public void SetWithCompactRepr(string value) + public void setWithCompactRepresentation(string value) { if (disabled) { @@ -61,7 +61,7 @@ public void SetWithCompactRepr(string value) } Dispatchers.LaunchAPI(() => { - LibGleanFFI.glean_jwe_set_with_compact_repr(this.handle, value); + LibGleanFFI.glean_jwe_set_with_compact_representation(this.handle, value); }); } @@ -136,7 +136,7 @@ public string TestGetValue(string pingName = null) /// Defaults to the first value in `sendInPings` /// value of the stored metric /// Thrown when the metric contains no value - public string TestGetValueAsPeriodDelimitedString(string pingName = null) + public string testGetCompactRepresentation(string pingName = null) { Dispatchers.AssertInTestingMode(); diff --git a/glean-core/csharp/GleanTests/Metrics/JweMetricTypeTest.cs b/glean-core/csharp/GleanTests/Metrics/JweMetricTypeTest.cs index 1968962562..92d6c7728a 100644 --- a/glean-core/csharp/GleanTests/Metrics/JweMetricTypeTest.cs +++ b/glean-core/csharp/GleanTests/Metrics/JweMetricTypeTest.cs @@ -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] @@ -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] @@ -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)); } } diff --git a/glean-core/ffi/glean.h b/glean-core/ffi/glean.h index eb9f9548ba..7aee3325df 100644 --- a/glean-core/ffi/glean.h +++ b/glean-core/ffi/glean.h @@ -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, diff --git a/glean-core/ffi/src/jwe.rs b/glean-core/ffi/src/jwe.rs index b4c93ed7c2..5602ef851c 100644 --- a/glean-core/ffi/src/jwe.rs +++ b/glean-core/ffi/src/jwe.rs @@ -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(()) }) }) diff --git a/glean-core/ios/Glean/GleanFfi.h b/glean-core/ios/Glean/GleanFfi.h index eb9f9548ba..7aee3325df 100644 --- a/glean-core/ios/Glean/GleanFfi.h +++ b/glean-core/ios/Glean/GleanFfi.h @@ -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, diff --git a/glean-core/ios/Glean/Metrics/JweMetric.swift b/glean-core/ios/Glean/Metrics/JweMetric.swift index d7fd118174..4007f937c8 100644 --- a/glean-core/ios/Glean/Metrics/JweMetric.swift +++ b/glean-core/ios/Glean/Metrics/JweMetric.swift @@ -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 @@ -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) } } @@ -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] diff --git a/glean-core/ios/GleanTests/Metrics/JweMetricTests.swift b/glean-core/ios/GleanTests/Metrics/JweMetricTests.swift index 74ddc2f010..048cb12683 100644 --- a/glean-core/ios/GleanTests/Metrics/JweMetricTests.swift +++ b/glean-core/ios/GleanTests/Metrics/JweMetricTests.swift @@ -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() { @@ -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") } @@ -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() { @@ -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)) } } diff --git a/glean-core/python/glean/metrics/jwe.py b/glean-core/python/glean/metrics/jwe.py index 53ff85d9b3..66166e7c33 100644 --- a/glean-core/python/glean/metrics/jwe.py +++ b/glean-core/python/glean/metrics/jwe.py @@ -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. """ @@ -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. @@ -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) ) @@ -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: """ diff --git a/glean-core/python/tests/metrics/test_jwe.py b/glean-core/python/tests/metrics/test_jwe.py index b66a9504d4..6df7595728 100644 --- a/glean-core/python/tests/metrics/test_jwe.py +++ b/glean-core/python/tests/metrics/test_jwe.py @@ -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(): @@ -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(): @@ -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) diff --git a/glean-core/src/metrics/jwe.rs b/glean-core/src/metrics/jwe.rs index bb6ede4628..0df965bf8d 100644 --- a/glean-core/src/metrics/jwe.rs +++ b/glean-core/src/metrics/jwe.rs @@ -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>(&self, glean: &Glean, value: S) { + pub fn set_with_compact_representation>(&self, glean: &Glean, value: S) { if !self.should_record(glean) { return; } diff --git a/glean-core/tests/jwe.rs b/glean-core/tests/jwe.rs index e79aa56a96..d6ddef4872 100644 --- a/glean-core/tests/jwe.rs +++ b/glean-core/tests/jwe.rs @@ -30,7 +30,7 @@ fn jwe_metric_is_generated_and_stored() { ..Default::default() }); - metric.set_with_compact_repr(&glean, JWE); + metric.set_with_compact_representation(&glean, JWE); let snapshot = StorageManager .snapshot_as_json(glean.storage(), "core", false) .unwrap(); @@ -55,7 +55,7 @@ fn set_properly_sets_the_value_in_all_stores() { ..Default::default() }); - metric.set_with_compact_repr(&glean, JWE); + metric.set_with_compact_representation(&glean, JWE); // Check that the data was correctly set in each store. for store_name in store_names { @@ -83,7 +83,7 @@ fn get_test_value_returns_the_period_delimited_string() { ..Default::default() }); - metric.set_with_compact_repr(&glean, JWE); + metric.set_with_compact_representation(&glean, JWE); assert_eq!(metric.test_get_value(&glean, "core").unwrap(), JWE); } @@ -101,7 +101,7 @@ fn get_test_value_as_json_string_returns_the_expected_repr() { ..Default::default() }); - metric.set_with_compact_repr(&glean, JWE); + metric.set_with_compact_representation(&glean, JWE); let expected_json = format!("{{\"header\":\"{}\",\"key\":\"{}\",\"init_vector\":\"{}\",\"cipher_text\":\"{}\",\"auth_tag\":\"{}\"}}", HEADER, KEY, INIT_VECTOR, CIPHER_TEXT, AUTH_TAG); assert_eq!(