From 5caaa238eb78373b44830ed46a56eeda62e597c0 Mon Sep 17 00:00:00 2001 From: Terry Wang Date: Fri, 4 Dec 2020 18:03:06 -0800 Subject: [PATCH] Split getInstance and createInstance in AppSearchImpl User could never call an uninitialized SearchSession, so convert and propagate the initialize error in getInstace to user for every call is unnecessary. Split createInstace for initialization, and getInstance for other calls. Removed AppSearchException from getInstace's signature. Test: presubmit Bug: 162450968 Change-Id: I925506ad0f6540b060c77ed2a535737ee71f781c --- .../app/appsearch/AppSearchSession.java | 19 +++++++------ .../appsearch/AppSearchManagerService.java | 27 ++++++++++--------- .../server/appsearch/ImplInstanceManager.java | 26 ++++++++++++++++-- 3 files changed, 47 insertions(+), 25 deletions(-) diff --git a/apex/appsearch/framework/java/android/app/appsearch/AppSearchSession.java b/apex/appsearch/framework/java/android/app/appsearch/AppSearchSession.java index b7cd4f5f8bce2..1d86595a40639 100644 --- a/apex/appsearch/framework/java/android/app/appsearch/AppSearchSession.java +++ b/apex/appsearch/framework/java/android/app/appsearch/AppSearchSession.java @@ -18,7 +18,6 @@ package android.app.appsearch; import android.annotation.CallbackExecutor; import android.annotation.NonNull; -import android.app.appsearch.exceptions.AppSearchException; import android.os.Bundle; import android.os.ParcelableException; import android.os.RemoteException; @@ -167,9 +166,9 @@ public final class AppSearchSession { * of the returned {@link AppSearchBatchResult} are the URIs of the input * documents. The values are {@code null} if they were successfully indexed, * or a failed {@link AppSearchResult} otherwise. - * Or {@link BatchResultCallback#onSystemError} will be invoked with an - * {@link AppSearchException} if an error occurred in AppSearch initialization - * or a cause {@link Throwable} if other error occurred in AppSearch service. + * Or {@link BatchResultCallback#onSystemError} will be invoked with a + * {@link Throwable} if an unexpected internal error occurred in AppSearch + * service. */ public void putDocuments( @NonNull PutDocumentsRequest request, @@ -210,9 +209,9 @@ public final class AppSearchSession { * {@link AppSearchResult} otherwise. URIs that are not found will return a * failed {@link AppSearchResult} with a result code of * {@link AppSearchResult#RESULT_NOT_FOUND}. - * Or {@link BatchResultCallback#onSystemError} will be invoked with an - * {@link AppSearchException} if an error occurred in AppSearch initialization - * or a cause {@link Throwable} if other error occurred in AppSearch service. + * Or {@link BatchResultCallback#onSystemError} will be invoked with a + * {@link Throwable} if an unexpected internal error occurred in AppSearch + * service. */ public void getByUri( @NonNull GetByUriRequest request, @@ -338,9 +337,9 @@ public final class AppSearchSession { * are {@code null} on success, or a failed {@link AppSearchResult} otherwise. * URIs that are not found will return a failed {@link AppSearchResult} with a * result code of {@link AppSearchResult#RESULT_NOT_FOUND}. - * Or {@link BatchResultCallback#onSystemError} will be invoked with an - * {@link AppSearchException} if an error occurred in AppSearch initialization - * or a cause {@link Throwable} if other error occurred in AppSearch service. + * Or {@link BatchResultCallback#onSystemError} will be invoked with a + * {@link Throwable} if an unexpected internal error occurred in AppSearch + * service. */ public void removeByUri( @NonNull RemoveByUriRequest request, diff --git a/apex/appsearch/service/java/com/android/server/appsearch/AppSearchManagerService.java b/apex/appsearch/service/java/com/android/server/appsearch/AppSearchManagerService.java index 551347c5c202e..53d097e844c8f 100644 --- a/apex/appsearch/service/java/com/android/server/appsearch/AppSearchManagerService.java +++ b/apex/appsearch/service/java/com/android/server/appsearch/AppSearchManagerService.java @@ -77,7 +77,7 @@ public class AppSearchManagerService extends SystemService { for (int i = 0; i < schemaBundles.size(); i++) { schemas.add(new AppSearchSchema(schemaBundles.get(i))); } - AppSearchImpl impl = ImplInstanceManager.getInstance(getContext(), callingUserId); + AppSearchImpl impl = ImplInstanceManager.getInstance(callingUserId); databaseName = rewriteDatabaseNameWithUid(databaseName, callingUid); impl.setSchema(databaseName, schemas, schemasNotPlatformSurfaceable, forceOverride); invokeCallbackOnResult(callback, @@ -103,7 +103,7 @@ public class AppSearchManagerService extends SystemService { try { AppSearchBatchResult.Builder resultBuilder = new AppSearchBatchResult.Builder<>(); - AppSearchImpl impl = ImplInstanceManager.getInstance(getContext(), callingUserId); + AppSearchImpl impl = ImplInstanceManager.getInstance(callingUserId); databaseName = rewriteDatabaseNameWithUid(databaseName, callingUid); for (int i = 0; i < documentBundles.size(); i++) { GenericDocument document = new GenericDocument(documentBundles.get(i)); @@ -138,7 +138,7 @@ public class AppSearchManagerService extends SystemService { try { AppSearchBatchResult.Builder resultBuilder = new AppSearchBatchResult.Builder<>(); - AppSearchImpl impl = ImplInstanceManager.getInstance(getContext(), callingUserId); + AppSearchImpl impl = ImplInstanceManager.getInstance(callingUserId); databaseName = rewriteDatabaseNameWithUid(databaseName, callingUid); for (int i = 0; i < uris.size(); i++) { String uri = uris.get(i); @@ -172,7 +172,7 @@ public class AppSearchManagerService extends SystemService { int callingUserId = UserHandle.getUserId(callingUid); final long callingIdentity = Binder.clearCallingIdentity(); try { - AppSearchImpl impl = ImplInstanceManager.getInstance(getContext(), callingUserId); + AppSearchImpl impl = ImplInstanceManager.getInstance(callingUserId); databaseName = rewriteDatabaseNameWithUid(databaseName, callingUid); SearchResultPage searchResultPage = impl.query( databaseName, @@ -198,7 +198,7 @@ public class AppSearchManagerService extends SystemService { int callingUserId = UserHandle.getUserId(callingUid); final long callingIdentity = Binder.clearCallingIdentity(); try { - AppSearchImpl impl = ImplInstanceManager.getInstance(getContext(), callingUserId); + AppSearchImpl impl = ImplInstanceManager.getInstance(callingUserId); SearchResultPage searchResultPage = impl.globalQuery( queryExpression, new SearchSpec(searchSpecBundle)); @@ -221,7 +221,7 @@ public class AppSearchManagerService extends SystemService { // TODO(b/162450968) check nextPageToken is being advanced by the same uid as originally // opened it try { - AppSearchImpl impl = ImplInstanceManager.getInstance(getContext(), callingUserId); + AppSearchImpl impl = ImplInstanceManager.getInstance(callingUserId); SearchResultPage searchResultPage = impl.getNextPage(nextPageToken); invokeCallbackOnResult(callback, AppSearchResult.newSuccessfulResult(searchResultPage.getBundle())); @@ -238,7 +238,7 @@ public class AppSearchManagerService extends SystemService { int callingUserId = UserHandle.getUserId(callingUid); final long callingIdentity = Binder.clearCallingIdentity(); try { - AppSearchImpl impl = ImplInstanceManager.getInstance(getContext(), callingUserId); + AppSearchImpl impl = ImplInstanceManager.getInstance(callingUserId); impl.invalidateNextPageToken(nextPageToken); } catch (Throwable t) { Log.d(TAG, "Unable to invalidate the query page token", t); @@ -257,10 +257,10 @@ public class AppSearchManagerService extends SystemService { int callingUid = Binder.getCallingUidOrThrow(); int callingUserId = UserHandle.getUserId(callingUid); final long callingIdentity = Binder.clearCallingIdentity(); - AppSearchBatchResult.Builder resultBuilder = - new AppSearchBatchResult.Builder<>(); try { - AppSearchImpl impl = ImplInstanceManager.getInstance(getContext(), callingUserId); + AppSearchBatchResult.Builder resultBuilder = + new AppSearchBatchResult.Builder<>(); + AppSearchImpl impl = ImplInstanceManager.getInstance(callingUserId); databaseName = rewriteDatabaseNameWithUid(databaseName, callingUid); for (int i = 0; i < uris.size(); i++) { String uri = uris.get(i); @@ -293,7 +293,7 @@ public class AppSearchManagerService extends SystemService { int callingUserId = UserHandle.getUserId(callingUid); final long callingIdentity = Binder.clearCallingIdentity(); try { - AppSearchImpl impl = ImplInstanceManager.getInstance(getContext(), callingUserId); + AppSearchImpl impl = ImplInstanceManager.getInstance(callingUserId); databaseName = rewriteDatabaseNameWithUid(databaseName, callingUid); impl.removeByQuery(databaseName, queryExpression, new SearchSpec(searchSpecBundle)); @@ -312,7 +312,7 @@ public class AppSearchManagerService extends SystemService { int callingUserId = UserHandle.getUserId(callingUid); final long callingIdentity = Binder.clearCallingIdentity(); try { - ImplInstanceManager.getInstance(getContext(), callingUserId); + ImplInstanceManager.getOrCreateInstance(getContext(), callingUserId); invokeCallbackOnResult(callback, AppSearchResult.newSuccessfulResult(null)); } catch (Throwable t) { invokeCallbackOnError(callback, t); @@ -374,13 +374,14 @@ public class AppSearchManagerService extends SystemService { } /** - * Invokes the {@link IAppSearchBatchResultCallback} with an throwable. + * Invokes the {@link IAppSearchBatchResultCallback} with an unexpected internal throwable. * *

The throwable is converted to {@link ParcelableException}. */ private void invokeCallbackOnError(IAppSearchBatchResultCallback callback, Throwable throwable) { try { + //TODO(b/175067650) verify ParcelableException could propagate throwable correctly. callback.onSystemError(new ParcelableException(throwable)); } catch (RemoteException e) { Log.d(TAG, "Unable to send error to the callback", e); diff --git a/apex/appsearch/service/java/com/android/server/appsearch/ImplInstanceManager.java b/apex/appsearch/service/java/com/android/server/appsearch/ImplInstanceManager.java index 2871eb622f114..d26269132c5ab 100644 --- a/apex/appsearch/service/java/com/android/server/appsearch/ImplInstanceManager.java +++ b/apex/appsearch/service/java/com/android/server/appsearch/ImplInstanceManager.java @@ -41,7 +41,7 @@ public final class ImplInstanceManager { private ImplInstanceManager() {} /** - * Gets an instance of AppSearchImpl for the given user. + * Gets an instance of AppSearchImpl for the given user, or creates one if none exists. * *

If no AppSearchImpl instance exists for this user, Icing will be initialized and one will * be created. @@ -51,7 +51,7 @@ public final class ImplInstanceManager { * @return An initialized {@link AppSearchImpl} for this user */ @NonNull - public static AppSearchImpl getInstance(@NonNull Context context, @UserIdInt int userId) + public static AppSearchImpl getOrCreateInstance(@NonNull Context context, @UserIdInt int userId) throws AppSearchException { AppSearchImpl instance = sInstances.get(userId); if (instance == null) { @@ -66,6 +66,28 @@ public final class ImplInstanceManager { return instance; } + /** + * Gets an instance of AppSearchImpl for the given user. + * + *

This method should only be called by an initialized SearchSession, which has been already + * created the AppSearchImpl instance for the given user. + * + * @param userId The multi-user userId of the device user calling AppSearch + * @return An initialized {@link AppSearchImpl} for this user + */ + @NonNull + public static AppSearchImpl getInstance(@UserIdInt int userId) { + AppSearchImpl instance = sInstances.get(userId); + if (instance == null) { + // Impossible scenario, user cannot call an uninitialized SearchSession, + // getInstance should always find the instance for the given user and never try to + // create an instance for this user again. + throw new IllegalStateException( + "AppSearchImpl has never been created for this user: " + userId); + } + return instance; + } + private static AppSearchImpl createImpl(@NonNull Context context, @UserIdInt int userId) throws AppSearchException { File appSearchDir = getAppSearchDir(context, userId);