fix: Update the depth validation used when writing documents#1815
fix: Update the depth validation used when writing documents#1815MarkDuckworth merged 3 commits intomainfrom
Conversation
… it matches the validation of the Firestore backend.
product-auto-label
Bot
added
size: l
|
The behavior in question: is path "foo.bar.baz" 3 levels deep or 2? Admin SDK says it's 3 but DBE says 2. The answer is not important since the backend already has customer data that is "21" levels deep. This PR modifies the depth validation in this SDK to compute the depth the same way. |
dconeybe
left a comment
dconeybe
left a comment
There was a problem hiding this comment.
One minor comment, but LGTM otherwise.
product-auto-label
Bot
added
size: m
| inArray?: boolean | ||
| ): void { | ||
| if (path && path.size > MAX_DEPTH) { | ||
| if (path && path.size - 1 > MAX_DEPTH) { |
There was a problem hiding this comment.
this may be the simplest workaround for the issue but may not be the best fix.
IMO, L348 is the one that's problematic, it adds the path of a top-level map property to the accumulating path, creating the off-by-one error. "foo" in {"foo": {"bar": "baz"}} have depth 0 but this implementation currently calculates it as depth 1.
Currently, path serves two purposes:
- to be used in the fieldPathMessage (as part of error message)
- to be used for counting the depth
I think we should separate this responsibility out.
Separately, I think validateUserInput may be exposing too wide of an API. Currently, it can be used to validate an array property or validate a Firestore value as a whole. The ambiguity really comes from this. This also lead to incorrect usage, e.g., in
nodejs-firestore/dev/src/field-value.ts
Line 571 in ce9abfd
I don't have a strong opinion on this though. This change is technically relaxing the requirement on the client side. If anything, we always have the backend side as fallback so the change as is may be fine.
There was a problem hiding this comment.
Thanks for the feedback.
Regarding L348 being problematic. If a path of ['foo', 'bar'] is considered to have a depth of 1 and ['foo'] is considered to have a depth of 0, then yes we could remove the top level map property or we could compute the depth as path.size - 1. It's the same IMO and allows the re-use of path.
I agree with your statement "I think validateUserInput may be exposing too wide of an API", but in consideration of our other discussion of removing all of this validation. I'm going to avoid any refactor for now.
3 participants
BEGIN_COMMIT_OVERRIDE
fix: Update the depth validation used when writing documents, so that it matches the validation of the Firestore backend.
END_COMMIT_OVERRIDE