googleapis / google-cloud-java

Google Cloud Client Library for Java
https://cloud.google.com/java/docs/reference
Apache License 2.0
1.9k stars 1.07k forks source link

google-cloud-firestore - DocumentReference.update() sends entire document. Conflict with rules. #4341

Closed matanshukry closed 5 years ago

matanshukry commented 5 years ago

Thanks for stopping by to let us know something could be better!

Please include as much information as possible:

Environment details

Steps to reproduce

  1. Create a collection in Firestore named "users".

  2. Create a document named "doc1" under collection "users".

  3. Add "displayName" field and at least 1 more field. example document:

    {
    "displayName": "Msh",
    "ewq": "ad",
    "fdsa": "asdf"
    }
  4. Add the following Firestore rule: match /users/{userId} { allow read: if true; allow update: if request.resource.data.keys().size() == 1; }

  5. run the following code snippet (kotlin):

        val docSnapshot = mFirestore.collection("users").document("doc1").get()
        docSnapshot.addOnSuccessListener { snapshot ->
            val displayName = snapshot.data!!["displayName"] as String
            val newDisplayName = displayName + "_b"
            val map = mutableMapOf("displayName" to (newDisplayName as Any))
            mFirestore.collection("users").document("doc1").update(
                    map
            ).addOnCompleteListener { task ->
                val isSuccess = task.isSuccessful
                val isComplete = task.isComplete
                val ex = task.exception
            }
        }
  6. Put a breakpoint inside the addOnCompleteListener, and notice isSuccessful is false (also, value doesn't change).

  7. Change the firestore rule from size() == 1 to size() === 3. 3 is the number of fields in the document, regardless the amount of fields we send to .update(). In my case it was 3

  8. Run the code above

  9. Inspect the isSuccessful value again. It is true now. Also, value in Firestore is changed.

Stacktrace

Not relevant

Code snippet

Added above

External references such as API reference guides used

Any additional information below

I have to admit I tried to look in the Github code and it does seems the code is only sending the provided fields and not the entire document. It is possible there is a problem inside Firestore; However, the simulator inside Firestore is working, and I have no other way to debug it..

p.s. In my example, I checked the size() of the incoming keys, but that was only to debug the problem. My actual use case is to verify only a single field is being updated, e.g. request.resource.data.keys().hasOnly(["displayName"]);

ajaaym commented 5 years ago

@matanshukry client only sends updated field but request.resource is the resource after update. Please see here

matanshukry commented 5 years ago

@ajaaym - How can you check only the request then? The idea is to verify that an update is only allowed on some specific fields.

ajaaym commented 5 years ago

@matanshukry sorry looks like link has wrong information. Below rule works, can you please try? basically data always have 2 extra fields in addition to your data (name & probably id). So for size condition you need to add 2 + your required number of fields or you can check for the keys that you dont want to allow for updates.

allow update: if !request.resource.data.keys().hasAny(['ewq','fdsa']);
matanshukry commented 5 years ago

@ajaaym - that's incorrect as far as I could see. data always had the same number of keys as the amount of fields in the resource. My updates do not add or remove fields, so I can't say if it was the amount of fields before or update the update (same amount).

ajaaym commented 5 years ago

@matanshukry after digging more in to this. Below is working for me. can you try below?

          // list of fields that you dont want to change.
allow update: if request.resource.data.field1 == resource.data.field1
        && request.resource.data.field2 == resource.data.field2
       // to make sure no new field is added.
        && request.resource.data.size() == resource.data.size();
matanshukry commented 5 years ago

@ajaaym - I knew this solution is a possibility, but it's pretty much checking all fields that you don't care about, making sure they have the same value as before. Also, it's much more complex when your fields aren't simple values. I have arrays and objects as fields. How do I check them too?

ajaaym commented 5 years ago

@matanshukry I just tested with array, maps and geopoint, it works fine.

matanshukry commented 5 years ago

well I can't verify that right now, but I'll take your word for it. I think we can close this then.

However, although possible, I do think this is kind of a 'hack'. That is, I can't really check if specific fields are being updated. I can only check ALL the fields I do NOT care about, are the same before and after the request.

I guess it should be a feature request though. Would you like me to open a separate feature request issue for that? is this even the right repository to do that?

ajaaym commented 5 years ago

@matanshukry thats the change in service, best way is to create support ticket and follow up through that.

MahmoudChik commented 2 years ago

Hi all, How can we test the complete task to synchronise the process ?

Because my bellow solution does not work ! :/

Code : ..... Task result = detector.processImage(image);

    try {
        result.addOnSuccessListener(new OnSuccessListener<FirebaseVisionText>() {
                    @Override
                    public void onSuccess(FirebaseVisionText firebaseVisionText) {

                        //Toast.makeText(MainActivity.this, "TranslateTextML", Toast.LENGTH_SHORT).show();

                        for (FirebaseVisionText.TextBlock block : firebaseVisionText.getTextBlocks()) {
                            for (FirebaseVisionText.Line line : block.getLines()) {
                                String lineText = line.getText();
                                builder.append(lineText + "\n");
                            }
                        }

                    }
                })
                .addOnFailureListener(
                        new OnFailureListener() {
                            @Override
                            public void onFailure(@NonNull Exception e) {
                                Toast.makeText(MainActivity.this, " not translate ", Toast.LENGTH_SHORT).show();

                            }
                        }).isComplete();

    } catch (Exception e) {
        e.printStackTrace();
    }

while(!result.isComplete()) {

        try {
            Thread.sleep(100);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }

    }

......