Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 2b3004dc authored by Tobias Thierer's avatar Tobias Thierer
Browse files

Fix DataChangedJournalTest.equals(). 

Problems:
 1. The class implemented equals() but not hashCode()
 2. equals() attempted to canonicalize the underlying path and silently
    returned false if the I/O operation failed.

Fixes:
 1. Implemented hashCode().
 2. Stopped equals() attempting canonicalization and built in directly
    on top of File.equals() instead. I also considered moving the
    canonicalization to the DataChangedJournal ctor, but that
     - had the undesirable side effect of stopping interaction with the
       journal for reasons other than the file itself being corrupt, and
     - posed the problem of what to do in case of an IOException during
       construction.
    The downside of course is that caller which (inappropriately) relied
    on equals() doing canonicalization can no longer rely on that. I
    didn't find any such callers.

Note:
 When DateChangedJournal.listJournals() fails to perform a directory
 listing (e.g. if the journal directory doesn't exist), it currently
 still returns an (empty) ArrayList<DateChangedJournal>. This CL does
 not address this, but in a follow-up CL I might change this to return
 an unmodifiable List or Set<DateChangedJournal>, and throw an
 IOException if the directory contents can't be listed.

Bug: 162022005
Test: atest FrameworksServicesTests:com.android.server.backup.DataChangedJournalTest

Change-Id: I94f35f9a3082f398758fe5b03fb8d260e4fb3e1b
parent 112e3c2d

services/backup/java/com/android/server/backup/DataChangedJournal.java

+8 −6
Original line number Diff line number Diff line
@@ -36,7 +36,7 @@ import java.util.function.Consumer; 
 * <p>This information is persisted to the filesystem so that it is not lost in the event of a

 * reboot.

 */

public class DataChangedJournal {

public final class DataChangedJournal {

    private static final String TAG = "DataChangedJournal";

    private static final String FILE_NAME_PREFIX = "journal";
@@ -54,6 +54,7 @@ public class DataChangedJournal { 
        mFile = file;

    }





    /**

     * Adds the given package to the journal.

     *
@@ -106,15 +107,16 @@ public class DataChangedJournal { 
        return mFile.delete();

    }



    @Override

    public int hashCode() {

        return mFile.hashCode();

    }



    @Override

    public boolean equals(@Nullable Object object) {

        if (object instanceof DataChangedJournal) {

            DataChangedJournal that = (DataChangedJournal) object;

            try {

                return this.mFile.getCanonicalPath().equals(that.mFile.getCanonicalPath());

            } catch (IOException exception) {

                return false;

            }

            return mFile.equals(that.mFile);

        }

        return false;

    }

services/backup/java/com/android/server/backup/UserBackupManagerService.java

+0 −1
Original line number Diff line number Diff line
@@ -1155,7 +1155,6 @@ public class UserBackupManagerService { 


    private void parseLeftoverJournals() {

        ArrayList<DataChangedJournal> journals = DataChangedJournal.listJournals(mJournalDir);

        // TODO(b/162022005): Fix DataChangedJournal implementing equals() but not hashCode().

        journals.removeAll(Collections.singletonList(mJournal));

        if (!journals.isEmpty()) {

            Slog.i(TAG, addUserIdToLogMessage(mUserId,

services/tests/servicestests/src/com/android/server/backup/DataChangedJournalTest.java

+9 −6
Original line number Diff line number Diff line
@@ -39,7 +39,6 @@ import org.mockito.MockitoAnnotations; 
import java.io.DataInputStream;

import java.io.File;

import java.io.FileInputStream;

import java.util.ArrayList;

import java.util.function.Consumer;



@SmallTest
@@ -90,7 +89,13 @@ public class DataChangedJournalTest { 


    @Test

    public void equals_isTrueForTheSameFile() throws Exception {

        assertThat(mJournal.equals(new DataChangedJournal(mFile))).isTrue();

        assertEqualsBothWaysAndHashCode(mJournal, new DataChangedJournal(mFile));

    }



    private static <T> void assertEqualsBothWaysAndHashCode(T a, T b) {

        assertEquals(a, b);

        assertEquals(b, a);

        assertEquals(a.hashCode(), b.hashCode());

    }



    @Test
@@ -117,9 +122,7 @@ public class DataChangedJournalTest { 
        DataChangedJournal.newJournal(folder);

        DataChangedJournal.newJournal(folder);



        ArrayList<DataChangedJournal> journals = DataChangedJournal.listJournals(folder);



        assertThat(journals).hasSize(2);

        assertThat(DataChangedJournal.listJournals(folder)).hasSize(2);

    }



    @Test
@@ -140,6 +143,6 @@ public class DataChangedJournalTest { 
    public void listJournals_invalidJournalFile_returnsEmptyList() throws Exception {

        when(invalidFile.listFiles()).thenReturn(null);



        assertEquals(0, DataChangedJournal.listJournals(invalidFile).size());

        assertThat(DataChangedJournal.listJournals(invalidFile)).isEmpty();

    }

}