chore(firestore): Log an error message when an array filter is passed a non-array-value type.

[xwkuang5]

This change logs an error message when the operand passed to ARRAY_CONTAINS_ANY, IN and NOT_IN filters are not of array types.

[telpirion]

@xwkuang5 Would you please describe how this is a breaking change in the PR description? Thank you!

[codyoss]

@telpirion I am not sure if you actually want to label this as a breaking change? If you are doing this there is module renaming and a bunch of other stuff to consider. If this is fixing a case that has always errored server side it may be better to just take it as a fix and have a nice clear description in the commit message of what/why.

[telpirion]

@codyoss Couldn't have said it better myself! @xwkuang5 please provide description of how this will break customers. Otherwise, I think we should follow @codyoss 's advice and just label this as a fix.

[xwkuang5]

Thanks for the comment. I updated the PR description. While I agree the change is small, this is indeed an user-observable breaking change:

Suppose users are sending us the following queries:

query := cities.Where("country", "in", nil);
query := cities.Where("country", "not-in", nil);
query := cities.Where("country", "array-contains-any", nil);

Before this change, the returned query result is an empty result set. After this change, users will see a client-side error.

That said, I don't have a lot of experience with semantic versioning in client SDKs so I will defer to owners for whether this should be labeled a breaking change or a fix.

[codyoss]

If it is determined this change should be considered breaking perhaps it is better, for now at least, to implement it in a non-breaking way. Maybe this "feature" can be feature flagged and off by default to change the API behavior for existing users, but allowing them to opt-in to the different behavior.

[xwkuang5]

I think I'm missing some context around how we release breaking changes in the SDK. What is the reason for the qualifier "for now at least"? Is it because we don't want to cut a breaking release too often? Allowing them to opt-in the different behavior is likely not very useful:

1. For users who are using the APIs correctly, the change is not breaking for them
2. For users who have code paths hitting the issues mentioned above, they can just update their codes to supply the right value type to the operator

I'm also curious how we deal with such bugs in general. Is there any past examples I can reference from?

[xwkuang5]

PTAL, thanks