Skip to content

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

Merged
merged 3 commits into from
Nov 9, 2019
Merged

Allow nested arrays for IN queries #2346

merged 3 commits into from
Nov 9, 2019

Conversation

thebrianchen
Copy link

Also making another IN filters integration test run against prod.

if (context.arrayElement) {
// Nested arrays are allowed when making IN queries, so we make the
// exception here.
if (context.arrayElement && context.methodName !== 'Query.where') {
Copy link
Contributor

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.

@thebrianchen
Copy link
Author

I realized that we can't use the context.dataSource !== UserDataSource.Argument check since calling doc.set({ x: FieldValue.arrayUnion(1, ['nested']) }) results in a timeout (didn't dig too deep).

I tried naming, but I'm not sure if allowArrays captures the essence of what we're going for here.

Copy link
Contributor

@mikelehen mikelehen left a 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. 😛

@mikelehen mikelehen assigned thebrianchen and unassigned mikelehen Nov 8, 2019
@thebrianchen thebrianchen merged commit 14b2878 into master Nov 9, 2019
@thebrianchen thebrianchen deleted the bc/in-nested branch November 9, 2019 01:12
@hsubox76 hsubox76 added this to the next milestone Nov 13, 2019
@firebase firebase locked and limited conversation to collaborators Dec 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants