Realm IDs are now its absolute path instead of its hashcode

[cmelchior]

Fixes #1040

@emanuelez @bmunkholm @kneth

[emanuelez]

I guess this can be renamed to absolutePath

[cmelchior]

We already have a path variabel we can use instead. I was just weary to overload the meaning of the name.

But removing it completely and just use path is fine with me. It also removes the ambiguity we have in some of the static methods that manually calculate the ID atm.

[zaki50]

if we use path as ID, it's better to use canonical path instead of absolute path.
is it too strict?

[emanuelez]

👍 for @zaki50 proposal

[cmelchior]

File.getCanonicalPath() is 1300x times slower than calling File.getAbsolutePath() on my Nexus 5. Is there a use case where using getAbsolutePath would result in the same ID for two different Realms?

ID's are only local and used on runtime. If we cached them on disk, I would probably agree, but right now I don't think we need the extra security or is there something I am missing?

[emanuelez]

The problem is the opposite. Two absolute paths can point to the same file.

[cmelchior]

True, but wouldn't you really have to go out of the way to make that happen, eg.

File f1 = new File(getContext().getFilesDir(), "new_dir/../default.realm");
File f2 = new File(getContext().getFilesDir(), "default.realm");
assertFalse(f1.getAbsolutePath(), f2.getAbsolutePath())
assertTrue(f1.getCanonicalPath(), f2.getCanonicalPath())

I am not against it per se, but at least on Android I doubt this will be an issue, so introducing a slowdown for a really small corner case?

Especially considering that even if you did do this, you would "just" have two instances of a Realm which would hog resources, but it wouldn't cause errors.

[emanuelez]

Maybe we can check if the string contains .. and throw if it does. I kinda like the idea of making this hard to misuse

[cmelchior]

Throwing seems kinda harsh though. We could also add a check for ".." and then fallback to canonical path if needed.

String path = file.getAbsolutePath();
if (path.contains("..")) {
   path = file.getCanonicalPath();
}

This introduces a slowdown of about 10x for paths not containing "..". Neither of these solutions work with symlinks though, but we shouldn't encounter them on Android at least (we probably will on the server side)

[emanuelez]

You last solution looks fine. We can document the performance drop in the Javadoc.

Only Java8 has proper support for symlinks so there's that

[zaki50]

there are some other corner cases.

    final String path1 = new File(getActivity().getFilesDir(), "foo.realm").getAbsolutePath();
    final String path2 = new File(getActivity().getFilesDir(), "./foo.realm").getAbsolutePath();

    final String path2_1 = new File(getActivity().getFilesDir(), "foo/bar.realm").getAbsolutePath();
    final String path2_2 = new File(getActivity().getFilesDir(), "foo/./bar.realm").getAbsolutePath();

"./" or simply "/" is better for fallback condition, I think.

[cmelchior]

Good point. Thinking about this a bit more, then ti would probably be cleaner just to use canonicalPath() and then take the performance hit. Once we introduce RealmConfiguration #929 , this becomes less of an issue anyway since we are only going to calculate it once.

[cmelchior]

Note, I deprecated getPath() in favour of getCanonicalPath() but I am not totally convinced it is a good idea, since the later is probably way less know. Maybe we should just document that getPath() returns the canonical path instead?

[zaki50]

I think getPath() is enough because we have just single getter of path.

[cmelchior]

Bump @emanuelez

[emanuelez]

👍

[kneth]

👍