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

Add support for value substitution in string based queries #2201

Merged
merged 49 commits into from
Feb 24, 2021

Conversation

LaPeste
Copy link
Contributor

@LaPeste LaPeste commented Jan 25, 2021

Description

Adding support for value substitution meant updating the wrapper to supply an array of mixed value to core. And from the C# side passing a marshable array of PrimitiveValue to the wrappers.
Documentation for the syntax of value substitution can be found here.

Fixes #1822

TODO

Nothing left

  • Changelog entry
  • Tests (if applicable)

=============================
UPDATE:

  1. The test QueryFilter_WithArgumentsUnmanagedObjects_ShouldThrow is currently failing because the SDK does not throw when an in-memory object is passed; instead the SDK sends a RealmValueType.Null to core when an in-memory object is supplied. This will need to be corrected, but not by this PR.
  2. The test QueryFilter_WithArgumentsObject_ShouldMatch is currently failing because of an issue in core.
  3. The test QueryFilter_WithAnyEmbeddedObjectArguments_ShouldMatch fails for the same issue of test 2.

LaPeste and others added 29 commits January 13, 2021 16:08
This reverts commit 89ae7f5.
Co-authored-by: Nikola Irinchev <irinchev@me.com>
@LaPeste LaPeste requested a review from papafe January 26, 2021 10:10
@LaPeste LaPeste marked this pull request as draft January 29, 2021 14:26
@LaPeste LaPeste marked this pull request as ready for review February 1, 2021 10:12
Copy link
Member

@nirinchev nirinchev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some final comments mostly related to tests/simplifying the conversion logic as that was quite hard to grasp/follow. In addition to that, some extra tests you might want to add:

  1. Tests for object matches that use embedded objects. You can use ObjectWithEmbeddedProperties and EmbeddedAllTypesObject. Then, the tests should verify that this works: realm.All<ObjectWithEmbeddedProperties>().Filter("AllTypesObject = $0", theEmbedded)
  2. Test that null is passed correctly as a value. e.g. realm.All<Foo>().Filter("Prop = null") should be equivalent by realm.All<Foo>().Filter("Prop = $0", (int?)null) (assuming Prop is int?). Note that you need the explicit cast as implicit null casts don't work yet and may not work due to reasons @papafe will explain on Wednesday.

CHANGELOG.md Outdated Show resolved Hide resolved
Tests/Realm.Tests/Database/TestObjects.cs Outdated Show resolved Hide resolved
Tests/Realm.Tests/Database/TestObjects.cs Outdated Show resolved Hide resolved
Tests/Realm.Tests/Database/CollectionTests.cs Outdated Show resolved Hide resolved
Tests/Realm.Tests/Database/CollectionTests.cs Outdated Show resolved Hide resolved
Tests/Realm.Tests/Database/CollectionTests.cs Outdated Show resolved Hide resolved
Tests/Realm.Tests/Database/CollectionTests.cs Outdated Show resolved Hide resolved
Tests/Realm.Tests/Database/CollectionTests.cs Outdated Show resolved Hide resolved
@LaPeste LaPeste marked this pull request as draft February 1, 2021 13:27
@LaPeste LaPeste marked this pull request as ready for review February 4, 2021 14:47
@@ -210,16 +211,18 @@ public static IDisposable SubscribeForNotifications<T>(this IDictionary<string,
/// var results1 = realm.All&lt;Foo&gt;("Bar.IntValue > 0");
/// var results2 = realm.All&lt;Foo&gt;("Bar.IntValue > 0 SORT(Bar.IntValue ASC Bar.StringValue DESC)");
/// var results3 = realm.All&lt;Foo&gt;("Bar.IntValue > 0 SORT(Bar.IntValue ASC Bar.StringValue DESC) DISTINCT(Bar.IntValue)");
/// var results4 = realm.All&lt;Foo&gt;("Bar.IntValue > $0 || (Bar.String == $1 &amp;&amp; Bar.Bool == $2)" 5, "small", true");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// var results4 = realm.All&lt;Foo&gt;("Bar.IntValue > $0 || (Bar.String == $1 &amp;&amp; Bar.Bool == $2)" 5, "small", true");
/// var results4 = realm.All&lt;Foo&gt;("Bar.IntValue > $0 || (Bar.String == $1 &amp;&amp; Bar.Bool == $2)", 5, "small", true);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this posted only now? I do remember not seeing the suggestion after the talk in the call. Anyway, it's already fixed.

@LaPeste LaPeste merged commit 5287703 into master Feb 24, 2021
@LaPeste LaPeste deleted the ac/value-substitution-in-str-queries branch February 24, 2021 11:29
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for value substitution in string based queries
3 participants