-
Notifications
You must be signed in to change notification settings - Fork 940
Allow nested arrays for IN queries #2346
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
if (context.arrayElement) { | ||
// Nested arrays are allowed when making IN queries, so we make the | ||
// exception here. | ||
if (context.arrayElement && context.methodName !== 'Query.where') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works in JS but is a little fragile (relies on string equality of magic vales) and won't be portable to other platforms since they don't have access to the methodName. [The only reason we have the methodName in JS is because we try to include it in the error message since (historically at least) in some cases you may not have a meaningful callstack to show you what method the error came from.]
I think it'd probably be better to use context.dataSource
which is a more formalized way to know where the data came from and how we should parse it. We could probably get away with context.dataSource !== UserDataSource.Argument
here. This means we will disable the nested-array validation in more cases, but I think it's probably okay. If we want to keep it more targeted, we could introduce UserDataSource.ArrayArgument
, indicating that we're parsing an array and so "nested" arrays is okay.
The other option I had mentioned previously would be to just parse the elements of the array individually for 'in' filters, e.g. here we would do:
fieldValue = new ArrayValue((value as unknown[]).map(val => this.firestore._dataConverter.parseQueryValue('Query.where', value)));
I don't have strong feelings, so whichever of these sounds good to you works for me.
I realized that we can't use the I tried naming, but I'm not sure if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach looks good, but I'm nitting you some new comments. 😛
Also making another IN filters integration test run against prod.