Realm-java: Proposal: Deprecate or change RealmQuery.findFirstAsync()

[Zhuinden]

There is no safe way to use findFirstAsync. If the object is deleted, you'll get an invalid object, and you'll need a results change listener to know if you need to re-execute the object query to find the object.

It would be much safer to provide a RealmOptional, which you could register change listener to, which is added to the object if it's found and present, or just "kept for later" if doesn't. And the RealmOptional would be based on a results listener that "if empty, then not present, else first element of result". This way it would be kept up to date like a RealmResults except it is single element.

[cmelchior]

Ref: https://github.com/realm/realm-java/issues/5179

First, I would say I probably agree with you. Secondly, I actually tried looking into this a couple of months back there is at least two corner case that is slightly annoying

Consider:

RealmOptional<Person> personOpt = realm.where(Person.class).equalTo("name", "Jane").findFirst();
personOpt.addChangeListener((person, changeset) -> {
  // 1) Person might be null, and current change listeners do not allow null
 //  2) How should the changeset look like when switching between objects?
 //     The current behaviour will be emitting an empty changeset when a new object is found.
 //     This might be fine though.
});

Behind the scenes, the RealmOptional would just be wrapping findAll() which is fairly easy, but having to create entirely new listener interfaces NullableRealmChangeListener andNullableRealmObjectChangeListener` is fairly annoying, but I'm not sure I see a way around it.

[Zhuinden]

// 1) Person might be null, and current change listeners do not allow null

Technically you could emit the invalid object when item is not found, it's already pretty much a placeholder for null anyways in case object was deleted.

// 2) How should the changeset look like when switching between objects?
// The current behaviour will be emitting an empty changeset when a new object is found.
// This might be fine though.

Empty change set would make sense, the trickier part is the changeset state that I believe comes from OS.

[Zhuinden]

Another crazy idea I had was that RealmOptional could be used synchronously too, but findFirst already has its uses, so I'm not lobbying to change that. In which case the question is naming.

[cmelchior]

RealmObjectChangeSet is just an interface, so creating an empty implementation would be trivial, but looking at the interface definition it seems we return null the first time the object is loaded, so we would just return null whenever a new object replaced the old one.
So an acceptable API would be the below?

// Will never return `null`
public class RealmOptional<T extends RealmModel> {
  T get(); // Return matching or object or invalid object if not found
  boolean isPresent(); // true if object is there and valid
  T orElse(t other); // Return other if isPresent() returns false
  void addChangeListener(RealmChangeListener listener);
  void addChangeListener(RealmObjectChangeListener listener);
  void removeChangeListener(RealmChangeListener listener);
  void removeChangeListener(RealmObjectChangeListener listener);
  void removeAllChangeListeners();

Usage

// New behaviour where you just want _some_ object to match your 
// query (i.e. continuously evaluate query).

RealmOptional<Person> p = realm.where(Person.class).equaTo("name", "Jane").findFirstAsync();
p.addChangeListener((person, changeset) -> {
  if (changeset == null) 
     // We only emit null the first time the object is loaded
     refreshPerson(p); // Fully refresh the person 
  } else if (person.isValid()) {
    // updatePerson(person, changeset); // Current person was updated
  } else {
    // Person was deleted and no new object matched the query, optional is empty
  }
});

// Opting in to the old behaviour is one extra method call
RealmOptional<Person> p = realm.where(Person.class).equaTo("name", "Jane").findFirstAsync();
Person per = p.get();
per.addChangeListener(...); // Like normal

Does this look acceptable @Zhuinden?

We would either need to break the API if we replace findFirstAsync() or find another name like findFirstOptionalAsync() 🤢 . I would probably be fine with breaking findFirstAsync() since it would also enable us to make subscriptions work with it. Implementation wise the above would be fairly trivial as we are just wrapping findAllAsync() and object listeners once an object is found.

Thoughts @nhachicha

For reference, the Android version of Optional is here: https://developer.android.com/reference/java/util/Optional

Guava version is here:
https://google.github.io/guava/releases/22.0/api/docs/com/google/common/base/Optional.html

Java 8 version is here:
https://docs.oracle.com/javase/8/docs/api/java/util/Optional.html

[Zhuinden]

Pretty much what I was thinking of, yes :+1:

findFirstOptionalAsync()

Or just findOne/findOneAsync 😄 no that'd be confusing, dammit

findOptional / findOptionalAsync? I'm bad with names.

[cmelchior]

Any comments on this @nhachicha ?

[nhachicha]

@cmelchior having findAllAsync as the underneath implementation could indeed solve the subscription issue we have in the query based Realm https://github.com/realm/realm-java/issues/5938

I'm in favour of breaking the API rather then introducing a new method, since this fixes the behaviour of the current implementation (+ Sync issue).

Do you plan to wrap findFirst as well with RealmOptional? my guess is yes to make the API symmetric ?

Do you think we should extend the use of RealmOptional to wrap nullable fileds as well in the model, obviously you can not register listeners, but you can benefit from the isPresent/orElse syntax?

[cmelchior]

I'm still on the fence regarding letting findFirst() return RealmOptional as the synchronous methods pretty much work as we expect and @Zhuinden also seems to prefer to keep it as it is.

Allowing Optional for properties makes sense, but there we should just add support for the standard Optional classes IMO, because as you point out, listeners probably don't make sense. This is already being tracked here: https://github.com/realm/realm-java/issues/1260 and doesn't seem to be highly requested.

[Zhuinden]

The only oddity in retrospect is that

p.addChangeListener((person, changeset) -> {
  if (changeset == null)  // <-- this here
     // We only emit null the first time the object is loaded

Realm 5.0+ made changeSet non-null because it emits a .INITIAL instead of null changeset.

the synchronous methods pretty much work as we expect and @Zhuinden also seems to prefer to keep it as it is.

Yes, findFirst() is predictable. findFirstAsync() is just awkward.

[cmelchior]

Also, we are close to merging LIMIT support in Core, which means that query.limit(1) can easily replace the current code and be fully serializable with regard to subscriptions.

[Zhuinden]

Oooh that's cool!

Wait a second, if you have LIMIT, can you do skip/take?