From c33fc77de4abfcc51a2a1be3c5c5e3be510f26a3 Mon Sep 17 00:00:00 2001 From: Abodunrinwa Toki Date: Wed, 6 Feb 2019 01:17:10 +0000 Subject: [PATCH] TextClassifier: normalize uri for browser intent. Also updates TemplateIntentFactory to validate RemoteActionTemplates. Bug: 123640937 Test: atest core/tests/coretests/src/android/view/textclassifier/TextClassifierTest.java Test: atest core/tests/coretests/src/android/view/textclassifier/TemplateIntentFactoryTest.java Change-Id: I2dc5d988234f1aaace4b30e9fbe272937c99ca47 --- .../view/textclassifier/ExtrasUtils.java | 15 +- .../textclassifier/LegacyIntentFactory.java | 3 +- .../textclassifier/TemplateIntentFactory.java | 75 ++++++---- ...mplateClassificationIntentFactoryTest.java | 48 +++---- .../TemplateIntentFactoryTest.java | 136 +++++++++++++++--- .../textclassifier/TextClassifierTest.java | 20 +++ .../TextClassifierEventTronLoggerTest.java | 4 +- 7 files changed, 215 insertions(+), 86 deletions(-) diff --git a/core/java/android/view/textclassifier/ExtrasUtils.java b/core/java/android/view/textclassifier/ExtrasUtils.java index 602455c65beb0..b0e7ad5d7264a 100644 --- a/core/java/android/view/textclassifier/ExtrasUtils.java +++ b/core/java/android/view/textclassifier/ExtrasUtils.java @@ -85,15 +85,16 @@ public final class ExtrasUtils { } /** - * Returns the first "translate" action found in the {@code classification} object. + * Returns the first action found in the {@code classification} object with an intent + * action string, {@code intentAction}. */ @Nullable - public static RemoteAction findTranslateAction(TextClassification classification) { + public static RemoteAction findAction(TextClassification classification, String intentAction) { final ArrayList actionIntents = getActionsIntents(classification); if (actionIntents != null) { final int size = actionIntents.size(); for (int i = 0; i < size; i++) { - if (Intent.ACTION_TRANSLATE.equals(actionIntents.get(i).getAction())) { + if (intentAction.equals(actionIntents.get(i).getAction())) { return classification.getActions().get(i); } } @@ -101,6 +102,14 @@ public final class ExtrasUtils { return null; } + /** + * Returns the first "translate" action found in the {@code classification} object. + */ + @Nullable + public static RemoteAction findTranslateAction(TextClassification classification) { + return findAction(classification, Intent.ACTION_TRANSLATE); + } + /** * Returns the entity type contained in the {@code extra}. */ diff --git a/core/java/android/view/textclassifier/LegacyIntentFactory.java b/core/java/android/view/textclassifier/LegacyIntentFactory.java index b6e5b3e26b16e..2d0d032cfef3c 100644 --- a/core/java/android/view/textclassifier/LegacyIntentFactory.java +++ b/core/java/android/view/textclassifier/LegacyIntentFactory.java @@ -182,7 +182,8 @@ public final class LegacyIntentFactory implements IntentFactory { actions.add(new LabeledIntent( context.getString(com.android.internal.R.string.browse), context.getString(com.android.internal.R.string.browse_desc), - new Intent(Intent.ACTION_VIEW, Uri.parse(text)) + new Intent(Intent.ACTION_VIEW) + .setDataAndNormalize(Uri.parse(text)) .putExtra(Browser.EXTRA_APPLICATION_ID, context.getPackageName()), LabeledIntent.DEFAULT_REQUEST_CODE)); return actions; diff --git a/core/java/android/view/textclassifier/TemplateIntentFactory.java b/core/java/android/view/textclassifier/TemplateIntentFactory.java index 52788436b3ea2..95f88c7de1462 100644 --- a/core/java/android/view/textclassifier/TemplateIntentFactory.java +++ b/core/java/android/view/textclassifier/TemplateIntentFactory.java @@ -49,20 +49,18 @@ public final class TemplateIntentFactory { } final List labeledIntents = new ArrayList<>(); for (RemoteActionTemplate remoteActionTemplate : remoteActionTemplates) { - Intent intent = createIntent(remoteActionTemplate); - if (intent == null) { + if (!isValidTemplate(remoteActionTemplate)) { + Log.w(TAG, "Invalid RemoteActionTemplate skipped."); continue; } - TextClassifierImpl.LabeledIntent - labeledIntent = new TextClassifierImpl.LabeledIntent( - remoteActionTemplate.title, - remoteActionTemplate.description, - intent, - remoteActionTemplate.requestCode == null - ? TextClassifierImpl.LabeledIntent.DEFAULT_REQUEST_CODE - : remoteActionTemplate.requestCode - ); - labeledIntents.add(labeledIntent); + labeledIntents.add( + new TextClassifierImpl.LabeledIntent( + remoteActionTemplate.title, + remoteActionTemplate.description, + createIntent(remoteActionTemplate), + remoteActionTemplate.requestCode == null + ? TextClassifierImpl.LabeledIntent.DEFAULT_REQUEST_CODE + : remoteActionTemplate.requestCode)); } labeledIntents.forEach( action -> action.getIntent() @@ -70,29 +68,43 @@ public final class TemplateIntentFactory { return labeledIntents; } - @Nullable - private static Intent createIntent(RemoteActionTemplate remoteActionTemplate) { - Intent intent = new Intent(); + private static boolean isValidTemplate(@Nullable RemoteActionTemplate remoteActionTemplate) { + if (remoteActionTemplate == null) { + Log.w(TAG, "Invalid RemoteActionTemplate: is null"); + return false; + } + if (TextUtils.isEmpty(remoteActionTemplate.title)) { + Log.w(TAG, "Invalid RemoteActionTemplate: title is null"); + return false; + } + if (TextUtils.isEmpty(remoteActionTemplate.description)) { + Log.w(TAG, "Invalid RemoteActionTemplate: description is null"); + return false; + } if (!TextUtils.isEmpty(remoteActionTemplate.packageName)) { - Log.w(TAG, "A RemoteActionTemplate is skipped as package name is set."); - return null; + Log.w(TAG, "Invalid RemoteActionTemplate: package name is set"); + return false; } - if (!TextUtils.isEmpty(remoteActionTemplate.action)) { - intent.setAction(remoteActionTemplate.action); - } - Uri data = null; - if (!TextUtils.isEmpty(remoteActionTemplate.data)) { - data = Uri.parse(remoteActionTemplate.data); - } - if (data != null || !TextUtils.isEmpty(remoteActionTemplate.type)) { - intent.setDataAndType(data, remoteActionTemplate.type); - } - if (remoteActionTemplate.flags != null) { - intent.setFlags(remoteActionTemplate.flags); + if (TextUtils.isEmpty(remoteActionTemplate.action)) { + Log.w(TAG, "Invalid RemoteActionTemplate: intent action not set"); + return false; } + return true; + } + + private static Intent createIntent(RemoteActionTemplate remoteActionTemplate) { + final Intent intent = new Intent(remoteActionTemplate.action); + final Uri uri = TextUtils.isEmpty(remoteActionTemplate.data) + ? null : Uri.parse(remoteActionTemplate.data).normalizeScheme(); + final String type = TextUtils.isEmpty(remoteActionTemplate.type) + ? null : Intent.normalizeMimeType(remoteActionTemplate.type); + intent.setDataAndType(uri, type); + intent.setFlags(remoteActionTemplate.flags == null ? 0 : remoteActionTemplate.flags); if (remoteActionTemplate.category != null) { for (String category : remoteActionTemplate.category) { - intent.addCategory(category); + if (category != null) { + intent.addCategory(category); + } } } intent.putExtras(createExtras(remoteActionTemplate.extras)); @@ -105,6 +117,9 @@ public final class TemplateIntentFactory { } Bundle bundle = new Bundle(); for (NamedVariant namedVariant : namedVariants) { + if (namedVariant == null) { + continue; + } switch (namedVariant.getType()) { case NamedVariant.TYPE_INT: bundle.putInt(namedVariant.getName(), namedVariant.getInt()); diff --git a/core/tests/coretests/src/android/view/textclassifier/TemplateClassificationIntentFactoryTest.java b/core/tests/coretests/src/android/view/textclassifier/TemplateClassificationIntentFactoryTest.java index 08ad62ab843d9..d9dac314fc7c0 100644 --- a/core/tests/coretests/src/android/view/textclassifier/TemplateClassificationIntentFactoryTest.java +++ b/core/tests/coretests/src/android/view/textclassifier/TemplateClassificationIntentFactoryTest.java @@ -41,6 +41,7 @@ public class TemplateClassificationIntentFactoryTest { private static final String TEXT = "text"; private static final String TITLE = "Map"; + private static final String DESCRIPTION = "Opens in Maps"; private static final String ACTION = Intent.ACTION_VIEW; @Mock @@ -57,19 +58,6 @@ public class TemplateClassificationIntentFactoryTest { @Test public void create_foreignText() { - RemoteActionTemplate remoteActionTemplate = new RemoteActionTemplate( - TITLE, - null, - ACTION, - null, - null, - null, - null, - null, - null, - null - ); - AnnotatorModel.ClassificationResult classificationResult = new AnnotatorModel.ClassificationResult( TextClassifier.TYPE_ADDRESS, @@ -81,7 +69,7 @@ public class TemplateClassificationIntentFactoryTest { null, null, null, - new RemoteActionTemplate[]{remoteActionTemplate}); + createRemoteActionTemplates()); List intents = mTemplateClassificationIntentFactory.create( @@ -106,19 +94,6 @@ public class TemplateClassificationIntentFactoryTest { @Test public void create_notForeignText() { - RemoteActionTemplate remoteActionTemplate = new RemoteActionTemplate( - TITLE, - null, - ACTION, - null, - null, - null, - null, - null, - null, - null - ); - AnnotatorModel.ClassificationResult classificationResult = new AnnotatorModel.ClassificationResult( TextClassifier.TYPE_ADDRESS, @@ -130,7 +105,7 @@ public class TemplateClassificationIntentFactoryTest { null, null, null, - new RemoteActionTemplate[]{remoteActionTemplate}); + createRemoteActionTemplates()); List intents = mTemplateClassificationIntentFactory.create( @@ -147,4 +122,21 @@ public class TemplateClassificationIntentFactoryTest { assertThat(intent.getAction()).isEqualTo(ACTION); assertThat(intent.hasExtra(TextClassifier.EXTRA_FROM_TEXT_CLASSIFIER)).isTrue(); } + + private static RemoteActionTemplate[] createRemoteActionTemplates() { + return new RemoteActionTemplate[]{ + new RemoteActionTemplate( + TITLE, + DESCRIPTION, + ACTION, + null, + null, + null, + null, + null, + null, + null + ) + }; + } } diff --git a/core/tests/coretests/src/android/view/textclassifier/TemplateIntentFactoryTest.java b/core/tests/coretests/src/android/view/textclassifier/TemplateIntentFactoryTest.java index 0d364a329de4a..a1158a7cbb6c5 100644 --- a/core/tests/coretests/src/android/view/textclassifier/TemplateIntentFactoryTest.java +++ b/core/tests/coretests/src/android/view/textclassifier/TemplateIntentFactoryTest.java @@ -81,7 +81,6 @@ public class TemplateIntentFactoryTest { REQUEST_CODE ); - List intents = mTemplateIntentFactory.create(new RemoteActionTemplate[]{remoteActionTemplate}); @@ -97,14 +96,79 @@ public class TemplateIntentFactoryTest { assertThat(intent.getFlags()).isEqualTo(FLAG); assertThat(intent.getCategories()).containsExactly((Object[]) CATEGORY); assertThat(intent.getPackage()).isNull(); - assertThat( - intent.getStringExtra(KEY_ONE)).isEqualTo(VALUE_ONE); + assertThat(intent.getStringExtra(KEY_ONE)).isEqualTo(VALUE_ONE); assertThat(intent.getIntExtra(KEY_TWO, 0)).isEqualTo(VALUE_TWO); assertThat(intent.hasExtra(TextClassifier.EXTRA_FROM_TEXT_CLASSIFIER)).isTrue(); } @Test - public void create_packageIsNotNull() { + public void normalizesScheme() { + RemoteActionTemplate remoteActionTemplate = new RemoteActionTemplate( + TITLE, + DESCRIPTION, + ACTION, + "HTTp://www.android.com", + TYPE, + FLAG, + CATEGORY, + /* packageName */ null, + NAMED_VARIANTS, + REQUEST_CODE + ); + + List intents = + mTemplateIntentFactory.create(new RemoteActionTemplate[] {remoteActionTemplate}); + + String data = intents.get(0).getIntent().getData().toString(); + assertThat(data).isEqualTo("http://www.android.com"); + } + + @Test + public void create_minimal() { + RemoteActionTemplate remoteActionTemplate = new RemoteActionTemplate( + TITLE, + DESCRIPTION, + ACTION, + null, + null, + null, + null, + null, + null, + null + ); + + List intents = + mTemplateIntentFactory.create(new RemoteActionTemplate[]{remoteActionTemplate}); + + assertThat(intents).hasSize(1); + TextClassifierImpl.LabeledIntent labeledIntent = intents.get(0); + assertThat(labeledIntent.getTitle()).isEqualTo(TITLE); + assertThat(labeledIntent.getDescription()).isEqualTo(DESCRIPTION); + assertThat(labeledIntent.getRequestCode()).isEqualTo( + TextClassifierImpl.LabeledIntent.DEFAULT_REQUEST_CODE); + Intent intent = labeledIntent.getIntent(); + assertThat(intent.getAction()).isEqualTo(ACTION); + assertThat(intent.getData()).isNull(); + assertThat(intent.getType()).isNull(); + assertThat(intent.getFlags()).isEqualTo(0); + assertThat(intent.getCategories()).isNull(); + assertThat(intent.getPackage()).isNull(); + assertThat(intent.hasExtra(TextClassifier.EXTRA_FROM_TEXT_CLASSIFIER)).isTrue(); + } + + @Test + public void invalidTemplate_nullTemplate() { + RemoteActionTemplate remoteActionTemplate = null; + + List intents = + mTemplateIntentFactory.create(new RemoteActionTemplate[] {remoteActionTemplate}); + + assertThat(intents).isEmpty(); + } + + @Test + public void invalidTemplate_nonEmptyPackageName() { RemoteActionTemplate remoteActionTemplate = new RemoteActionTemplate( TITLE, DESCRIPTION, @@ -121,41 +185,69 @@ public class TemplateIntentFactoryTest { List intents = mTemplateIntentFactory.create(new RemoteActionTemplate[] {remoteActionTemplate}); - assertThat(intents).hasSize(0); + assertThat(intents).isEmpty(); } @Test - public void create_minimal() { + public void invalidTemplate_emptyTitle() { RemoteActionTemplate remoteActionTemplate = new RemoteActionTemplate( null, + DESCRIPTION, + ACTION, null, null, null, null, null, null, + null + ); + + List intents = + mTemplateIntentFactory.create(new RemoteActionTemplate[] {remoteActionTemplate}); + + assertThat(intents).isEmpty(); + } + + @Test + public void invalidTemplate_emptyDescription() { + RemoteActionTemplate remoteActionTemplate = new RemoteActionTemplate( + TITLE, + null, + ACTION, + null, + null, + null, + null, + null, + null, + null + ); + + List intents = + mTemplateIntentFactory.create(new RemoteActionTemplate[] {remoteActionTemplate}); + + assertThat(intents).isEmpty(); + } + + @Test + public void invalidTemplate_emptyIntentAction() { + RemoteActionTemplate remoteActionTemplate = new RemoteActionTemplate( + TITLE, + DESCRIPTION, + null, + null, + null, + null, + null, null, null, null ); List intents = - mTemplateIntentFactory.create(new RemoteActionTemplate[]{remoteActionTemplate}); + mTemplateIntentFactory.create(new RemoteActionTemplate[] {remoteActionTemplate}); - - assertThat(intents).hasSize(1); - TextClassifierImpl.LabeledIntent labeledIntent = intents.get(0); - assertThat(labeledIntent.getTitle()).isNull(); - assertThat(labeledIntent.getDescription()).isNull(); - assertThat(labeledIntent.getRequestCode()).isEqualTo( - TextClassifierImpl.LabeledIntent.DEFAULT_REQUEST_CODE); - Intent intent = labeledIntent.getIntent(); - assertThat(intent.getAction()).isNull(); - assertThat(intent.getData()).isNull(); - assertThat(intent.getType()).isNull(); - assertThat(intent.getFlags()).isEqualTo(0); - assertThat(intent.getCategories()).isNull(); - assertThat(intent.getPackage()).isNull(); - assertThat(intent.hasExtra(TextClassifier.EXTRA_FROM_TEXT_CLASSIFIER)).isTrue(); + assertThat(intents).isEmpty(); } } diff --git a/core/tests/coretests/src/android/view/textclassifier/TextClassifierTest.java b/core/tests/coretests/src/android/view/textclassifier/TextClassifierTest.java index 582be9d51731d..bdd03707c1f18 100644 --- a/core/tests/coretests/src/android/view/textclassifier/TextClassifierTest.java +++ b/core/tests/coretests/src/android/view/textclassifier/TextClassifierTest.java @@ -176,6 +176,7 @@ public class TextClassifierTest { TextClassification classification = mClassifier.classifyText(request); assertThat(classification, isTextClassification(classifiedText, TextClassifier.TYPE_URL)); + assertThat(classification, containsIntentWithAction(Intent.ACTION_VIEW)); } @Test @@ -207,6 +208,7 @@ public class TextClassifierTest { TextClassification classification = mClassifier.classifyText(request); assertThat(classification, isTextClassification(classifiedText, TextClassifier.TYPE_URL)); + assertThat(classification, containsIntentWithAction(Intent.ACTION_VIEW)); } @Test @@ -517,6 +519,24 @@ public class TextClassifierTest { }; } + private static Matcher containsIntentWithAction(final String action) { + return new BaseMatcher() { + @Override + public boolean matches(Object o) { + if (o instanceof TextClassification) { + TextClassification result = (TextClassification) o; + return ExtrasUtils.findAction(result, action) != null; + } + return false; + } + + @Override + public void describeTo(Description description) { + description.appendText("intent action=").appendValue(action); + } + }; + } + private static Matcher isTextLanguage(final String languageTag) { return new BaseMatcher() { @Override diff --git a/core/tests/coretests/src/android/view/textclassifier/logging/TextClassifierEventTronLoggerTest.java b/core/tests/coretests/src/android/view/textclassifier/logging/TextClassifierEventTronLoggerTest.java index 73af56743b5f1..1980a604fdd2a 100644 --- a/core/tests/coretests/src/android/view/textclassifier/logging/TextClassifierEventTronLoggerTest.java +++ b/core/tests/coretests/src/android/view/textclassifier/logging/TextClassifierEventTronLoggerTest.java @@ -91,8 +91,8 @@ public class TextClassifierEventTronLoggerTest { .isEqualTo(ConversationAction.TYPE_CALL_PHONE); assertThat((float) logMaker.getTaggedData(FIELD_TEXT_CLASSIFIER_SCORE)) .isWithin(0.00001f).of(0.5f); - assertThat(logMaker.getTaggedData(FIELD_TEXT_CLASSIFIER_EVENT_TIME)) - .isEqualTo(EVENT_TIME); + // Never write event time. + assertThat(logMaker.getTaggedData(FIELD_TEXT_CLASSIFIER_EVENT_TIME)).isNull(); assertThat(logMaker.getPackageName()).isEqualTo(PACKAGE_NAME); assertThat(logMaker.getTaggedData(FIELD_TEXT_CLASSIFIER_WIDGET_TYPE)) .isEqualTo(WIDGET_TYPE);