OSDN Git Service

Relax mapping rule to make the mapping logic simple.
authorDaichi Hirono <hirono@google.com>
Sun, 7 Feb 2016 04:20:22 +0000 (13:20 +0900)
committerDaichi Hirono <hirono@google.com>
Tue, 9 Feb 2016 18:36:27 +0000 (18:36 +0000)
MtpDocumentsProvider remembers the mapping between SAF's ID and MTP's
ID. Sometimes we need to do heuristic to restore the mapping when MTP
device is reconnected.

Previously we do the mapping files that shares the same name more
strictly. For example,

1. Found file name "test.txt". Assign document ID "1".
2. MTP device is disconnected and the MTP ID of "1" is lost.
3. Found two files that have same name "test.txt" in the same directory.

Previously we don't reuse existing document ID "1" for neither of two
"test.txt" because it's not 1-to-1 mapping and we cannot determine which
one should be mapped with existing document ID. It means we need the
complete list of files in a directory to remap IDs. It takes long time
to fetch all file names in a directory when a directory has 100+
files. It's rare that a MTP device has the two files sharing the same
name in the same directory. Also the strict rule makes the mapping code
more complex.

The CL relax the rule of mapping, and it allows to reuse existing
document ID even if it is not 1-to-1 mapping. For the previous example,
it assigns "1" for either of "test.txt".

BUG=27053734
Change-Id: I19406fafc21f13ab94ba99411ce5e7f55ce7f658
(cherry picked from commit acdbc6e740ffbd465488b6eb0cf9388d43ae860a)

packages/MtpDocumentsProvider/src/com/android/mtp/Mapper.java
packages/MtpDocumentsProvider/src/com/android/mtp/MtpDatabaseConstants.java
packages/MtpDocumentsProvider/tests/src/com/android/mtp/MtpDatabaseTest.java

index 3faa8f4..6af492c 100644 (file)
@@ -73,7 +73,6 @@ class Mapper {
                     extraValuesList,
                     COLUMN_PARENT_DOCUMENT_ID + " IS NULL",
                     EMPTY_ARGS,
-                    /* heuristic */ false,
                     COLUMN_DEVICE_ID);
             database.setTransactionSuccessful();
             return changed;
@@ -92,16 +91,13 @@ class Mapper {
         final SQLiteDatabase database = mDatabase.getSQLiteDatabase();
         database.beginTransaction();
         try {
-            final boolean heuristic;
             final String mapColumn;
             Preconditions.checkState(mMappingMode.containsKey(parentDocumentId));
             switch (mMappingMode.get(parentDocumentId)) {
                 case MAP_BY_MTP_IDENTIFIER:
-                    heuristic = false;
                     mapColumn = COLUMN_STORAGE_ID;
                     break;
                 case MAP_BY_NAME:
-                    heuristic = true;
                     mapColumn = Document.COLUMN_DISPLAY_NAME;
                     break;
                 default:
@@ -120,7 +116,6 @@ class Mapper {
                     extraValuesList,
                     COLUMN_PARENT_DOCUMENT_ID + "=?",
                     strings(parentDocumentId),
-                    heuristic,
                     mapColumn);
 
             database.setTransactionSuccessful();
@@ -137,16 +132,13 @@ class Mapper {
      * @param documents List of document information.
      */
     synchronized void putChildDocuments(int deviceId, String parentId, MtpObjectInfo[] documents) {
-        final boolean heuristic;
         final String mapColumn;
         Preconditions.checkState(mMappingMode.containsKey(parentId));
         switch (mMappingMode.get(parentId)) {
             case MAP_BY_MTP_IDENTIFIER:
-                heuristic = false;
                 mapColumn = COLUMN_OBJECT_HANDLE;
                 break;
             case MAP_BY_NAME:
-                heuristic = true;
                 mapColumn = Document.COLUMN_DISPLAY_NAME;
                 break;
             default:
@@ -163,17 +155,13 @@ class Mapper {
                 null,
                 COLUMN_PARENT_DOCUMENT_ID + "=?",
                 strings(parentId),
-                heuristic,
                 mapColumn);
     }
 
-    @VisibleForTesting
     void clearMapping() {
         final SQLiteDatabase database = mDatabase.getSQLiteDatabase();
         database.beginTransaction();
         try {
-            mDatabase.deleteDocumentsAndRootsRecursively(
-                    COLUMN_ROW_STATE + " = ?", strings(ROW_STATE_PENDING));
             final ContentValues values = new ContentValues();
             values.putNull(COLUMN_OBJECT_HANDLE);
             values.putNull(COLUMN_STORAGE_ID);
@@ -209,11 +197,6 @@ class Mapper {
         final SQLiteDatabase database = mDatabase.getSQLiteDatabase();
         database.beginTransaction();
         try {
-            // Delete all pending rows.
-            mDatabase.deleteDocumentsAndRootsRecursively(
-                    selection + " AND " + COLUMN_ROW_STATE + "=?",
-                    DatabaseUtils.appendSelectionArgs(args, strings(ROW_STATE_PENDING)));
-
             // Set all documents as invalidated.
             final ContentValues values = new ContentValues();
             values.put(COLUMN_ROW_STATE, ROW_STATE_INVALIDATED);
@@ -245,7 +228,6 @@ class Mapper {
      * @param rootExtraValuesList Values for root extra to be stored in the database.
      * @param selection SQL where closure to select rows that shares the same parent.
      * @param args Argument for selection SQL.
-     * @param heuristic Whether the mapping mode is heuristic.
      * @return Whether the method adds new rows.
      */
     private boolean putDocuments(
@@ -253,7 +235,6 @@ class Mapper {
             @Nullable ContentValues[] rootExtraValuesList,
             String selection,
             String[] args,
-            boolean heuristic,
             String mappingKey) {
         final SQLiteDatabase database = mDatabase.getSQLiteDatabase();
         boolean added = false;
@@ -285,7 +266,7 @@ class Mapper {
                     if (candidateCursor.getCount() == 0) {
                         rowId = database.insert(TABLE_DOCUMENTS, null, values);
                         added = true;
-                    } else if (!heuristic) {
+                    } else {
                         candidateCursor.moveToNext();
                         rowId = candidateCursor.getLong(0);
                         database.update(
@@ -293,9 +274,6 @@ class Mapper {
                                 values,
                                 SELECTION_DOCUMENT_ID,
                                 strings(rowId));
-                    } else {
-                        values.put(COLUMN_ROW_STATE, ROW_STATE_PENDING);
-                        rowId = database.insertOrThrow(TABLE_DOCUMENTS, null, values);
                     }
                     // Document ID is a primary integer key of the table. So the returned row
                     // IDs should be same with the document ID.
@@ -334,84 +312,10 @@ class Mapper {
             selection = COLUMN_PARENT_DOCUMENT_ID + " IS NULL";
             args = EMPTY_ARGS;
         }
-        final String groupKey;
-        switch (mMappingMode.get(parentId)) {
-            case MAP_BY_MTP_IDENTIFIER:
-                groupKey = parentId != null ? COLUMN_OBJECT_HANDLE : COLUMN_STORAGE_ID;
-                break;
-            case MAP_BY_NAME:
-                groupKey = Document.COLUMN_DISPLAY_NAME;
-                break;
-            default:
-                throw new Error("Unexpected mapping state.");
-        }
         mMappingMode.remove(parentId);
         final SQLiteDatabase database = mDatabase.getSQLiteDatabase();
         database.beginTransaction();
         try {
-            // Get 1-to-1 mapping of invalidated document and pending document.
-            final String invalidatedIdQuery = createStateFilter(
-                    ROW_STATE_INVALIDATED, Document.COLUMN_DOCUMENT_ID);
-            final String pendingIdQuery = createStateFilter(
-                    ROW_STATE_PENDING, Document.COLUMN_DOCUMENT_ID);
-            // SQL should be like:
-            // SELECT group_concat(CASE WHEN raw_state = 1 THEN document_id ELSE NULL END),
-            //        group_concat(CASE WHEN raw_state = 2 THEN document_id ELSE NULL END)
-            // WHERE device_id = ? AND parent_document_id IS NULL
-            // GROUP BY display_name
-            // HAVING count(CASE WHEN raw_state = 1 THEN document_id ELSE NULL END) = 1 AND
-            //        count(CASE WHEN raw_state = 2 THEN document_id ELSE NULL END) = 1
-            final Cursor mergingCursor = database.query(
-                    TABLE_DOCUMENTS,
-                    new String[] {
-                            "group_concat(" + invalidatedIdQuery + ")",
-                            "group_concat(" + pendingIdQuery + ")"
-                    },
-                    selection,
-                    args,
-                    groupKey,
-                    "count(" + invalidatedIdQuery + ") = 1 AND count(" + pendingIdQuery + ") = 1",
-                    null);
-
-            final ContentValues values = new ContentValues();
-            while (mergingCursor.moveToNext()) {
-                final String invalidatedId = mergingCursor.getString(0);
-                final String pendingId = mergingCursor.getString(1);
-
-                // Obtain the new values including the latest object handle from mapping row.
-                getFirstRow(
-                        TABLE_DOCUMENTS,
-                        SELECTION_DOCUMENT_ID,
-                        new String[] { pendingId },
-                        values);
-                values.remove(Document.COLUMN_DOCUMENT_ID);
-                values.put(COLUMN_ROW_STATE, ROW_STATE_VALID);
-                database.update(
-                        TABLE_DOCUMENTS,
-                        values,
-                        SELECTION_DOCUMENT_ID,
-                        new String[] { invalidatedId });
-
-                getFirstRow(
-                        TABLE_ROOT_EXTRA,
-                        SELECTION_ROOT_ID,
-                        new String[] { pendingId },
-                        values);
-                if (values.size() > 0) {
-                    values.remove(Root.COLUMN_ROOT_ID);
-                    database.update(
-                            TABLE_ROOT_EXTRA,
-                            values,
-                            SELECTION_ROOT_ID,
-                            new String[] { invalidatedId });
-                }
-
-                // Delete 'pending' row.
-                mDatabase.deleteDocumentsAndRootsRecursively(
-                        SELECTION_DOCUMENT_ID, new String[] { pendingId });
-            }
-            mergingCursor.close();
-
             boolean changed = false;
 
             // Delete all invalidated rows that cannot be mapped.
@@ -421,58 +325,10 @@ class Mapper {
                 changed = true;
             }
 
-            // The database cannot find old document ID for the pending rows.
-            // Turn the all pending rows into valid state, which means the rows become to be
-            // valid with new document ID.
-            values.clear();
-            values.put(COLUMN_ROW_STATE, ROW_STATE_VALID);
-            if (database.update(
-                    TABLE_DOCUMENTS,
-                    values,
-                    COLUMN_ROW_STATE + " = ? AND " + selection,
-                    DatabaseUtils.appendSelectionArgs(strings(ROW_STATE_PENDING), args)) != 0) {
-                changed = true;
-            }
             database.setTransactionSuccessful();
             return changed;
         } finally {
             database.endTransaction();
         }
     }
-
-    /**
-     * Obtains values of the first row for the query.
-     * @param values ContentValues that the values are stored to.
-     * @param table Target table.
-     * @param selection Query to select rows.
-     * @param args Argument for query.
-     */
-    private void getFirstRow(String table, String selection, String[] args, ContentValues values) {
-        final SQLiteDatabase database = mDatabase.getSQLiteDatabase();
-        values.clear();
-        final Cursor cursor = database.query(table, null, selection, args, null, null, null, "1");
-        try {
-            if (cursor.getCount() == 0) {
-                return;
-            }
-            cursor.moveToNext();
-            DatabaseUtils.cursorRowToContentValues(cursor, values);
-        } finally {
-            cursor.close();
-        }
-    }
-
-    /**
-     * Gets SQL expression that represents the given value or NULL depends on the row state.
-     * You must pass static constants to this methods otherwise you may be suffered from SQL
-     * injections.
-     * @param state Expected row state.
-     * @param a SQL value.
-     * @return Expression that represents a if the row state is expected one, and represents NULL
-     *     otherwise.
-     */
-    private static String createStateFilter(int state, String a) {
-        return "CASE WHEN " + COLUMN_ROW_STATE + " = " + Integer.toString(state) +
-                " THEN " + a + " ELSE NULL END";
-    }
 }
index 3cfb82f..33687cb 100644 (file)
@@ -78,14 +78,6 @@ class MtpDatabaseConstants {
     static final int ROW_STATE_INVALIDATED = 1;
 
     /**
-     * The state represents the raw has a valid object handle but it may be going to be mapped with
-     * another rows invalidated. After fetching all documents under the parent, the database tries
-     * to map the pending documents and the invalidated documents in order to keep old document ID
-     * alive.
-     */
-    static final int ROW_STATE_PENDING = 2;
-
-    /**
      * Mapping mode that uses MTP identifier to find corresponding rows.
      */
     static final int MAP_BY_MTP_IDENTIFIER = 0;
index a49dc67..a75012e 100644 (file)
@@ -310,14 +310,14 @@ public class MtpDatabaseTest extends AndroidTestCase {
             assertEquals(3, cursor.getCount());
             cursor.moveToNext();
             assertEquals(1, getInt(cursor, COLUMN_DOCUMENT_ID));
-            assertTrue(isNull(cursor, COLUMN_STORAGE_ID));
+            assertEquals(200, getInt(cursor, COLUMN_STORAGE_ID));
             assertEquals("Storage A", getString(cursor, COLUMN_DISPLAY_NAME));
             cursor.moveToNext();
             assertEquals(2, getInt(cursor, COLUMN_DOCUMENT_ID));
             assertTrue(isNull(cursor, COLUMN_STORAGE_ID));
             assertEquals("Storage B", getString(cursor, COLUMN_DISPLAY_NAME));
             cursor.moveToNext();
-            assertEquals(4, getInt(cursor, COLUMN_DOCUMENT_ID));
+            assertEquals(3, getInt(cursor, COLUMN_DOCUMENT_ID));
             assertEquals(202, getInt(cursor, COLUMN_STORAGE_ID));
             assertEquals("Storage C", getString(cursor, COLUMN_DISPLAY_NAME));
             cursor.close();
@@ -333,7 +333,7 @@ public class MtpDatabaseTest extends AndroidTestCase {
             assertEquals(200, getInt(cursor, COLUMN_STORAGE_ID));
             assertEquals("Storage A", getString(cursor, COLUMN_DISPLAY_NAME));
             cursor.moveToNext();
-            assertEquals(4, getInt(cursor, COLUMN_DOCUMENT_ID));
+            assertEquals(3, getInt(cursor, COLUMN_DOCUMENT_ID));
             assertEquals(202, getInt(cursor, COLUMN_STORAGE_ID));
             assertEquals("Storage C", getString(cursor, COLUMN_DISPLAY_NAME));
             cursor.close();
@@ -387,7 +387,7 @@ public class MtpDatabaseTest extends AndroidTestCase {
             assertEquals(4, cursor.getCount());
 
             cursor.moveToPosition(3);
-            assertEquals(5, getInt(cursor, COLUMN_DOCUMENT_ID));
+            assertEquals(4, getInt(cursor, COLUMN_DOCUMENT_ID));
             assertEquals(203, getInt(cursor, COLUMN_OBJECT_HANDLE));
             assertEquals("video.mp4", getString(cursor, COLUMN_DISPLAY_NAME));
 
@@ -406,7 +406,7 @@ public class MtpDatabaseTest extends AndroidTestCase {
             assertEquals("note.txt", getString(cursor, COLUMN_DISPLAY_NAME));
 
             cursor.moveToNext();
-            assertEquals(5, getInt(cursor, COLUMN_DOCUMENT_ID));
+            assertEquals(4, getInt(cursor, COLUMN_DOCUMENT_ID));
             assertEquals(203, getInt(cursor, COLUMN_OBJECT_HANDLE));
             assertEquals("video.mp4", getString(cursor, COLUMN_DISPLAY_NAME));
             cursor.close();
@@ -544,7 +544,7 @@ public class MtpDatabaseTest extends AndroidTestCase {
             assertEquals(1, cursor.getCount());
             cursor.moveToNext();
             assertEquals(2, getInt(cursor, COLUMN_DOCUMENT_ID));
-            assertTrue(isNull(cursor, COLUMN_OBJECT_HANDLE));
+            assertEquals(201, getInt(cursor, COLUMN_OBJECT_HANDLE));
             cursor.close();
         }
     }
@@ -626,11 +626,12 @@ public class MtpDatabaseTest extends AndroidTestCase {
             final Cursor cursor = mDatabase.queryRootDocuments(columns);
             assertEquals(2, cursor.getCount());
             cursor.moveToNext();
-            assertEquals(2, getInt(cursor, COLUMN_DOCUMENT_ID));
+            // One reuses exisitng document ID 1.
+            assertEquals(1, getInt(cursor, COLUMN_DOCUMENT_ID));
             assertEquals(200, getInt(cursor, COLUMN_STORAGE_ID));
             assertEquals("Storage", getString(cursor, COLUMN_DISPLAY_NAME));
             cursor.moveToNext();
-            assertEquals(3, getInt(cursor, COLUMN_DOCUMENT_ID));
+            assertEquals(2, getInt(cursor, COLUMN_DOCUMENT_ID));
             assertEquals(201, getInt(cursor, COLUMN_STORAGE_ID));
             assertEquals("Storage", getString(cursor, COLUMN_DISPLAY_NAME));
             cursor.close();