Google-cloud-dotnet: [firestore] Shouldn't a start/end query cursor allow empty set?

[Thaina]

C# var anomaly = await firestoreDB.Value.Collection("col").OrderByDescending("field").StartAt(new object[0])

This above line cause an error does not allow empty set at a start/end query cursor

But doesn't this should be allowed for query the empty array and null? Actually it should let me got the whole documents which has value of that field not an array nor object?

Is it really a limit in firestore or just this library?

[jskeet]

Am I right in saying the error message is actually the following?

Cannot specify an empty set of values for a start/end query cursor

It's helpful if you copy/paste the exact error message - I initially searched for what you posted (with "does not" instead of "Cannot") and that's not in the repo, so it took longer to find this.

But yes, this is a deliberate restriction, and is documented. (fieldValues can't be empty.) If you created an array with a single (null) element, that would be fine. That said, I don't think the Java library has this restriction, so we may be able to remove it. I'll consult with the team.

[Thaina]

@jskeet Yes that's the correct message

[jskeet]

Right. I've asked internally about the intended behavior. It should be easy to fix if it should be allowed - and we'll add a conformance test either way to make sure all languages are consistent.

[jskeet]

Just checked, and yes, it should be allowed. I'll see if I can get to that this week.

[jskeet]

@Thaina I'm reopening this because we've rethought the policy across all SDKs. Previously, C# was the only SDK to validate this explicitly, although Node.js did so _implicitly_, throwing a not-terribly-helpful exception.

We (the Firestore team and client library developers) have decided that we should validate this and prohibit an empty set of values - reverting the previous change. We don't think many developers want this functionality (as otherwise they would have requested it in Node.js) and we'd prefer them to receive an exception if they accidentally provide no values. However, it should be easy enough for you to add an extension method to your codebase to allow you to handle your specific use case. You'd write something like:

public static class QueryExtensions
{
    public static Query MaybeStartAt(this Query query, object[] values) =>
        values.Length == 0 ? query : query.StartAt(values);
}

(You could add null validation for both query and values if you want.)

Does that sound okay? I'd like to make the change reasonably soon, but I don't want to release with the restriction in place until you've had a chance to add the workaround.

[Thaina]

I think this should not be the case to prohibit

The reason is. The empty array value is the valid value of field in firestore

{
    "__name__" : "A",
    "field" : [] // valid and should be able to queried 
}

Also what happen if we use this document as the StartAt or StartAfter?

Suppose we query by OrderBy("field").Limit(10) and this document just happen to be the 10th and so we use the same query to OrderBy("field").Limit(10).StartAfter(results.Last())

```C#
var results = await fireStoreDB.OrderBy("field").Limit(10).StreamAsync().ToArray();
// This would be unexpected random error
var another10 = await fireStoreDB.OrderBy("field").Limit(10).StartAfter(results.Last()).StreamAsync().ToArray();

```

Also in the specification of firestore, the Array, even it's empty, would be ordered after the string and before object, not null. Same go for the object, even empty will be ordered after any array. So trying to query it after null is not really correct

I think we should put that exception in js as a bug instead

[jskeet]

It's fine to pass an empty value as "a value in the StartAt sequence". What's not fine is to post on values.

So for example, I'd expect this to work:

var emptyValue = new List<string>;
query = query.StartAt(new object[] { emptyValue });

But this shouldn't:

query = query.StartAt(new object[0]);

That's trying to start at "an empty sequence of values" rather than "a sequence of values with a single element, which is an empty list/array".

[Thaina]

@jskeet I think in firestore we could not have array in array?

And also I think the array with 1 element, even the element itself is just null or another empty element, would be ordered after the array with 0 element too

[jskeet]

All of that is irrelevant though: the point is that you can provide an empty element as "the value to start at", but you can't provide an empty sequence, i.e. no values saying where to start.

[Thaina]

@jskeet Also by empty set, I think you would also mean empty object?

Empty object also ordered after every array. And it would be hard to specified the least value of object too

And also I think the firestore REST API does not prohibit the empty array or empty object isn't it? If firestore server itself permit the value, I don't think it should be any reason to prohibit it on the SDK

[jskeet]

I think all of this is explained by StartAt accepting a parameter array. StartAt(null) doesn't try to start at a single null value - it's passing null as the sequence itself. Likewise StartAt(new object[0]) is passing the empty array as the sequence of values. That's not what you intended to do.

It's easy to fix that: just cast to object first or use an object local variable:

object value = null;
query = query.StartAt(value);

That will then be handled by the compiler as:

object value = null;
query = query.StartAt(new object[] { value });

And that's what you intended. You're trying to pass a single value - but because both the null literal and new object[0] are applicable without parameter array expansion, the compiler isn't creating a "wrapper" array for you.

This isn't specific to Firestore - here's an equivalent example just using Console.WriteLine:

// Succeeds
object value = new object[0];
Console.WriteLine("Value: {0}", value);

// Fails
Console.WriteLine("Value: {0}", new object[0]);        

[jskeet]

Empty object also ordered after every array. And it would be hard to specified the least value of object too

That's completely irrelevant, because you're not passing in an empty object as "the thing to start at". You're passing no objects as "the sequence of values to start at" and that's the problem. See my previous comment for how to get it to do what you want.

[Thaina]

@jskeet If I understand you correctly, what you trying to say is, because firestore StartAt function (and Console.WriteLine) are using params so it ambiguously between object[0] as params or as an array itself

But then this is not related to passing empty set. It should just be solved by putting StartAt(new List<object>()) which would not ambiguous with params, without the need to wrap it with array

[jskeet]

@jskeet If I understand you correctly, what you trying to say is, because firestore StartAt function (and Console.WriteLine) are using params so it ambiguously between object[0] as params or as an array itself

Yes.

But then this is not related to passing empty set. It should just be solved by putting StartAt(new List<object>()) which would not ambiguous with params, without the need to wrap it with array

Yes, I'd expect that to be fine - the compiler will wrap it in an object[]. There are lots of ways of making sure that what the method receives is an array with a single element, rather than an empty array.

[Thaina]

@jskeet I see. Thank you for your clarification

[jskeet]

Okay - with all that sorted out, are you happy for me to revert the previous change "pretty much immediately"?

[Thaina]

@jskeet Yes, but could you please add a test for StartAt(new List<object>()) too?

[jskeet]

Sure, happy to. (It's effectively covered in the new conformance tests which I haven't merged yet, but I can do it more explicitly.)

[Thaina]

@jskeet A little bit wonder

Is it possible to revise the API of StartAt(params object[] items) to StartAt(object item,params object[] additionals) ?

Because this API actually require at least one item, it would be better to design the API to reflect the requirement at compile time. And it would also solve the single argument ambiguous for both StartAt(null) and StartAt(new object[0])

[jskeet]

@Thaina: I considered that, but then it would make it much harder to call if you already had an object[]. (You'd have to extract the first item, then create a separate array with the other items.) The various workarounds with the current design are really simple. If you find yourself wanting this frequently, you could always create your own extension method for a single item (StartAtOneValue or whatever) - you should be able to chain to StartAt very easily.