From 0dd233f3849e4b3d05407ad0166168e3bcad08c9 Mon Sep 17 00:00:00 2001 From: Alexander Dorokhine Date: Thu, 8 Oct 2020 13:43:24 -0700 Subject: [PATCH] Merge over AppSearchResult, AppSearchBatchResult and setSchema error improvements. Bug: 162450968 Test: AppSearchSchemaTest#testRemoveSchema Change-Id: I724dc01e24acc45f10b416eb4b6728733a4b2b1b --- .../app/appsearch/AppSearchBatchResult.java | 51 +++++-- .../app/appsearch/AppSearchResult.java | 32 +++-- .../exceptions/AppSearchException.java | 4 + .../server/appsearch/ImplInstanceManager.java | 4 +- .../external/localbackend/AppSearchImpl.java | 135 +++++++----------- .../localbackend/AppSearchImplTest.java | 6 +- 6 files changed, 119 insertions(+), 113 deletions(-) diff --git a/apex/appsearch/framework/java/android/app/appsearch/AppSearchBatchResult.java b/apex/appsearch/framework/java/android/app/appsearch/AppSearchBatchResult.java index dc758253f1a4b..98daa66183a3a 100644 --- a/apex/appsearch/framework/java/android/app/appsearch/AppSearchBatchResult.java +++ b/apex/appsearch/framework/java/android/app/appsearch/AppSearchBatchResult.java @@ -1,5 +1,5 @@ /* - * Copyright (C) 2020 The Android Open Source Project + * Copyright 2020 The Android Open Source Project * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -22,6 +22,8 @@ import android.os.Parcel; import android.os.Parcelable; import android.util.ArrayMap; +import com.android.internal.util.Preconditions; + import java.util.Collections; import java.util.Map; @@ -33,11 +35,11 @@ import java.util.Map; * @param The type of result objects associated with the keys. * @hide */ -public class AppSearchBatchResult implements Parcelable { +public final class AppSearchBatchResult implements Parcelable { @NonNull private final Map mSuccesses; @NonNull private final Map> mFailures; - private AppSearchBatchResult( + AppSearchBatchResult( @NonNull Map successes, @NonNull Map> failures) { mSuccesses = successes; @@ -61,8 +63,8 @@ public class AppSearchBatchResult implements Parcelable { } /** - * Returns a {@link Map} of all successful keys mapped to the successful {@link ValueType} - * values they produced. + * Returns a {@link Map} of all successful keys mapped to the successful + * {@link AppSearchResult}s they produced. * *

The values of the {@link Map} will not be {@code null}. */ @@ -82,6 +84,22 @@ public class AppSearchBatchResult implements Parcelable { return mFailures; } + /** + * Asserts that this {@link AppSearchBatchResult} has no failures. + * @hide + */ + public void checkSuccess() { + if (!isSuccess()) { + throw new IllegalStateException("AppSearchBatchResult has failures: " + this); + } + } + + @Override + @NonNull + public String toString() { + return "{\n successes: " + mSuccesses + "\n failures: " + mFailures + "\n}"; + } + @Override public int describeContents() { return 0; @@ -112,16 +130,18 @@ public class AppSearchBatchResult implements Parcelable { public static final class Builder { private final Map mSuccesses = new ArrayMap<>(); private final Map> mFailures = new ArrayMap<>(); - - /** Creates a new {@link Builder} for this {@link AppSearchBatchResult}. */ - public Builder() {} + private boolean mBuilt = false; /** * Associates the {@code key} with the given successful return value. * *

Any previous mapping for a key, whether success or failure, is deleted. */ - public Builder setSuccess(@NonNull KeyType key, @Nullable ValueType result) { + @NonNull + public Builder setSuccess( + @NonNull KeyType key, @Nullable ValueType result) { + Preconditions.checkState(!mBuilt, "Builder has already been used"); + Preconditions.checkNotNull(key); return setResult(key, AppSearchResult.newSuccessfulResult(result)); } @@ -130,10 +150,13 @@ public class AppSearchBatchResult implements Parcelable { * *

Any previous mapping for a key, whether success or failure, is deleted. */ - public Builder setFailure( + @NonNull + public Builder setFailure( @NonNull KeyType key, @AppSearchResult.ResultCode int resultCode, @Nullable String errorMessage) { + Preconditions.checkState(!mBuilt, "Builder has already been used"); + Preconditions.checkNotNull(key); return setResult(key, AppSearchResult.newFailedResult(resultCode, errorMessage)); } @@ -143,7 +166,11 @@ public class AppSearchBatchResult implements Parcelable { *

Any previous mapping for a key, whether success or failure, is deleted. */ @NonNull - public Builder setResult(@NonNull KeyType key, @NonNull AppSearchResult result) { + public Builder setResult( + @NonNull KeyType key, @NonNull AppSearchResult result) { + Preconditions.checkState(!mBuilt, "Builder has already been used"); + Preconditions.checkNotNull(key); + Preconditions.checkNotNull(result); if (result.isSuccess()) { mSuccesses.put(key, result.getResultValue()); mFailures.remove(key); @@ -157,6 +184,8 @@ public class AppSearchBatchResult implements Parcelable { /** Builds an {@link AppSearchBatchResult} from the contents of this {@link Builder}. */ @NonNull public AppSearchBatchResult build() { + Preconditions.checkState(!mBuilt, "Builder has already been used"); + mBuilt = true; return new AppSearchBatchResult<>(mSuccesses, mFailures); } } diff --git a/apex/appsearch/framework/java/android/app/appsearch/AppSearchResult.java b/apex/appsearch/framework/java/android/app/appsearch/AppSearchResult.java index 7f3834852edad..979eab90a980c 100644 --- a/apex/appsearch/framework/java/android/app/appsearch/AppSearchResult.java +++ b/apex/appsearch/framework/java/android/app/appsearch/AppSearchResult.java @@ -1,5 +1,5 @@ /* - * Copyright (C) 2020 The Android Open Source Project + * Copyright 2020 The Android Open Source Project * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -32,9 +32,12 @@ import java.util.Objects; * @param The type of result object for successful calls. * @hide */ -public class AppSearchResult implements Parcelable { - /** Result codes from {@link AppSearchManager} methods. */ - @IntDef(prefix = {"RESULT_"}, value = { +public final class AppSearchResult implements Parcelable { + /** + * Result codes from {@link AppSearchManager} methods. + * @hide + */ + @IntDef(value = { RESULT_OK, RESULT_UNKNOWN_ERROR, RESULT_INTERNAL_ERROR, @@ -120,15 +123,18 @@ public class AppSearchResult implements Parcelable { } /** - * Returns the returned value associated with this result. + * Returns the result value associated with this result, if it was successful. * - *

If {@link #isSuccess} is {@code false}, the result value is always {@code null}. The value - * may be {@code null} even if {@link #isSuccess} is {@code true}. See the documentation of the - * particular {@link AppSearchManager} call producing this {@link AppSearchResult} for what is - * returned by {@link #getResultValue}. + *

See the documentation of the particular {@link AppSearchManager} call producing this + * {@link AppSearchResult} for what is placed in the result value by that call. + * + * @throws IllegalStateException if this {@link AppSearchResult} is not successful. */ @Nullable public ValueType getResultValue() { + if (!isSuccess()) { + throw new IllegalStateException("AppSearchResult is a failure: " + this); + } return mResultValue; } @@ -146,14 +152,14 @@ public class AppSearchResult implements Parcelable { } @Override - public boolean equals(Object other) { + public boolean equals(@Nullable Object other) { if (this == other) { return true; } if (!(other instanceof AppSearchResult)) { return false; } - AppSearchResult otherResult = (AppSearchResult) other; + AppSearchResult otherResult = (AppSearchResult) other; return mResultCode == otherResult.mResultCode && Objects.equals(mResultValue, otherResult.mResultValue) && Objects.equals(mErrorMessage, otherResult.mErrorMessage); @@ -168,9 +174,9 @@ public class AppSearchResult implements Parcelable { @NonNull public String toString() { if (isSuccess()) { - return "AppSearchResult [SUCCESS]: " + mResultValue; + return "[SUCCESS]: " + mResultValue; } - return "AppSearchResult [FAILURE(" + mResultCode + ")]: " + mErrorMessage; + return "[FAILURE(" + mResultCode + ")]: " + mErrorMessage; } @Override diff --git a/apex/appsearch/framework/java/android/app/appsearch/exceptions/AppSearchException.java b/apex/appsearch/framework/java/android/app/appsearch/exceptions/AppSearchException.java index 00f6e75afe54e..d490469be3d60 100644 --- a/apex/appsearch/framework/java/android/app/appsearch/exceptions/AppSearchException.java +++ b/apex/appsearch/framework/java/android/app/appsearch/exceptions/AppSearchException.java @@ -54,6 +54,10 @@ public class AppSearchException extends Exception { mResultCode = resultCode; } + public @AppSearchResult.ResultCode int getResultCode() { + return mResultCode; + } + /** * Converts this {@link java.lang.Exception} into a failed {@link AppSearchResult} */ 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 c1e6b0fd205f1..60f7005a7c0a8 100644 --- a/apex/appsearch/service/java/com/android/server/appsearch/ImplInstanceManager.java +++ b/apex/appsearch/service/java/com/android/server/appsearch/ImplInstanceManager.java @@ -69,9 +69,7 @@ public final class ImplInstanceManager { private static AppSearchImpl createImpl(@NonNull Context context, @UserIdInt int userId) throws AppSearchException { File appSearchDir = getAppSearchDir(context, userId); - AppSearchImpl appSearchImpl = new AppSearchImpl(appSearchDir); - appSearchImpl.initialize(); - return appSearchImpl; + return AppSearchImpl.create(appSearchDir); } private static File getAppSearchDir(@NonNull Context context, @UserIdInt int userId) { diff --git a/apex/appsearch/service/java/com/android/server/appsearch/external/localbackend/AppSearchImpl.java b/apex/appsearch/service/java/com/android/server/appsearch/external/localbackend/AppSearchImpl.java index 462f45869b341..642378d992ace 100644 --- a/apex/appsearch/service/java/com/android/server/appsearch/external/localbackend/AppSearchImpl.java +++ b/apex/appsearch/service/java/com/android/server/appsearch/external/localbackend/AppSearchImpl.java @@ -18,7 +18,6 @@ package com.android.server.appsearch.external.localbackend; import android.util.Log; -import android.annotation.AnyThread; import com.android.internal.annotations.GuardedBy; import android.annotation.NonNull; import android.annotation.Nullable; @@ -27,6 +26,7 @@ import com.android.internal.annotations.VisibleForTesting; import android.annotation.WorkerThread; import android.app.appsearch.AppSearchResult; import android.app.appsearch.exceptions.AppSearchException; +import com.android.internal.util.Preconditions; import com.google.android.icing.IcingSearchEngine; import com.google.android.icing.proto.DeleteByNamespaceResultProto; @@ -58,7 +58,6 @@ import java.util.HashMap; import java.util.HashSet; import java.util.Map; import java.util.Set; -import java.util.concurrent.CountDownLatch; import java.util.concurrent.locks.ReadWriteLock; import java.util.concurrent.locks.ReentrantReadWriteLock; @@ -66,8 +65,7 @@ import java.util.concurrent.locks.ReentrantReadWriteLock; * Manages interaction with the native IcingSearchEngine and other components to implement AppSearch * functionality. * - *

Callers should call {@link #initialize} before using the AppSearchImpl instance. Never create - * two instances using the same folder. + *

Never create two instances using the same folder. * *

A single instance of {@link AppSearchImpl} can support all databases. Schemas and documents * are physically saved together in {@link IcingSearchEngine}, but logically isolated: @@ -106,9 +104,7 @@ public final class AppSearchImpl { static final int CHECK_OPTIMIZE_INTERVAL = 100; private final ReadWriteLock mReadWriteLock = new ReentrantReadWriteLock(); - private final CountDownLatch mInitCompleteLatch = new CountDownLatch(1); - private final File mIcingDir; - private IcingSearchEngine mIcingSearchEngine; + private final IcingSearchEngine mIcingSearchEngine; // The map contains schemaTypes and namespaces for all database. All values in the map have // been already added database name prefix. @@ -121,33 +117,24 @@ public final class AppSearchImpl { */ private int mOptimizeIntervalCount = 0; - /** Creates an instance of {@link AppSearchImpl} which writes data to the given folder. */ - @AnyThread - public AppSearchImpl(@NonNull File icingDir) { - mIcingDir = icingDir; + /** + * Creates and initializes an instance of {@link AppSearchImpl} which writes data to the given + * folder. + */ + @NonNull + public static AppSearchImpl create(@NonNull File icingDir) throws AppSearchException { + Preconditions.checkNotNull(icingDir); + return new AppSearchImpl(icingDir); } - /** - * Initializes the underlying IcingSearchEngine. - * - *

This method belongs to mutate group. - * - * @throws AppSearchException on IcingSearchEngine error. - */ - public void initialize() throws AppSearchException { - if (isInitialized()) { - return; - } + private AppSearchImpl(@NonNull File icingDir) throws AppSearchException { boolean isReset = false; mReadWriteLock.writeLock().lock(); try { - // We synchronize here because we don't want to call IcingSearchEngine.initialize() more - // than once. It's unnecessary and can be a costly operation. - if (isInitialized()) { - return; - } + // We synchronize here because we don't want to call IcingSearchEngine.initialize() more + // than once. It's unnecessary and can be a costly operation. IcingSearchEngineOptions options = IcingSearchEngineOptions.newBuilder() - .setBaseDir(mIcingDir.getAbsolutePath()).build(); + .setBaseDir(icingDir.getAbsolutePath()).build(); mIcingSearchEngine = new IcingSearchEngine(options); InitializeResultProto initializeResultProto = mIcingSearchEngine.initialize(); @@ -170,7 +157,8 @@ public final class AppSearchImpl { for (String qualifiedNamespace : getAllNamespacesResultProto.getNamespacesList()) { addToMap(mNamespaceMap, getDatabaseName(qualifiedNamespace), qualifiedNamespace); } - mInitCompleteLatch.countDown(); + // TODO(b/155939114): It's possible to optimize after init, which would reduce the time + // to when we're able to serve queries. Consider moving this optimize call out. if (!isReset) { checkForOptimize(/* force= */ true); } @@ -179,12 +167,6 @@ public final class AppSearchImpl { } } - /** Checks if the internal state of {@link AppSearchImpl} has been initialized. */ - @AnyThread - public boolean isInitialized() { - return mInitCompleteLatch.getCount() == 0; - } - /** * Updates the AppSearch schema for this app. * @@ -195,12 +177,9 @@ public final class AppSearchImpl { * @param forceOverride Whether to force-apply the schema even if it is incompatible. Documents * which do not comply with the new schema will be deleted. * @throws AppSearchException on IcingSearchEngine error. - * @throws InterruptedException if the current thread was interrupted during execution. */ public void setSchema(@NonNull String databaseName, @NonNull SchemaProto origSchema, - boolean forceOverride) throws AppSearchException, InterruptedException { - awaitInitialized(); - + boolean forceOverride) throws AppSearchException { SchemaProto schemaProto = getSchemaProto(); SchemaProto.Builder existingSchemaBuilder = schemaProto.toBuilder(); @@ -212,10 +191,32 @@ public final class AppSearchImpl { SetSchemaResultProto setSchemaResultProto; mReadWriteLock.writeLock().lock(); try { - setSchemaResultProto = mIcingSearchEngine.setSchema(existingSchemaBuilder.build(), - forceOverride); - checkSuccess(setSchemaResultProto.getStatus()); + // Apply schema + setSchemaResultProto = + mIcingSearchEngine.setSchema(existingSchemaBuilder.build(), forceOverride); + + // Determine whether it succeeded. + try { + checkSuccess(setSchemaResultProto.getStatus()); + } catch (AppSearchException e) { + // Improve the error message by merging in information about incompatible types. + if (setSchemaResultProto.getDeletedSchemaTypesCount() > 0 + || setSchemaResultProto.getIncompatibleSchemaTypesCount() > 0) { + String newMessage = e.getMessage() + + "\n Deleted types: " + + setSchemaResultProto.getDeletedSchemaTypesList() + + "\n Incompatible types: " + + setSchemaResultProto.getIncompatibleSchemaTypesList(); + throw new AppSearchException(e.getResultCode(), newMessage, e.getCause()); + } else { + throw e; + } + } + + // Update derived data structures. mSchemaMap.put(databaseName, newTypeNames); + + // Determine whether to schedule an immediate optimize. if (setSchemaResultProto.getDeletedSchemaTypesCount() > 0 || (setSchemaResultProto.getIncompatibleSchemaTypesCount() > 0 && forceOverride)) { @@ -237,12 +238,9 @@ public final class AppSearchImpl { * @param databaseName The databaseName this document resides in. * @param document The document to index. * @throws AppSearchException on IcingSearchEngine error. - * @throws InterruptedException if the current thread was interrupted during execution. */ public void putDocument(@NonNull String databaseName, @NonNull DocumentProto document) - throws AppSearchException, InterruptedException { - awaitInitialized(); - + throws AppSearchException { DocumentProto.Builder documentBuilder = document.toBuilder(); rewriteDocumentTypes(getDatabasePrefix(databaseName), documentBuilder, /*add=*/ true); @@ -270,12 +268,10 @@ public final class AppSearchImpl { * @param uri The URI of the document to get. * @return The Document contents, or {@code null} if no such URI exists in the system. * @throws AppSearchException on IcingSearchEngine error. - * @throws InterruptedException if the current thread was interrupted during execution. */ @Nullable public DocumentProto getDocument(@NonNull String databaseName, @NonNull String namespace, - @NonNull String uri) throws AppSearchException, InterruptedException { - awaitInitialized(); + @NonNull String uri) throws AppSearchException { GetResultProto getResultProto; mReadWriteLock.readLock().lock(); try { @@ -303,16 +299,13 @@ public final class AppSearchImpl { * @return The results of performing this search The proto might have no {@code results} if no * documents matched the query. * @throws AppSearchException on IcingSearchEngine error. - * @throws InterruptedException if the current thread was interrupted during execution. */ @NonNull public SearchResultProto query( @NonNull String databaseName, @NonNull SearchSpecProto searchSpec, @NonNull ResultSpecProto resultSpec, - @NonNull ScoringSpecProto scoringSpec) throws AppSearchException, InterruptedException { - awaitInitialized(); - + @NonNull ScoringSpecProto scoringSpec) throws AppSearchException { SearchSpecProto.Builder searchSpecBuilder = searchSpec.toBuilder(); SearchResultProto searchResultProto; mReadWriteLock.readLock().lock(); @@ -347,13 +340,10 @@ public final class AppSearchImpl { * @param nextPageToken The token of pre-loaded results of previously executed query. * @return The next page of results of previously executed query. * @throws AppSearchException on IcingSearchEngine error. - * @throws InterruptedException if the current thread was interrupted during execution. */ @NonNull public SearchResultProto getNextPage(@NonNull String databaseName, long nextPageToken) - throws AppSearchException, InterruptedException { - awaitInitialized(); - + throws AppSearchException { SearchResultProto searchResultProto = mIcingSearchEngine.getNextPage(nextPageToken); checkSuccess(searchResultProto.getStatus()); if (searchResultProto.getResultsCount() == 0) { @@ -367,8 +357,7 @@ public final class AppSearchImpl { * @param nextPageToken The token of pre-loaded results of previously executed query to be * Invalidated. */ - public void invalidateNextPageToken(long nextPageToken) throws InterruptedException { - awaitInitialized(); + public void invalidateNextPageToken(long nextPageToken) { mIcingSearchEngine.invalidateNextPageToken(nextPageToken); } @@ -381,12 +370,9 @@ public final class AppSearchImpl { * @param namespace Namespace of the document to remove. * @param uri URI of the document to remove. * @throws AppSearchException on IcingSearchEngine error. - * @throws InterruptedException if the current thread was interrupted during execution. */ public void remove(@NonNull String databaseName, @NonNull String namespace, - @NonNull String uri) throws AppSearchException, InterruptedException { - awaitInitialized(); - + @NonNull String uri) throws AppSearchException { String qualifiedNamespace = getDatabasePrefix(databaseName) + namespace; DeleteResultProto deleteResultProto; mReadWriteLock.writeLock().lock(); @@ -407,12 +393,9 @@ public final class AppSearchImpl { * @param databaseName The databaseName that contains documents of schemaType. * @param schemaType The schemaType of documents to remove. * @throws AppSearchException on IcingSearchEngine error. - * @throws InterruptedException if the current thread was interrupted during execution. */ public void removeByType(@NonNull String databaseName, @NonNull String schemaType) - throws AppSearchException, InterruptedException { - awaitInitialized(); - + throws AppSearchException { String qualifiedType = getDatabasePrefix(databaseName) + schemaType; DeleteBySchemaTypeResultProto deleteBySchemaTypeResultProto; mReadWriteLock.writeLock().lock(); @@ -437,12 +420,9 @@ public final class AppSearchImpl { * @param databaseName The databaseName that contains documents of namespace. * @param namespace The namespace of documents to remove. * @throws AppSearchException on IcingSearchEngine error. - * @throws InterruptedException if the current thread was interrupted during execution. */ public void removeByNamespace(@NonNull String databaseName, @NonNull String namespace) - throws AppSearchException, InterruptedException { - awaitInitialized(); - + throws AppSearchException { String qualifiedNamespace = getDatabasePrefix(databaseName) + namespace; DeleteByNamespaceResultProto deleteByNamespaceResultProto; mReadWriteLock.writeLock().lock(); @@ -469,11 +449,9 @@ public final class AppSearchImpl { * * @param databaseName The databaseName to remove all documents from. * @throws AppSearchException on IcingSearchEngine error. - * @throws InterruptedException if the current thread was interrupted during execution. */ public void removeAll(@NonNull String databaseName) - throws AppSearchException, InterruptedException { - awaitInitialized(); + throws AppSearchException { mReadWriteLock.writeLock().lock(); try { Set existingNamespaces = mNamespaceMap.get(databaseName); @@ -732,15 +710,6 @@ public final class AppSearchImpl { values.add(prefixedValue); } - /** - * Waits for the instance to become initialized. - * - * @throws InterruptedException if the current thread was interrupted during waiting. - */ - private void awaitInitialized() throws InterruptedException { - mInitCompleteLatch.await(); - } - /** * Checks the given status code and throws an {@link AppSearchException} if code is an error. * diff --git a/services/tests/servicestests/src/com/android/server/appsearch/external/localbackend/AppSearchImplTest.java b/services/tests/servicestests/src/com/android/server/appsearch/external/localbackend/AppSearchImplTest.java index 24f78308c6008..081bfb5b2724d 100644 --- a/services/tests/servicestests/src/com/android/server/appsearch/external/localbackend/AppSearchImplTest.java +++ b/services/tests/servicestests/src/com/android/server/appsearch/external/localbackend/AppSearchImplTest.java @@ -50,8 +50,7 @@ public class AppSearchImplTest { @Before public void setUp() throws Exception { - mAppSearchImpl = new AppSearchImpl(mTemporaryFolder.newFolder()); - mAppSearchImpl.initialize(); + mAppSearchImpl = AppSearchImpl.create(mTemporaryFolder.newFolder()); } /** @@ -288,7 +287,8 @@ public class AppSearchImplTest { SchemaProto finalSchemaProto = schemaProto; AppSearchException e = expectThrows(AppSearchException.class, () -> mAppSearchImpl.setSchema("database1", finalSchemaProto, /*forceOverride=*/false)); - assertThat(e).hasMessageThat().isEqualTo("Schema is incompatible."); + assertThat(e).hasMessageThat().contains("Schema is incompatible"); + assertThat(e).hasMessageThat().contains("Deleted types: [database1/Document]"); // ForceOverride to delete. mAppSearchImpl.setSchema("database1", finalSchemaProto, /*forceOverride=*/true);