google / go-cloud

The Go Cloud Development Kit (Go CDK): A library and tools for open cloud development in Go.
https://gocloud.dev/
Apache License 2.0
9.57k stars 812 forks source link

docstore/memdocstore query for nested documents with slices does not work #3506

Open eqinox76 opened 1 week ago

eqinox76 commented 1 week ago

Describe the bug

Documents like

docmap{
    "goals": []any{
        docmap{"id": "1145755"},
        docmap{"id": "8336474"},
        docmap{"id": "1474479"},
    },
}

are not found by the following query:

collection.Query().Where("goals.id", "=", "1145755")

This is working fine when using docstore/mongodocstore.

To Reproduce

I forked this repository and added a unit test to show the issue eqinox76/go-cloud@7f3ef044a00cc76d258147fd9e1b529e0b4f8ad9.

Expected behavior

Find the document.

Version

v0.39.0

Additional context

eqinox76/go-cloud@7f3ef044a00cc76d258147fd9e1b529e0b4f8ad9 contains a potential fix. If it looks good to you i could make a pr.

jba commented 1 week ago

Can you send a PR with a fix?

vangent commented 1 week ago

If this is expected to work, the test should be in drivertest so it gets checked for all providers, not just memdocstore.

eqinox76 commented 6 days ago

I think https://github.com/google/go-cloud/pull/3508 should fix it. Let me know what you think.

jba commented 6 days ago

Thanks for the PR. It helped me understand this issue better. It is not a bugfix, it is a new feature.

The docstore documentation about field paths is underspecified; it never says how they work. For maps and structs it's obvious, but not so for slices.

I understand that MongoDB treats s.f, where s is a sequence, as the slice of e.f for each e in s. And your PR does the same for memdocstore. The question is how other providers interpret that. If they do the same, we can add the feature. If not, we can't.

I just checked for Firestore and it doesn't support field selectors inside slices at all.

I don't know what DynamoDB does, but I think Firestore is the only counterexample we need.

The remaining question is: should we add this behind some option, so that people primarily using MongoDB can turn it on to test with memdocstore? I don't know if we do that elsewhere. I'd be OK with it, but @vangent has the final say.

eqinox76 commented 5 days ago

Ah i see. The mongodb driver supports that behavior a bit by accident.

I like the option idea that would make it easier to test with the memdocstore when using only the mongodb driver. But its of course adding more complexity.

vangent commented 3 days ago

I'm OK with adding an option to memdocstore to support this, can you update your PR to remove the drivertest changes and add that option? You should be able to add it to the existing Options struct.