Issue 4231 - Fix unstable FilterEngineTest.testSetRemoveFilterChangeCallback

libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidFileSystem.java

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 package org.adblockplus.libadblockplus.android;
 
 import org.adblockplus.libadblockplus.AdblockPlusException;
 import org.adblockplus.libadblockplus.FileSystem;
 import org.adblockplus.libadblockplus.FileSystemUtils;
 
 import java.io.File;
-import java.io.FileNotFoundException;
 import java.io.IOException;
 
 /**
  * AndroidFileSystem is inefficient for production files routines
  * as C++ streams can't pass JNI layer and be converted into Java streams and vice versa.
  *
  * So in case of any stream routines full stream content is read in either C++ or Java side
- * and it's being passed as string though JNI layer
+ * and it's being passed as bytes array though JNI layer
  *
  * AndroidFileSystem is meant to be used in tests mostly
+ *
+ * All paths are considered relative to the base path, or to be absolute
+ * (see `resolve(String path)`) if no base path is set (see `AndroidFileSystem(File basePath)`)
  */
 public class AndroidFileSystem extends FileSystem
 {
   private File basePath;
 
   public File getBasePath()
   {
     return basePath;
   }
 
   public AndroidFileSystem()
   {
   }
 
   /*
    * Sets the base path, all paths are considered relative to it.
    * @param basePath base path
    */
   public AndroidFileSystem(File basePath)
   {
     this();

[diegocarloslima, 2017/04/28 13:23:53]

Recapping our discussion here from the old code review: I didn't get your point
here. If we omit 'this()' call, by default it will implicitly call 'super()', 
which is the empty ctor of FileSystem. Shouldn't that be the desired behaviour?
Right now, it just calls the empty ctor of AndroidFileSystem, and this one
implicitly calls super(), which produces the same result.

[anton, 2017/05/22 13:04:38]

AndroidFileSystem can be used without passing `basePath` in ctor and with it.
The desired behaviour is to have a method to init instance for both use-cases
and currently it's empty ctor body.
So the desired behaviour is to call this instance empty ctor, not super ctor.

We could remove `this()` invocation but then we should add some `init()` methods
and call it from both empty and 1-var ctors.

     this.basePath = basePath;
   }
 
-  private File buildFullPath(String path)
+  @Override
+  public byte[] read(String path)
   {
-    return basePath != null
-      ? new File(basePath, path)
-      : new File(path);
-  }
-
-  @Override
-  public String read(String path)
-  {
-    File file = buildFullPath(path);
+    File file = new File(path);

[anton, 2017/05/29 11:30:03]

Warning: it does not do resolve() just like in DefaultFileSystem.cpp. This is
going to be updated as soon as it's changed in c++

     if (!file.exists())
     {
       return null;
     }
 
     try
     {
       return FileSystemUtils.readFile(file);
     }
-    catch (FileNotFoundException e)
+    catch (IOException e)
     {
       throw new AdblockPlusException(e);
     }
   }
 
   @Override
-  public void write(String path, String data)
+  public void write(String path, byte[] data)
   {
-    File file = buildFullPath(path);
+    File file = new File(path);

[anton, 2017/05/29 11:30:02]

Warning: it does not do resolve() just like in DefaultFileSystem.cpp. This is
going to be updated as soon as it's changed in c++

     if (file.exists())
     {
       file.delete();
     }
 
     try
     {
       FileSystemUtils.writeFile(file, data);
     }
     catch (IOException e)
     {
       throw new AdblockPlusException(e);
     }
   }
 
   @Override
   public void move(String fromPath, String toPath)
   {
-    File fullFromFile = buildFullPath(fromPath);
-    try
+    File fromFile = new File(fromPath);
+    if (!fromFile.exists())
+      throw new AdblockPlusException("File does not exist: " + fromPath);
+
+    File toFile = new File(toPath);
+    if (!fromFile.renameTo(toFile))
     {
-      String data = FileSystemUtils.readFile(fullFromFile);
-      write(toPath, data);
-      remove(fromPath);
-    }
-    catch (FileNotFoundException e)
-    {
-      throw new AdblockPlusException(e);
+      throw new AdblockPlusException("Failed to move " + fromPath + " to " + toFile);
     }
   }
 
   @Override
   public void remove(String path)
   {
-    File file = buildFullPath(path);
+    File file = new File(path);
     if (file.exists())
     {
       file.delete();
     }
   }
 
   @Override
   public StatResult stat(String path)
   {
-    File file = buildFullPath(path);
+    File file = new File(path);
     return new StatResult(
       file.exists(),
       file.isDirectory(),
       file.isFile(),
       file.lastModified());
   }
 
   @Override
   public String resolve(String path)
   {
-    String fullPath = buildFullPath(path).getAbsolutePath();
-    return basePath != null
-      ? fullPath.substring(basePath.getAbsolutePath().length())
-      : fullPath;
+    return (basePath != null ? new File(basePath, path).getAbsolutePath() : path);
   }

[sergei, 2017/05/22 12:09:07]

I think it inconsistent with C++ version where the method returns the absolute
path to a file.
https://github.com/adblockplus/libadblockplus/blob/master/include/AdblockPlus...

[anton, 2017/05/22 13:04:38]

if it's created with `basePath` argument (see ctor) then basePath is the root
and `absolute path` means `relative to basePath` (see `buildFullPath`).
If `basePath` is not passed, then it's real absolute path.

[sergei, 2017/05/22 13:51:04]

I think that `resolve` method had been introduced to firstly resolve any kind of
relative paths and it should always return a full absolute path, thus
potentially exposing any information about internal base path.
E.g., though it's like an error in the app, for the following call
resolve("dirX/../../fileZ"); and base path "/dirA/dirB" the result should be
"/dirA/fileZ", however with that implementation it will be
"/dirA/fileZ".substring("/dirA/dirB".length => 10) => "Z". For instance, the
implementation for windows is
https://github.com/adblockplus/libadblockplus/blob/master/src/DefaultFileSyst....

Anyway, in the recent version of adblockpluscore it seems there are no anymore
calls to this resolve method and I guess it's supposed that other methods are
aware about it, so if it works now, then maybe it's fine because later it will
be removed as a dead code.

[anton, 2017/05/23 05:46:47]

You did not get the idea that was introduced by android file system specific.

The app's files root is /data/data/package/ in Android. So we need to consider
all paths in c++ as related to that root.
If you're using path "/somefile" in c++, it will be /data/data/package/somefile
in Android and when you request "/somefile/../somanotherfile" in android
it will be "/data/data/package/somefile/../someanotherfile" but in c++ you will
have "/someanotherfile" path returned by "resolve".

So in c++ you will never know actual basePath used on android side.

Another example. You have c++ path "patterns.ini". On android side it will be
"/data/data/package/patterns.ini". if you request resolve("patterns.ini") in
c++, it will
return you "/patterns.ini" (though on android side path
"/data/data/package/patterns.ini" is used).

[sergei, 2017/05/23 08:08:19]

It was clear and I think that it's incorrect.
I think that it's expected that resolve always returns an absolute path what is
consistent with documentation and in addition without any relative things like
"..", "./". E.g. for for "patterns.ini", for "./patterns.ini" and for
"./some-path/../patterns.ini" the result is "/data/data/package/patterns.ini".
BTW, the same for other methods, one should use base path only if the path does
not start from "/".
How can one write to /sdcard/file-some using the proposed API?

[anton, 2017/05/23 11:11:18]

Can't get your point. All the paths are considered to be relative to 'basePath'.
"/data/data/package" was introduced by android side and it should not be exposed
to c++ side.
For all incoming paths "/data/data/package" is added when entering android side
and stripped when returning back to c++ side.
You will not get any relative paths just because of adding basePath at the
beginning. 
I mean adding basePath is fully transparent for c++ side and does not affect
absolute/relative path point.
For resolve("/somepath/../file") you will get "/file"
if you wish to access "/sdcard/file" in c++ code it means you're trying to leave
app sand-box and you just don't need to pass 'basePath' in AndroidFileSystem
ctor. You can do it but it means c++ part is aware of the environment and it
should take into account it's an android and it's fs limitations, e.g you can't
write to "/" and so on.

[sergei, 2017/05/23 13:00:37]

In my opinion the aim of FileSystem interface is to provide with an abstraction
from platform low level functions. However, here, merely because it's convenient
and seems logical, we use it in addition to override a current working directory
for ABP which is denoted as the base path here. Since there was no demand in
additional sendboxing level so far I think we should not add it until it's
required. If the code is willing to write or read at absolute location and it's
not allowed, then it's an issue of that code. For example, there can be a
completely legit code in libadblockplus-android which is aware about current
environment and which should work with something outside the base path, any why
should it not be able to work with the provided interface for file system? Even
more, it is expected that method of the interface returns "the absolute path to
a file"
(https://github.com/adblockplus/libadblockplus/blob/master/include/AdblockPlus...)
and it's an abstraction around platform low level functions.

 }