Firestore - Inconsistent behavior in key addressing

[Pirayya]

OS Type: Its Java - version 11
Firestore Java SDK version (1.33.0, but I can see that the problem still exists in the code on master)

Using field keys with dots (.), i.e. my.key: "My value" is allowed, albeit a bit frowned upon, has inconsistent behavior when creating compared to deleting. It is perfectly fine to create a key with dots but cannot be deleted (unless it exists on the top level of the document).

{
   "my.key": true
}

can be removed, but:

{
   "my": {
      "specialized.key": true
   }
}

cannot.

That's not all! Using the JavaScript SDK for Firestore, deleting is not a problem.

It is always debatable whether or not to use dots in keys, but I think the inconsistency is something that has to be fixed.

Steps to reproduce

1. Create a document like the one above (with specialized.key)
2. Run one of the code blocks below

var updates = Map.of("my.specialized.key", FieldValue.delete());
firestore.collection("myCollection").document("myDocument").update(updates)

or

var path = new FieldPath("my", "specialized.key");
firestore.collection("myCollection").document("myDocument").update(path, FieldValue.delete()) // <-- Very similar to JavaScript SDK

The first code block will succeed without any change, but the second will result in a failed Precondition.

Code example

See steps to reproduce

Stack trace

Will fail right here:

java-firestore/google-cloud-firestore/src/main/java/com/google/cloud/firestore/UserDataConverter.java

Line 101 in 80087a3

options.allowDelete(path), "FieldValue.delete() is not supported at field '%s'.", path);

If you don't have time to fix this issue(? don't know if you regard this as an issue), I can definitely contribute!

Tell me if anything is unclear!

[Pirayya]

@suraj-qlogic, you have the same issue? Or what is happening?

[suraj-qlogic]

@Pirayya. Thanks for filling an issue.I am currently investigating on it if found any inconsistent behaviour, then i will open PR soon

[Pirayya]

@suraj-qlogic, I am saying that there is inconsistent behavior in addressing keys, although it might've been implemented like that deliberately. Do you have an idea of what the 'correct' behavior is? I would like to see someone with authority to make a decision here, rather than just making a PR fixing it one way or the other.

[suraj-qlogic]

@suraj-qlogic, I am saying that there is inconsistent behavior in addressing keys, although it might've been implemented like that as deliberately. Do you have an idea of what the 'correct' behavior is? I would like to see someone with authority to make a decision here, rather than just making a PR fixing it one way or the other.

@BenWhitehead @schmidt-sebastian

[schmidt-sebastian]

First, I want to point out that the code snippets above to very different things:

var updates = Map.of("my.specialized.key", FieldValue.delete());
firestore.collection("myCollection").document("myDocument").update(updates)

This deletes the field { 'my.specialized.key' : ... }

var path = new FieldPath("my", "specialized.key");
firestore.collection("myCollection").document("myDocument").update(path, FieldValue.delete()) // <-- Very similar to JavaScript SDK

This deletes the field { 'my' { 'specialized.key' : ... }}

That being said, the second code snippet should be supported and should not be rejected.

@suraj-qlogic Is this something you can fix?

[Pirayya]

@schmidt-sebastian, I just tested creating a JSON structure like:

{
   "users": {
      "Pirayya": {
         "color": "blue"
      }
}

And ran the following code (Kotlin)

            val key = "users.Pirayya.color"
            val updates = mapOf(key to FieldValue.delete())
            firestore.collection(collection).document(accountKey).update(updates)

And the "color" field inside the object "Pirayya" was removed.

So if the expected behavior is to remove the key { 'users.Pirayya.color': { ... } }, then that doesn't work...

I also think the issue regarding the second code example I showed in the ticket can be fixed by figuring out whether or not to use append() here:

java-firestore/google-cloud-firestore/src/main/java/com/google/cloud/firestore/UserDataConverter.java

Line 163 in 80087a3

Value encodedValue = encodeValue(path.append(entry.getKey()), entry.getValue(), options);

I believe each segment in FieldPath should not be split here. If a string is supplied like users.Pirayya.color I would agree that splitting it would be a solution, but not if you use FieldPath.of() as input.

java-firestore/google-cloud-firestore/src/main/java/com/google/cloud/firestore/BasePath.java

Line 61 in 80087a3

components.add(splitChildPath(path));

[schmidt-sebastian]

As you stated, string keys are expanded (split on dots):
'a.b.c' => { a : { b : { c: {..}}}

Field keys created with FieldPath.of() are not:
'FieldPath.of('a', 'b.c') => { a : { 'b.c' {..}}

[Pirayya]

Ok, sorry, I might've not made myself clear. As a user of the Java Firestore SDK I expect FieldPath.of('a.b', 'c.d') to point to the key 'c.d' inside object 'a.b' when trying to mutate the object:

{
  "a.b": {
     "c.d": {
        ... 
     }
}

Somewhere along the way, all the keys except the first one is changed. So right before the update is made the FieldPath transforms and points to 'd' inside object 'c' inside object 'a.b':

{
   "a.b": {
      "c": {
         "d": {
            ...
         }
      }
   }
}

[schmidt-sebastian]

Your understanding is correct. This looks like a bug.

[BenWhitehead]

@suraj-qlogic Any chance you can take a look at a fix for this?

[suraj-qlogic]

@BenWhitehead ,Yes it's in progress.i have been investigated and found this is happen due to this code.I will open PR as soon as possible.

[BenWhitehead]

Sounds great, thanks @suraj-qlogic!

[Pirayya]

@suraj-qlogic @BenWhitehead, sorry, accidentally opened a PR here, closed it immediately though. But I have a suggestion for a fix: https://github.com/Pirayya/java-firestore/pull/1/commits/167443c88e1d33a02ce93882559d368b03ece5e9

What do you think?

Edit: I haven't tested this approach, it is only somewhat of a suggestion (mostly implemented in a way to not break anything)

Edit2: I've tried two scenarios:

firestore.collection("my-collection")
    .document("my-document")
    .update(FieldPath.of("user", "properties", "meta", "favorite.color"), "blue");

This will update the field: user->properties->meta->favorite.color to 'blue'. But the second scenario (deleting):

firestore.collection("my-collection")
    .document("my-document")
    .update(FieldPath.of("user", "properties", "meta", "favorite.color"), FieldValue.delete());

Fails because of the EncodingOptions created here:

java-firestore/google-cloud-firestore/src/main/java/com/google/cloud/firestore/UpdateBuilder.java

Line 502 in b91c57c

new EncodingOptions() {

The fields object contains one key with a FieldPath pointing to: users->properties->meta->favorite.color but before we call encodeValue we split up the fields object to a map (still pointing correctly to favorite.color) and inside encodeValue we traverse the map and append each key to a new FieldPath (I don't know why this is done) but this time (as @suraj-qlogic pointed out) we split each key on dot. This will generate another path then the original one and therefore the Precondition will fail here:

java-firestore/google-cloud-firestore/src/main/java/com/google/cloud/firestore/UserDataConverter.java

Line 100 in b91c57c

Preconditions.checkArgument(

(but only for deletes since that is the only time we run this Precondition).

So the AppendOptions might fix this issue but might introduce new issues 😨

[schmidt-sebastian]

@suraj-qlogic I will look at this in a bit (in a meeting now).

[schmidt-sebastian]

So the problem is in UserDataConverter in line 164. To process the nested updated, we call path.append with a Map key, which is a String. In the user's case, it is "specialized.key". append splits on dots for all input strings, which we don't want to do here since the field keys in an update Map are all individual field paths.

The "ugly" fix is:

diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/UserDataConverter.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/UserDataConverter.java
index 2b08a12..844f097 100644
--- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/UserDataConverter.java
+++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/UserDataConverter.java
@@ -18,6 +18,7 @@ package com.google.cloud.firestore;
 
 import com.google.cloud.Timestamp;
 import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
 import com.google.firestore.v1.ArrayValue;
 import com.google.firestore.v1.MapValue;
 import com.google.firestore.v1.Value;
@@ -160,7 +161,7 @@ class UserDataConverter {
       Map<String, Object> map = (Map<String, Object>) sanitizedObject;
 
       for (Map.Entry<String, Object> entry : map.entrySet()) {
-        Value encodedValue = encodeValue(path.append(entry.getKey()), entry.getValue(), options);
+        Value encodedValue = encodeValue(path.append(new AutoValue_FieldPath(ImmutableList.of(entry.getKey()))), entry.getValue(), options);
         if (encodedValue != null) {
           res.putFields(entry.getKey(), encodedValue);
         }

We should find a way to make this pretty.

[Pirayya]

@schmidt-sebastian, the fix I made in the forked project has another - well, maybe not so beautiful - fix but it is another suggestion. But as I've stated before, I know too little of the intricacies of this project to know all the paths in to encodeValue so I don't know if my proposed fix would be useful. Although if you thought it to be good, instead of me spending a lot of time signing the CLA, getting to know what tests to fix/add etc. maybe someone else can copy it and create a real PR? :P (Sorry for being lazy)

[schmidt-sebastian]

@Pirayya Sorry, I missed the link to the fork in the earlier comment. Yours is a much cleaner version of what I have. @suraj-qlogic Can we take https://github.com/Pirayya/java-firestore/commit/167443c88e1d33a02ce93882559d368b03ece5e9 and add a test?

[Pirayya]

@schmidt-sebastian, no problem! But all credit should go to @dmitry-fa, he came with a better suggestion than my first one! 👍

[suraj-qlogic]

@schmidt-sebastian ,Ok i will do that.

[BenWhitehead]

@schmidt-sebastian Is there anything else we need to address this issue other than what was merged in #295? If not I'll plan on performing a release with this fix early next week.

[schmidt-sebastian]

No - we are good to go.

[Pirayya]

Nice, thank you all for helping me solve this so quickly! :) @BenWhitehead, @schmidt-sebastian, @suraj-qlogic and @dmitry-fa!