From 750da42c98d8388de4323d2cad2a6f71547c7463 Mon Sep 17 00:00:00 2001 From: ohall-m Date: Thu, 23 May 2024 17:53:55 +0000 Subject: [PATCH] Bug 1898358 - Normalize ModelManagementOptions to Lower Case r=android-reviewers,geckoview-reviewers,calu There is a bug where `ModelManagementOptions` accepts un-normalized strings and will have errant behavior. Differential Revision: https://phabricator.services.mozilla.com/D211295 --- .../engine/translate/ModelOperation.kt | 8 +++++ .../engine/translate/OperationLevel.kt | 8 +++++ .../geckoview/test/TranslationsTest.kt | 33 +++++++++++++++++-- .../geckoview/TranslationsController.java | 11 +++++-- .../geckoview/GeckoViewTranslations.sys.mjs | 9 +++-- 5 files changed, 61 insertions(+), 8 deletions(-) diff --git a/mobile/android/android-components/components/concept/engine/src/main/java/mozilla/components/concept/engine/translate/ModelOperation.kt b/mobile/android/android-components/components/concept/engine/src/main/java/mozilla/components/concept/engine/translate/ModelOperation.kt index 66ee50227c54..5ea54423f144 100644 --- a/mobile/android/android-components/components/concept/engine/src/main/java/mozilla/components/concept/engine/translate/ModelOperation.kt +++ b/mobile/android/android-components/components/concept/engine/src/main/java/mozilla/components/concept/engine/translate/ModelOperation.kt @@ -17,4 +17,12 @@ enum class ModelOperation(val operation: String) { * Delete the model(s). */ DELETE("delete"), + ; + + /** + * The operation will use the string literal on the engine. + */ + override fun toString(): String { + return operation + } } diff --git a/mobile/android/android-components/components/concept/engine/src/main/java/mozilla/components/concept/engine/translate/OperationLevel.kt b/mobile/android/android-components/components/concept/engine/src/main/java/mozilla/components/concept/engine/translate/OperationLevel.kt index e39a3239ecdc..31eec5373f99 100644 --- a/mobile/android/android-components/components/concept/engine/src/main/java/mozilla/components/concept/engine/translate/OperationLevel.kt +++ b/mobile/android/android-components/components/concept/engine/src/main/java/mozilla/components/concept/engine/translate/OperationLevel.kt @@ -23,4 +23,12 @@ enum class OperationLevel(val operationLevel: String) { * Complete the operation all models. */ ALL("all"), + ; + + /** + * The operation level will use the string literal on the engine. + */ + override fun toString(): String { + return operationLevel + } } diff --git a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/TranslationsTest.kt b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/TranslationsTest.kt index 966eba73b633..29ee7c952918 100644 --- a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/TranslationsTest.kt +++ b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/TranslationsTest.kt @@ -352,10 +352,23 @@ class TranslationsTest : BaseSessionTest() { fun testManageLanguageModel() { val options = ModelManagementOptions.Builder() .languageToManage("en") - .operation(TranslationsController.RuntimeTranslation.DOWNLOAD) + .operation(DOWNLOAD) .build() - assertTrue("ModelManagementOptions builder options work as expected.", options.language == "en" && options.operation == DOWNLOAD) + assertTrue( + "ModelManagementOptions builder options work as expected.", + options.language == "en" && options.operation == DOWNLOAD, + ) + + val nonNormalizedOptions = ModelManagementOptions.Builder() + .languageToManage("EN") + .operation("DoWnLoAd") + .build() + + assertTrue( + "ModelManagementOptions builder options work as expected on non-normalized options.", + nonNormalizedOptions.language == "en" && nonNormalizedOptions.operation == DOWNLOAD, + ) } @Test @@ -641,6 +654,22 @@ class TranslationsTest : BaseSessionTest() { te.code == ERROR_MODEL_COULD_NOT_DELETE, ) } + + val malformedRequest = ModelManagementOptions.Builder() + .operation("not-a-function") + .operationLevel("not-an-operation") + .build() + try { + sessionRule.waitForResult(RuntimeTranslation.manageLanguageModel(malformedRequest)) + assertTrue("Should not complete malformed requests in automation.", false) + } catch (e: RuntimeException) { + // Wait call causes a runtime exception too. + val te = e.cause as TranslationsException + assertTrue( + "Correctly could not submit a malformed request.", + te.code == TranslationsException.ERROR_UNKNOWN, + ) + } } } diff --git a/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/TranslationsController.java b/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/TranslationsController.java index 256877532b3f..f2681ce86ff4 100644 --- a/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/TranslationsController.java +++ b/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/TranslationsController.java @@ -18,6 +18,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.Objects; import org.mozilla.gecko.EventDispatcher; @@ -410,9 +411,13 @@ public class TranslationsController { */ protected ModelManagementOptions( final @NonNull RuntimeTranslation.ModelManagementOptions.Builder builder) { - this.language = builder.mLanguage; - this.operation = builder.mOperation; - this.operationLevel = builder.mOperationLevel; + if (builder.mLanguage != null) { + this.language = builder.mLanguage.toLowerCase(Locale.ROOT); + } else { + this.language = builder.mLanguage; + } + this.operation = builder.mOperation.toLowerCase(Locale.ROOT); + this.operationLevel = builder.mOperationLevel.toLowerCase(Locale.ROOT); } /** Serializer for Model Management Options */ diff --git a/mobile/shared/modules/geckoview/GeckoViewTranslations.sys.mjs b/mobile/shared/modules/geckoview/GeckoViewTranslations.sys.mjs index 7dc0cc676bb5..37019e227314 100644 --- a/mobile/shared/modules/geckoview/GeckoViewTranslations.sys.mjs +++ b/mobile/shared/modules/geckoview/GeckoViewTranslations.sys.mjs @@ -218,10 +218,8 @@ export const GeckoViewTranslationsSettings = { ); } ); - return; } - } - if (operation === "download") { + } else if (operation === "download") { if (operationLevel === "all") { lazy.TranslationsParent.downloadAllFiles().then( function () { @@ -253,6 +251,11 @@ export const GeckoViewTranslationsSettings = { } ); } + } else { + aCallback.onError( + `ERROR_UNKNOWN - The request to manage models appears to be malformed. Please check the parameters and try again. + Language: ${language}, Operation: ${operation}, Operation Level: ${operationLevel}` + ); } break; }