Migrate to text based query parser. #7318

clementetb · 2021-02-22T09:56:33Z

Refactors the current type-safe query system in Ream Java to use the new string-based query parser. It introduced some behavioral changes:

Querying for equalto/notequalto null on non-nullable fields no longer throws.
~~or() with not right part does not throw.~~
distinct() does not fail on invalid types/unknown fields.
isEmpty/isNotEmpty now works with all Realm list types (before it only worked on Realm Objects Lists).
~~There is no actual query validation until it gets executed.~~

String operations contains, like, beginsWith, endsWith enforce non-null value arguments. These operations arguments were marked as non-null but no check was in place.

Known issues

…ing classes out from the schema

… defined.

cmelchior

Really nice... are a bunch of tests that needs to be revisited though. And I had some comments to implementation in places.

One question though: If FieldDescriptors are no longer needed, couldn't we also remove a lot of code from the ProxyClassGenerator?

cmelchior · 2021-03-24T08:08:48Z

CHANGELOG.md

@@ -1,12 +1,18 @@
 ## 10.4.0 (YYYY-MM-DD)
+### Breaking Changes
+* Queries no longer check do nullability checks on non-nullable fields.


Suggested change

* Queries no longer check do nullability checks on non-nullable fields.

* Queries no longer do nullability checks on non-nullable fields, so using `null` as an argument will not throw an `IllegalArgumentException`

cmelchior · 2021-03-24T08:09:32Z

CHANGELOG.md

+
+### Known Bugs
+* Java field names not supported on Sort or Distinct operations. (Issue [#4550] (https://github.com/realm/realm-core/issues/4550))
+* Queries on fields named with non-latin characters are not currently supported. (Issue [#4467] (https://github.com/realm/realm-core/issues/4467))


It is fine to have this entry when merging this PR, but I would consider this a blocker for releasing.

cmelchior · 2021-03-24T08:10:50Z

realm/realm-library/src/androidTest/java/io/realm/DynamicRealmTests.java

@@ -725,6 +725,7 @@ public void run() throws Exception {
    }

    @Test
+    @Ignore("FIXME: See https://github.com/realm/realm-core/issues/4469")


The issue has been closed, so this test should probably be updated.

cmelchior · 2021-03-24T09:06:23Z

realm/realm-library/src/androidTest/java/io/realm/RealmLinkTests.java

-            fail();
-        } catch (IllegalArgumentException ignored) {
-        }
+        RealmResults<Owner> owners7 = testRealm.where(Owner.class).between("cat.age", 1, 20).findAll();


It also closes this issue: #2204 🥳 , so should definitely be in the CHANGELOG

realm/realm-library/src/androidTest/java/io/realm/RealmQueryTests.java

cmelchior · 2021-03-24T11:19:25Z

realm/realm-library/src/main/java/io/realm/RealmQuery.java

     * <p>
     * Class and property names used in the raw predicate can be either the names defined in the
     * Realm Model classes or the internal names defined using the {@link io.realm.annotations.RealmClass}
     * or {@link io.realm.annotations.RealmField} annotations.
-     * <p>


Any reason to remove this?
We should probably also add something about neededing to escape field names with spaces?

Shouldn't have removed. Added a comment on escaping field and class names.

cmelchior · 2021-03-24T11:27:36Z

realm/realm-library/src/main/java/io/realm/internal/Table.java

        this.context = sharedRealm.context;
        this.sharedRealm = sharedRealm;
        this.nativeTableRefPtr = nativeTableRefPointer;
+        this.osKeyPathMapping = osKeyPathMapping;


Not sure it is safe to store it like this? The keypath mapping can be updated by a callback from Core, making storing it here dangerous. Unless the usage is very shortlived. But maybe I am missing something?

Great catch, it is not safe as it can be recreated by a callback from core.

cmelchior · 2021-03-24T11:34:40Z

realm/realm-library/src/main/java/io/realm/internal/TableQuery.java

    private boolean queryValidated = true;

-    // TODO: Can we protect this?
-    public TableQuery(NativeContext context, Table table, long nativeQueryPtr) {
+    private static String escapeFieldName(String fieldName) {


Not sure it is a bug as such, just something we need to document for using the string-based queries.

cmelchior · 2021-03-24T11:39:52Z

realm/realm-library/src/main/java/io/realm/internal/TableQuery.java

-        queryValidated = false;
-        return this;
-    }
+            sortSeparator = ", ";


This seems to lead to trailing , . That is okay with the Query Parser?

It is fine! We use the separator between fields.

cmelchior · 2021-03-24T11:41:00Z

realm/realm-library/src/main/java/io/realm/internal/TableQuery.java

-        if (value == null) { throw new IllegalArgumentException(DATE_NULL_ERROR_MESSAGE); }
-        nativeGreaterTimestamp(nativePtr, columnKey, tablePtrs, value.getTime());
+    public TableQuery isEmpty(String fieldName) {
+        rawPredicateWithPointers(escapeFieldName(fieldName) + ".@count = 0");


Does this also work with Strings? If I remember correctly the old isEmpty() also accepted Strings?

Yes, it does!

clementetb · 2021-03-26T00:01:25Z

realm/realm-library/src/androidTest/java/io/realm/RealmQueryTests.java

@@ -3097,30 +3095,28 @@ private void distinctAllFields(Realm realm, String prefix) {
            types.remove(type);
        }

-        // Verify that we have tested all field types except LinkingObjects which is not part of
-        // the schema lookup
+        // Validate that backlinks are not supported by sort/distinct


Distinct and sort don't support backlinks: realm/realm-core#4524

clementetb added 4 commits February 17, 2021 12:17

Allow argument passing on raw predicates

8ba609b

Add object argument substitution

4bf5608

Initial work on adapting typed queries to text based queries

b371480

Check for if the first connector is an OR. Fixes queries extension .

a057d86

clementetb self-assigned this Feb 22, 2021

clementetb added 22 commits February 22, 2021 12:36

Fix out of bounds exception when trying to find first

7639116

Leave nullability checks to query parser

f265004

Add support for isempty/isnotempty

680d4a0

Fix isempty implementation

fddd990

Fix get query description

ab9567d

Avoid appending truepredicates

3907d84

Allow passing null strings into string queries

822e18b

Fix query description test case

3391158

Stop using field descriptors in realm querys

7cb641f

Add raw predicates into the query builder, fix bunch of tests

d5c8829

Handle query argument list lifecycle

adaa578

Adapt pending realmquery test cases

19b3064

Reenable and adapt realm result tests

ca9b1d1

Reenable JNI query tests

0f2e463

Remove non strings based query parser 🚀

0f0e3bf

Clean up

903871e

Allow to exclude classes from the schema

881ade0

Add debug method into the realm configuration builder to allow exclud…

40276b6

…ing classes out from the schema

Adapt LinkingObjectsQueryTests

2a196c1

Fix TestRealmConfigurationFactory to only apply schema filtering when…

00c35a4

… defined.

Adapt tests to the string based queries

17010d3

Styling

f7e60c3

clementetb requested review from cmelchior and edualonso and removed request for cmelchior February 26, 2021 16:03

cmelchior suggested changes Mar 24, 2021

View reviewed changes

clementetb and others added 22 commits March 24, 2021 12:58

Improve changelog updates

884f88b

Update changelog and update support distinct fields

7c4ae44

Remove not reachable method calls

3586ee0

Recover or test cases

ca80e8a

Update changelog

a032f94

reenable DynamicRealmTest equalTo_noFieldObjectShouldThrow

9b4b525

Encapsulate tests

9da3b22

Reenable tests affected by core issue 4469

388fc3e

Reenable tests affected by core issue 4475

a6745d2

PR changes

e004400

Remove references to oskeypathmapping

c22e528

Use pass keypathmapping on the query calls

644f5da

Update test cases

90de866

Remove unnecessary test case

d20dc75

Merge branch 'ct/migrate-to-raw-queries' into ct/fix-oskeymapping

a56b79d

Merge pull request #7386 from realm/ct/fix-oskeymapping

9e257ee

Merge branch 'ct/mixed-datatype-story' into ct/migrate-to-raw-queries

db9ebad

Reenable non-latin test case

37a637b

Merge branch 'ct/mixed-datatype-story' into ct/migrate-to-raw-queries

c3bb3ca

Reenable test

89ccbe5

Sort/distinct do not support backlinks

350df13

Validate distinct/sort is not supported on backlinks

04a046f

clementetb commented Mar 26, 2021

View reviewed changes

Remove changelog entry

dcccda2

clementetb requested a review from cmelchior March 26, 2021 08:19

cmelchior approved these changes Mar 26, 2021

View reviewed changes

clementetb merged commit 2f0eb94 into ct/mixed-datatype-story Mar 26, 2021

clementetb deleted the ct/migrate-to-raw-queries branch March 26, 2021 13:59

github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to text based query parser. #7318

Migrate to text based query parser. #7318

clementetb commented Feb 22, 2021 •

edited

Loading

cmelchior left a comment

cmelchior Mar 24, 2021

cmelchior Mar 24, 2021

cmelchior Mar 24, 2021

cmelchior Mar 24, 2021

cmelchior Mar 24, 2021

clementetb Mar 24, 2021

cmelchior Mar 24, 2021

clementetb Mar 24, 2021

cmelchior Mar 24, 2021

cmelchior Mar 24, 2021

clementetb Mar 24, 2021

cmelchior Mar 24, 2021

clementetb Mar 24, 2021

clementetb Mar 26, 2021

	* Queries no longer check do nullability checks on non-nullable fields.
	* Queries no longer do nullability checks on non-nullable fields, so using `null` as an argument will not throw an `IllegalArgumentException`

Migrate to text based query parser. #7318

Migrate to text based query parser. #7318

Conversation

clementetb commented Feb 22, 2021 • edited Loading

cmelchior left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clementetb commented Feb 22, 2021 •

edited

Loading