cachapa / firedart

A dart-native implementation of the Firebase Auth and Firestore SDKs
https://pub.dev/packages/firedart
Apache License 2.0
174 stars 62 forks source link

Collection Queries #23

Closed SaadArdati closed 4 years ago

SaadArdati commented 4 years ago

I took the collection query code from PR #19 and redid it so it is more in-line with official firestore sdks.

Also tested, can't share my test file but I did add a dummy test that should succeed in theory in private_test.dart

SaadArdati commented 4 years ago

Don't be fooled by the code formatting quality check..

sjapps commented 4 years ago

i'm using this library and every time I see a feature missing, I come to the PR and I see a PR from you with the feature ❤️. First the admin and now this. Collection query is a must.

cachapa commented 4 years ago

Hey, thanks a lot for your effort. Looks good on the surface, but could you share private_test.dart that you mention in your first comment? It seems it was added to .gitignore and never commited.

Don't be fooled by the code formatting quality check..

Why's that? It seems to be failing for firestore/models.dart.

SaadArdati commented 4 years ago

@cachapa > Hey, thanks a lot for your effort. Looks good on the surface, but could you share private_test.dart that you mention in your first comment? It seems it was added to .gitignore and never commited.

I can't, its sensitive to my database. ... It's called privatetest.dart >>

cachapa commented 4 years ago

Sorry I misunderstood your comment to mean that you had created stub tests. Ok no worries, I'll experiment a bit and merge your PR if there's nothing else.

SaadArdati commented 4 years ago

👍

SaadArdati commented 4 years ago

I'll see what I can do

SaadArdati commented 4 years ago

@cachapa This is how it works in java.

        QuerySnapshot queryDocumentSnapshots = Firebase.firestore.collection("myCollection").whereWhateverFilters().limit(15).get();

So I think we should add a new builder thing for filters and return a QuerySnapshot object

https://github.com/googleapis/java-firestore/blob/master/google-cloud-firestore/src/main/java/com/google/cloud/firestore/QuerySnapshot.java

SaadArdati commented 4 years ago

Honestly, I think I just want to return a list of documents when running a query.... That should be more than fine for all cases. If anyone wants to complain, they can submit an issue...

cachapa commented 4 years ago

Yeah I think that's perfectly acceptable for now. Being able to specify the limit should cover the vast majority of use cases and if someone absolutely needs paging queries, it shouldn't be too hard to add them in.

SaadArdati commented 4 years ago

I think the query API could be improved to be a bit more "darty" by having a single Collection.where() method that would accept diverse filters as argument. Similar to the Dart test framework API. so this is funny because the PR i based this off of actually did exactly this. But I didn't like how it looked so I split it up to match java.

And looking at the official firestore pub.dev package, that's exactly what they did...

https://github.com/FirebaseExtended/flutterfire/blob/master/packages/cloud_firestore/cloud_firestore/lib/src/query.dart#L56

Also CollectionReference extends Query, so I'll do that.

SaadArdati commented 4 years ago

I'm getting this error in the query test: gRPC Error (3, kind is required for filter: test_field[*]) Do you know what it is?

i need more info, what's the code, and what is the full error?

SaadArdati commented 4 years ago

Huh, I actually managed to reproduce your error...

SaadArdati commented 4 years ago

Fixed it.

SaadArdati commented 4 years ago

That should be everything done

SaadArdati commented 4 years ago

So your solution isn't exactly what I was thinking about. I meant creating a QueryOperator abstract class that you would pass to the where() method. Your solution works but there's no way to guarantee order in which the operators are going to be executed - they will be executed in the arbitrary order you used in your implementation.

I don't want to delay the implementation any longer so if you're ok with this I can merge your solution now and refactor it later.

Definitely something for another time :P Merge this thing

cachapa commented 4 years ago

Alright, code looks good, tests are passing. Thanks a lot!