[WIP] Expose query constrains on the public Query class #178

thatfiredev · 2018-12-21T15:04:55Z

DO NOT MERGE

As discussed in #176, this is a skeleton PR (for discussion) that adds the API surface area.

google-oss-bot · 2018-12-21T15:05:09Z

Hi @rosariopfernandes. Thanks for your PR.

I'm waiting for a firebase member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

samtstern · 2018-12-21T16:20:45Z

@rosariopfernandes thanks for doing this so quickly! I know this is just a skeleton but I think one of the issues is that it now makes these classes part of the API:

com.google.firebase.firestore.core.Filter;
com.google.firebase.firestore.core.OrderBy;
com.google.firebase.firestore.core.Bound;

So as @mikelehen said we'd have to find some way to serialize these into Strings or Integers or something before they could be exposed.

thatfiredev · 2018-12-22T18:58:27Z

@samtstern Thanks for kick-starting the discussion. I've pushed some changes to not expose the core.Bound class.

As for core.Filter, maybe we could create a Filter class that would be part of the public API? Then move the core.Filter.Operator enum to that class to be exposed as well. (if moving this enum is not an option, we'll use the operator's string value instead).

This is what the public Filter class would look like:

Filter
fieldName: String
operator: Operator (or String)
fieldValue: String

The same idea would apply for core.OrderBy.

mikelehen · 2019-01-24T19:58:15Z

@rosariopfernandes Sorry for the silence on this. We had some discussion internally and agree that it would be useful to expose query constraints, but we'll want to be very deliberate about the additional API and it'll need to go through our internal API review process, which could take some time and back/forth.

So if you want to push this forward, I'd say go for it with the following guidance:

Any types in our public API needs to be in the com.google.firebase.firestore namespace (so we don't want to directly expose classes that are under ...firestore.core).
That said, we are interested in re-using types where possible rather duplicating, so e.g. it may make sense to move firestore.core.Filter to firestore.Filter so we can publicly expose it on the Query class without duplicating.
As I mentioned, any API changes need to go through an internal API review process so there may be some delays and several rounds of feedback before we settle all the details... so just prepare yourself.

Anyway, feel free to keep working on this PR (or you could write up a rough proposal in a comment if that's easier to start with), and let us know when it's in shape for us to look at again. Thanks!

thatfiredev · 2019-01-25T17:49:36Z

@mikelehen Thank you for your reply, I totally understand the delay. I'll proceed as instructed :)

thatfiredev · 2019-02-18T09:36:56Z

@mikelehen Sorry for the delay here...

I duplicated the Filter class to avoid exposing the core.Filter, core.NullFilter, core.NanFilter and core.RelationFilter. In my opinion, using a single class to get the filters applied on a query will be easier than using all these 4 classes.

I think this is ready for review.

mikelehen

Sorry for the delay on this, and thanks again for putting it together!

I think this is generally very much like what we want, but I've flagged a number of things that I think need to be tweaked. There are also some things that we need to discuss more internally, which I've flagged with "note to self:" and you can ignore those for now.

If you're up for making the changes I suggest, that would be great. If you're not sure about anything, feel free to punt for the moment.

In any case, the next big step here will be for me to take what you've done, come up with JavaScript and iOS equivalent APIs, and then write it all up in a doc that then gets reviewed internally. I'll add this to my TODO list, but I can't promise when I'll get to it, so there may be more delays. I apologize in advance.

firebase-firestore/src/main/java/com/google/firebase/firestore/Filter.java

mikelehen · 2019-02-28T01:54:22Z

firebase-firestore/src/main/java/com/google/firebase/firestore/Filter.java

+
+    @PublicApi
+    public String getFieldPath() {
+        return filter.getField().canonicalString();


Returning the field path as a String here using .canonicalString() exposes the underlying backend representation of field paths which we don't currently expose in our API, specifically:

Escaped field paths. i.e. If you do a query on FieldPath.of("emails", "[email protected]") then the string returned here will be something like "emails.`test@example\.com`" (since we have to escape . when it's contained within a field).

A query on FieldPath.documentId() will show up as "__name__" here.

I need to discuss this with the team, but I think my suggestion would be to:

Return FieldPath here instead of String

Add a getSegments() method to FieldPath that returns a String[] containing the path segments.

Add a isDocumentId() method that returns a bool indicating whether the FieldPath is equal to FieldPath.documentId().

mikelehen · 2019-02-28T22:18:27Z

firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java

+  }
+
+  @PublicApi
+  public List<OrderBy> getExplicitOrderBy() {


Implicit vs explicit order-by is an internal implementation detail. So I think you can rename this to just getOrderBy().

The reason why I had named it getExplicitOrderBy() was to avoid conflicts with the existing getOrderBy(), which is only used internally. So in order to differentiate the two:

getOrderBy() became getInternalOrderBy()

getExplicityOrderBy() became getOrderBy()

mikelehen · 2019-02-28T22:19:04Z

firebase-firestore/src/main/java/com/google/firebase/firestore/OrderBy.java

@@ -45,19 +50,28 @@ public Direction getDirection() {
    return direction;
  }

+  @NonNull
+  @PublicApi
+  public Query.Direction getSortDirection() {


I'd drop "Sort" and just make this getDirection()

mikelehen · 2019-03-01T15:18:29Z

firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java

+  }
+
+  @PublicApi
+  public @Nullable String getStartAt() {


I think getStartAt() / getEndAt() need to return Object, like getFieldValue() on Filter does, so that you could pass the value back into .startAt(...) to reconstruct the query... But we also need to be able to distinguish the lack of a startAt from the null startAt. So we may need .hasStartAt() that returns bool and .getStartAt() that returns the Object value of the startAt (and probably throws an exception if there is no startAt).

mikelehen · 2019-03-01T15:24:22Z

firebase-firestore/src/main/java/com/google/firebase/firestore/Filter.java

+    @PublicApi
+    public Object getFieldValue() {
+        if (isRelationFilter()) {
+            return ((RelationFilter) filter).getValue().value();


This probably needs a bit of extra work to handle DocumentReference round-tripping. A public DocumentReference will get parsed into a core.ReferenceValue, but the result of calling .value() on that is a DocumentKey, not a DocumentReference so we need to detect that and re-wrap it in a DocumentReference (here and also in getStartAt(), getEndAt()).

mikelehen · 2019-03-01T15:26:30Z

firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java

+  }
+
+  @PublicApi
+  public List<com.google.firebase.firestore.Filter> getFilters() {


Note to self: Since we're exposing filters as Filter objects, should we have a Query.addFilter(...) method in order to re-construct the query? Else, you'd need to do a switch on the operator and then call the appropriate Query.where...() method to match the operator which would be tedious.

mikelehen · 2019-03-01T15:27:44Z

firebase-firestore/src/main/java/com/google/firebase/firestore/core/Bound.java

@@ -70,14 +71,22 @@ public String canonicalString() {
    return builder.toString();
  }

+  public String getFieldValue() {


Per my comment on getStartAt() / getEndAt(), this should probably go away (we don't want / need the field value as a string).

mikelehen · 2019-03-01T15:31:24Z

firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java

@@ -900,6 +899,47 @@ private ListenerRegistration addSnapshotListenerInternal(
        firestore.getClient(), queryListener, activity, wrappedListener);
  }

+  @PublicApi
+  public String getPath() {


Note to self: We need to figure out how we want to expose Collection Group queries (#233) in general.

mikelehen · 2019-03-14T19:31:58Z

FYI- After some internal discussion we do think it probably makes sense to additionally:

Expose public constructors (or static factory methods) for Filter and OrderBy so developers can construct them manually.
Have a .orderBy(OrderBy) overload that allows adding an existing OrderBy directly to a query.
Have a .whereFilter(Filter) method that allows adding a Filter directly to a query.

One additional question that came up was whether we should also be thinking about implementing Parcelable on these classes. Though that may not actually be feasible since a Filter could include include a DocumentReference (i.e. whereEqualTo("embeddedDocReference", firestore.doc('foo/bar')) and DocumentReference contains references to Firestore which can't be Parcelable... so maybe this is a non-starter. But cc/ @samtstern in case he has any thoughts.

samtstern · 2019-03-14T19:41:02Z

FWIW this PR would be really useful in situations like this PR I just got on FirebaseUI:
firebase/FirebaseUI-Android#1599

The developer over there has basically re-implemented Filter and Query and some other classes in order to enable flexible abstractions.

That's interesting that DocumentReference is not Parcelable since we're able to serialize it into a Proto to communicate over the wire, right? Would we be able to do some refactoring to make it Parcelable?

That said I think Parcelable compat is a P2 here and not necessary to make this whole thing useful.

mikelehen · 2019-03-14T21:36:33Z

@samtstern Thanks! Yeah, the problem is essentially that DocumentReference is tied to a FirebaseFirestore instance (to make .set() and all the other DocumentReference methods work) and so we can't recreate a DocumentReference without access to your FirebaseFirestore object... so we could expose DocumentReference.FromParcelable(FirebaseFirestore, Parcelable) but I assume that's not enough. Alternatively, maybe we could create a Parcelable DocumentKey object that is like DocumentReference but doesn't have any of the DocumentReference methods on it... but I'm not sure if that works out well. In any case, it sounds like we can put Parcelable on the back-burner for now and revisit down the road if it seems warranted.

all these changes were made according to feedback on the PR: - Rename Query.getExplicitOrderBy() to Query.getOrderBy() - Rename Filter.getFieldOperator() to getOperator() - Remove isNaNFilter() and isNullFilter() - Remove Bound.getFieldValue()

thatfiredev · 2019-05-01T12:02:00Z

@mikelehen Apologies for the delay here, haven't had much time to work on this. I'll be pushing some commits to address your feedback from time to time, and I'll let you know when it's ready for another review. Your patience is appreciated.

thatfiredev · 2023-04-03T23:10:49Z

This PR is 4+ years old and my use case for this feature is no longer feasible (it relied on the Cloud Functions Spark plan).
Going to close this now

feat: expose query constrains on the Query class

b34151a

google-oss-bot added the do-not-merge/work-in-progress label Dec 21, 2018

googlebot added the cla: yes Override cla label Dec 21, 2018

google-oss-bot added size/M needs-ok-to-test labels Dec 21, 2018

refactor getStartAt(), getEndAt() to return String instead of core.Bound

ee2f364

thatfiredev added 2 commits February 13, 2019 14:10

refactor: expose the OrderBy type as part of the public api

449a701

refactor: duplicate the Filter class to expose filters

e4c50c9

google-oss-bot added size/L and removed size/M labels Feb 18, 2019

mikelehen self-assigned this Feb 27, 2019

mikelehen suggested changes Mar 1, 2019

View reviewed changes

mikelehen assigned thatfiredev Mar 1, 2019

mikelehen removed their assignment Jan 17, 2020

daymxn pushed a commit that referenced this pull request Jan 11, 2023

Banner sizing (#178)

523843d

thatfiredev closed this Apr 3, 2023

firebase locked and limited conversation to collaborators May 4, 2023

[WIP] Expose query constrains on the public Query class #178

[WIP] Expose query constrains on the public Query class #178

Uh oh!

Conversation

thatfiredev commented Dec 21, 2018

DO NOT MERGE

Uh oh!

google-oss-bot commented Dec 21, 2018

Uh oh!

samtstern commented Dec 21, 2018

Uh oh!

thatfiredev commented Dec 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikelehen commented Jan 24, 2019

Uh oh!

thatfiredev commented Jan 25, 2019

Uh oh!

thatfiredev commented Feb 18, 2019

Uh oh!

mikelehen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikelehen commented Mar 14, 2019

Uh oh!

samtstern commented Mar 14, 2019

Uh oh!

mikelehen commented Mar 14, 2019

Uh oh!

thatfiredev commented May 1, 2019

Uh oh!

thatfiredev commented Apr 3, 2023

Uh oh!

Uh oh!

thatfiredev commented Dec 22, 2018 •

edited

Loading