Skip to content
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

Firestore security rule emulator - get and getAfter throwing an exception on non existant doc #2067

Open
olivierkrener opened this issue Mar 25, 2020 · 27 comments

Comments

@olivierkrener
Copy link

[REQUIRED] Environment info

**firebase-tools:**7.16.1

**Platform:**Windows

[REQUIRED] Test case

Just write a rule using a get or getAfter on a non existant doc.
For instance:
allow update: if get(/databases/$(database)/documents/doc/doesnot/exist) == null;

[REQUIRED] Steps to reproduce

See above

[REQUIRED] Expected behavior

As per the doc get and getAfter should return null. this is what the online simulator does.

[REQUIRED] Actual behavior

An exception is thrown
Service call error. Function: [get], Argument: [path_value {
...}

@samtstern
Copy link
Contributor

@olivierkrener thanks for reporting, @scottcrossen is looking into this.

@scottcrossen
Copy link
Contributor

@olivierkrener Thanks for reporting it! The fix for this is actually quite simple, but there's a lot of organizational complexity surrounding it which will delay the release of the fix by around 1-2 months.

@olivierkrener
Copy link
Author

Ok, thanks for the update, I am doing without for the time being

@scottcrossen
Copy link
Contributor

scottcrossen commented Mar 27, 2020

Okay! That's great to hear. I'm sorry you're experiencing this. Because the emulator mirrors the production experience, you may also encounter this in production. One workaround is to write a global function like this:

rules_version='2';
// Overload firestore-provided function at global scope.
function get(path) {
  // You'll have to wait for the ternary operator to be released in the emulators
  return exists(path) ? get(path) : null;
}
service cloud.firestore {
  // Your code here.
}

It's not ideal because in production this may count as two data lookups instead of one, but it works. Also keep in mind that rules logic is 'short-circuiting' and that includes error handling. For example, `allow read: if get(/bad/path).boolean_field || true' would return true even though the first branch is an error.

Edit: It probably doesn't count as two rules lookups

@olivierkrener
Copy link
Author

And that's why I raised an issue for the ternary operator ;)

@olivierkrener
Copy link
Author

You say that

this will count as two data lookups instead of one

Can you elaborate? I thought that data lookup are counted by document reference. So doing 10x get(pathdoc) only counts as 1 data lookup in the end. I got this confirmed by one of your colleague on stackoverflow
https://stackoverflow.com/questions/60676202/firestore-security-rules-how-are-reads-counted-for-exists-get-getafter

@olivierkrener
Copy link
Author

Apparently this is no working in prod either, as you mention

@bumpmann
Copy link

Confirmed, through i could not use ternary operator with the emulator.

@scottcrossen
Copy link
Contributor

Can you elaborate?

Doug would know better than me about the billing and limitations that are customer facing. What he says is generally canon.

@scottcrossen
Copy link
Contributor

Update: The first PR was merged on March 26th that will begin to fix this. It will take a few more months for it to be fully fixed because of deploy schedules.

@samtstern
Copy link
Contributor

@scottcrossen can you close this issue when the fix is rolled out? Also is there an internal bug I could follow?

@olivierkrener
Copy link
Author

@scottcrossen can you close this issue when the fix is rolled out? Also is there an internal bug I could follow?

In what release is this planned to be rolled out?

@scottcrossen
Copy link
Contributor

scottcrossen commented Jun 3, 2020

Not sure if I'm supposed to link internal bugs here, but YOLO for posterity: b/152525307

@samtstern

Unfortunately I haven't followed up with the current assignee for a while. I'm sorry about that. I'll start annoying him to work on it. This is pretty egregious and needs to be fixed.

@kaiserschmarrn0
Copy link

clever way to bring down the max allowed get calls if i do say so myself

@scottcrossen scottcrossen removed their assignment Jan 18, 2021
@wmadden
Copy link

wmadden commented Apr 22, 2021

Any progress here?

@SamyPesse
Copy link
Contributor

An issue with the current workaround with the ternary is that it consumes more expressions compared to a direct get call and Firestore has a 1000 limit on expressions.

@t4tapas
Copy link

t4tapas commented Aug 28, 2021

Is there any update on this issue? Using the workaround in frequent actions will cose double the amount of reads.

@moekify
Copy link

moekify commented Mar 31, 2022

How can a bug like this one be open for over two years? :'( BUMP

@Daspy11
Copy link

Daspy11 commented Jul 1, 2022

...bump, still broken

@VGerris
Copy link

VGerris commented Oct 8, 2022

this is still not working. also, the workaround gives me the following on the emulator output:
i firestore: Change detected, updating rules...
⚠ firestore.rules:3:10 - ERROR Recursive call is not allowed.
⚠ firestore.rules:5:10 - WARNING Call to service function 'exists' occurs outside of defining scope.
✔ firestore: Rules updated.

I don't understand the error since the document exists as far as I can see.

@christhompsongoogle
Copy link
Contributor

Thanks for reporting your experience VGerris, we're aware of this issue (exists in production too) but we have no expected fix timeline.

@finallyblueskies
Copy link

finallyblueskies commented Apr 22, 2023

Bump, this is still an issue. Any chance there's been progress on this?

@jellynoone
Copy link

Just bumped into this.
I'm trying to perform a transactional write, where two documents are created as a result and one of them is validated against another.
Is there any other way to achieve this without bumping into this bug?

@VGerris
Copy link

VGerris commented May 4, 2023 via email

@Branchverse
Copy link

Still an issue, please fix

@LarsFlieger
Copy link

Any update on this issue?

@sdirosa
Copy link

sdirosa commented Dec 6, 2023

Started experiencing this on a very specific existsAfter call on only 2 of our 10+ dev environments. It throws an exception instead of returning as expected.

Anything we could do to debug this further? E.g. What could be affecting the affected envs, compared to the others?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests