Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix findFirst() for queried RealmList #826

Merged
merged 3 commits into from Feb 11, 2015
Merged

Fix findFirst() for queried RealmList #826

merged 3 commits into from Feb 11, 2015

Conversation

@chibatching
Copy link
Contributor

@chibatching chibatching commented Feb 6, 2015

I found the bug of RealmQuery#findFirst() for queried ReamList.
FindFirst() may return different result of FindAll().get(0).

Code to reproduce:

public class Book extends RealmObject {
    private String id;
    private RealmList<Page> pages;
}
public class Page extends RealmObject {
    private String id;
}

String book = "book_city";
String page = "city_japan";
RealmList<Page> pages = realm.where(Book.class)
                             .equalTo("id", book)
                             .findFirst()
                             .getPages();

RealmList<Page> page1 = pages.where()
                             .equalTo("id", page)
                             .findAll()
                             .get(0);

RealmList<Page> page2 = pages.where()
                             .equalTo("id", page)
                             .findFirst();

Data:

[book]      [page]
book_food   food_fast
book_food   food_chinese
book_food   food_japanese
book_city   city_america
book_city   city_japan

Results:

page1 -> city_japan   -- Expected
page2 -> food_chinese -- Not expected

I don't know whether my PR is best way.
But RealmQuery#findFirst() should look queried table or view because returned rowIndex is not source row index.

@cmelchior
Copy link
Contributor

@cmelchior cmelchior commented Feb 6, 2015

Hi @chibatching
Thanks for finding and fixing the bug. A few initial comments:

  • In order for us to accept it, you need to sign the contributors form first: https://github.com/realm/realm-java/blob/master/CONTRIBUTING.md
  • Please add a unit test that catches the original error.
  • Im a bit concerned about you executing a query in the RealmQuery.where() method, but I'll have to take a deeper look at the code first to understand if it is really required there.
@@ -236,8 +236,8 @@ public int size() {
*/
public RealmQuery<E> where() {
if (managedMode) {
TableQuery query = this.view.where();
return new RealmQuery<E>(this.realm, query, clazz);
TableOrView table = this.view.where().findAll();

This comment has been minimized.

@bmunkholm

bmunkholm Feb 6, 2015
Contributor

We should definitely not do a query in where(). Youmust be able to build up a query and then execute it afterwards.

@chibatching
Copy link
Contributor Author

@chibatching chibatching commented Feb 6, 2015

Hi @cmelchior
Thank you for comment.
I sent CLA form and added unit test for checking this error.
Please check it.

@bmunkholm
Thank you for reviewing my code.
I removed executing query in where(). Please re-review.

@kneth
Copy link
Member

@kneth kneth commented Feb 11, 2015

This looks good to me, and I merge it now. We might have a few extra tricks in the core library to speed it up but we haven't exposed them in Java yet.

kneth pushed a commit that referenced this pull request Feb 11, 2015
Fix findFirst() for queried RealmList
@kneth kneth merged commit 201da45 into realm:master Feb 11, 2015
1 check passed
1 check passed
default Merged build finished.
Details
@degill
Copy link

@degill degill commented Apr 8, 2015

I see that this was merged into master in february. But I think I am still have issues related to different results from findFirst() and findAll().first()

I have a RealmResults which I further want to query: mAllUsersForCompany is of type RealmResults:

The following works. It gives me a user whos name has length of three and who was in the mAllUsersForCompany list:

mAllUsersForCompany.where().equalTo("nameLength", 3).findAll().first()

But the following seems to completely ignore the additional equalTo constraint and gives me a user which has namelength != 3

mAllUsersForCompany.where().equalTo("nameLength", 3).findFirst()
@cmelchior
Copy link
Contributor

@cmelchior cmelchior commented Apr 8, 2015

Hi @gillde
I have moved your comment to a separate issue so it is more easily trackable as this has already been merged: #1018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants