realm / realm-swift

Realm is a mobile database: a replacement for Core Data & SQLite
https://realm.io
Apache License 2.0
16.31k stars 2.15k forks source link

Query Syntax: Incorrect NSPredicate When Using `.values` on a Map Property #8654

Open bdkjones opened 3 months ago

bdkjones commented 3 months ago

How frequently does the bug occur?

Always

Description

Consider these objects:

class Flag: Object 
{
    @Persisted var isLoud: Bool = true
}

class Bar: Object
{
    @Persisted var flags: Map<String, Flag?>
}

I want to query the database for all Bar objects whose flags map contains a loud Flag. So I write this:

realm.objects(Bar.self).where({ ($0.flags.values.isLoud == true).count > 0 })

Realm transforms that query syntax into this NSPredicate, which throws an exception at runtime:

(SUBQUERY(flags, $col1, $col1.@allValues.isLoud == true).@count > %@)
Exception:  Invalid keypath '@allValues.isLoud': @allValues must follow a dictionary property.

I think @allValues is misplaced here because each value in the collection (referenced by the variable $col1) does not have an @allValues property; it's a single object. I think the correct NSPredicate should be:

(SUBQUERY(flags.@allValues, $col1, $col1.isLoud == true).@count > 0)

Replacing the Realm-Query-Syntax-Produced predicate with this manual one resolves the exception. (I don't know if it actually fetches the Bar objects yet, though, because I got sucked into this issue.)

Can you reproduce the bug?

Always

Reproduction Steps

Run the sample code above.

Version

10.52.1

What Atlas Services are you using?

Both Atlas Device Sync and Atlas App Services

Are you using encryption?

No

Platform OS and version(s)

macOS 14.3

Build environment

Xcode version: 16 Beta 3 Dependency manager and version: SPM

sync-by-unito[bot] commented 3 months ago

➤ PM Bot commented:

Jira ticket: RCOCOA-2412

bdkjones commented 3 months ago

Updated the issue with a more concise summary.

An Aside:

It would be nice if the Realm Query Syntax API were smart enough to add == true when required. My initial query was:

($0.flags.values.isLoud).count > 0

The compiler showed no errors here, but at runtime the resulting predicate failed to parse and it took me a minute to realize why, since the other issue with @allValues misplacement happened simultaneously. I think most Swift developers don't write == true when evaluating a boolean, so that habit carries over into the query syntax and it'll bite you. If possible, Realm should consider adding handling for that or a more explicit warning about the footgun.

Jaycyn commented 3 months ago

I am not exactly sure what the expected result is so let me take a crack at it.

Looking at the code - it appears the goal is to return all Bar objects where the flags Map property contains isLoud == true.

Allow me to set up a test case

let f0 = Flag()
f0.isLoud = false

let f1 = Flag()
f1.isLoud = true

let f2 = Flag()
f2.isLoud = false

let f3 = Flag()
f3.isLoud = false

let bar0 = Bar()
bar0.flags["a"] = f0
bar0.flags["b"] = f1 //f1.isLoud == true

let bar1 = Bar()
bar1.flags["c"] = f2
bar1.flags["d"] = f3

try! realm.write {
    realm.add(bar0)
    realm.add(bar1)
}

So the result here is that bar0's flags property "b" key contains value f1 where isLoud is true but bar1 (which contains f2 and f3, has no flags property where isLoud is true.

The goal is return bar0 since it has at least one flags property where isLoud == true

Doesn't this query solve that?

let results = realm.objects(Bar.self).where { $0.flags.values.isLoud == true }
print(results)

and the result is bar 0

Results<Bar> <0x151e3bd40> (
    [0] Bar {
        flags = Map<string, Flag> <0x6000014b5800> (
        [a]: Flag {
                isLoud = 0;
            },
        [b]: Flag {
                isLoud = 1;
            }
        );
    }
)

likewise if we change f2, which is in bar1 to have isLoud set to true, both bar0 and bar1 are returned.

let f2 = Flag()
f2.isLoud = true

I think the .count is extraneous in this case as any flags within a bar where isLoud == true will be returned. e.g. if the count was 0, it would not be returned because there is no isLoud == true.

Let me know if the desired result is different than my guess.

bdkjones commented 3 months ago

Perhaps I've not been clear.

Realm translates .where{} clauses into NSPredicate objects on-the-fly. It would appear that this translation layer is placing @allValues in the wrong spot.

Whether .count > 0 is strictly necessary is not the point. The point is that given the where{} clause above (which IS valid), the translation-on-the-fly layer is producing an invalid NSPredicate.

In other words, adding .count > 0 should not cause the translation layer to place @allValues where it is when a SUBQUERY is written by that translation layer.

(In my actual app, there's several parts to this query. I've narrowed it down to a simplified example.)

bdkjones commented 3 months ago

@Jaycyn here's another way to see the problem: suppose we wanted to fetch all Bar objects that have exactly 3 loud flags in their Map property.

Now .count == 3 is needed and you should be able to reproduce the bug I'm reporting here.

Jaycyn commented 3 months ago

Duplicated. Kinda odd as well. Looks like the placement is in the wrong place as you mentioned.

This predicate as suggest in the initial report works and has the correct placement for @allValues

let subquery = NSPredicate(format: "SUBQUERY(flags.@allValues, $flag, $flag.isLoud == true).@count == 3", "")

bdkjones commented 2 months ago

Is there anything else you guys need to fix this? My workaround eliminates type safety and adds fragility, so I'd like to get back to the Realm Query Syntax when this is solved.

I'm unclear if @Jaycyn works for Mongo, but he was able to duplicate the bug at least.

Jaycyn commented 2 months ago

No, I do not work for Mongo and yes this is definitely not working as intended.

I would also not set an expectation this will be reviewed any time soon. Support for the platform appears to be minimal at this time - hardly a peep from anyone in months. Perhaps there's simply not enough support employees to handle the amount or depth of issues? Hopefully they will acknowledge and get to it in the future.

Your workaround, while not ideal, will be your best bet.

aehlke commented 2 months ago

@Jaycyn I noticed a drop off in project momentum (like features) and support once the focus shifted to the sync service. Even major core features such as full-text search are absent from RealmSwift. Most updates for a long while are centered around syncing now, which is their business model. I don't use the syncing so it has been especially frustrating to see this stall out, especially now that SwiftData has released and shown itself to be relatively immature and unusable, leaving few alternatives / hope for the future. (Sorry for the aside.)

(edit: There are some hard working employees who do follow up sometimes if you ping them directly in here - don't mean to shade their service when the above is a complaint about their employer's direction)

nirinchev commented 2 months ago

I don't mean to dismiss your concerns and I'll be the first to admit we have shifted focus. This is not surprising though - Realm is no longer a startup, it's part of MongoDB which is for-profit business and it's expected that paying customers would be prioritized in the current macroeconomic environment. Obviously issues with the core database affect those customers, so we're trying to address them based on feedback and open tickets, but we only have limited resources and we have to be very deliberate deciding what to work on.

That being said, Realm is a fully open source project and if members of the community are willing to dip their feet in SDK development, we'd be happy to merge well thought out and tested PRs, even if they primarily benefit local use cases.

bdkjones commented 2 months ago

It's not my intent to start a "local vs sync" debate.

(I think I'm actually one of the heaviest Atlas Device Sync users around--at least in how much my app demands from the service, if not in sheer database size. I've gathered that I'm an edge case for the sync service, at least.)

This bug isn't really a local-vs.-sync issue; it affects both groups. The issue has been silent for a month and didn't have any replies from Mongo staff, so I was just following up.

Jaycyn commented 2 months ago

Thanks for that insight @nirinchev

I think it would help us to understand the direction if this is an acknowledged bug or if we are simply doing something wrong/incorrect expectation and it's working as intended. If so, can you provide clarity on how it should be done?

Regardless of Sync/No Sync, Realm is promoted as an "Offline first database"

Network reliability: The SDKs are offline-first. You always read from and write to the persistence layer on the device, not over the network.

So while shifting focus is great because sync'ing is very important, having focus on the Offline First aspect is equally as important. Many of the bugs we encounter affect both, and if your core isn't solid, sync'ing will be a mess.

IMO, we've been in limbo for a couple of years, unable to release products backed by Realm. So while shifting focus is warranted, not addressing core Realm SDK (errr, Atlas Device SDK) issues like this prevent us from becoming paying customers.

nirinchev commented 2 months ago

It's definitely a bug or at least a use case we didn't think through. And I completely agree - it's a deficiency that impacts both paying customers and people using the open source local-only database. I was merely trying to provide context for why it hasn't been addressed in a while - we have limited resources, this issue has a workaround - even if unpleasant - and there are more pressing tasks we have been focusing on.