grahamearley / FirestoreGoogleAppsScript

A Google Apps Script library for accessing Google Cloud Firestore.
http://grahamearley.website/blog/2017/10/18/firestore-in-google-apps-script.html
MIT License
648 stars 109 forks source link

Remove specific fields using `updateDocuments` - feature request/question #141

Closed YuvAIR closed 2 years ago

YuvAIR commented 2 years ago

In the regular Firestore JS API, you can do:

docRef.update({
    "field.subfield": firebase.firestore.FieldValue.delete()
});

to remove a specific field from a document.
Is there any way of doing that with this library as well? Or do I have to set the whole document to everything but that specific field (which of course costs more time as I have to do another API read call).

Other Comments

I also tried to update the field to null/undefined, but that does not work, it simply sets that field to null.

Library Version: 33
YuvAIR commented 2 years ago

Ok, so after some research I found out that if you include some field in the mask, but not in the actual body of an updateDocument request, it deletes that field. I will work on a pull request to allow you to do just that.

LaughDonor commented 2 years ago

Right, you can use updateDocument without the mask parameter and just not include that field.

This library internally uses the Cloud Firestore API which does not have a direct method to delete a specific field. If your pull request creates a function that does this in multiple operations (get all fields, and then removing the 1 unwanted field, I assume) and add a test case for it; I'll be happy to merge that in!

YuvAIR commented 2 years ago

It actually does have a direct method to delete a specific field, take a look at my #143, only one API call and you can delete whichever fields you want to.

LaughDonor commented 2 years ago

How is this different from updating the object to remove the desired field(s) and calling updateDocument(obj, false)?

Yuvix25 commented 2 years ago

Because this way you dont need to get all the other values, you just do, for example:

firestore.updateDocument("path", {}, ["fieldToRemove"]);

and it would delete "fieldToRemove", while keeping everything else the same, just as the screenshot from the documentation defines.

Edit: Oops, wrong account😅

YuvAIR commented 2 years ago

Can you take a look at my pull request then? It's a really small fix which adds a pretty important feature.

LaughDonor commented 2 years ago

Because this way you dont need to get all the other values, you just do, for example:

firestore.updateDocument("path", {}, ["fieldToRemove"]);

and it would delete "fieldToRemove", while keeping everything else the same, just as the screenshot from the documentation defines.

I like this idea, but I don't think this feature should be meshed with updateDocument as it could lead to more confusion on its usage. It can probably work like this under the hood, but we can create a new exposed function to clearly handle this. Maybe something along the lines of removeDocumentFields("path", ["list", "of", "fields", "to", remove"]), thoughts?

YuvAIR commented 2 years ago

Hmmm I don't think this would be ideal, as sometimes you want to both update and delete fields at the same time. However, I do have another idea which would probably be better, and I have also implemented it within my app because much easier to use. So the idea is like this, under the hood, it would work as in my PR, and then if you want to both update and remove fields, you can do for example:

firestore.updateDocument("path", {
    field1: "new value",
    field2: FieldValue.delete(), // FieldValue acts like an enum, and delete() gives you a FieldValue object of type delete (and FieldValue could also be used in the future for more features
}, true); // this would under the hood do: firestore.updateDocument("path", {field1: "new value"}, ["field2"]);

And then the library would detect that FieldValue.delete(), and apply the mask appropriately.
So basically the idea is to just copy the way it works in the official JS Firebase Firestore library.
What do you think?

(PS: I could also add my code for this feature to the PR)

LaughDonor commented 2 years ago

If you need to update and delete fields are the same time, you can do this already (with your example above):

const obj = {"field1": "some value", "field2": "another value"};  // Exists in database
obj.field1 = "new value";
delete obj.field2;
firestore.updateDocument("path", obj);

Keeps the process explicit and simple by reducing the burden of responsibility on the library.

Yuvix25 commented 2 years ago

No, that's not what I mean, I meant that field1, field2 are both already fields in the database, and the above command which I wrote (with FieldValue) would update one and delete the other. What you said (the one with JS's delete) would only update one, it would not delete the other from the database.

LaughDonor commented 2 years ago

No, that's not what I mean, I meant that field1, field2 are both already fields in the database, and the above command which I wrote (with FieldValue) would update one and delete the other. What you said (the one with JS's delete) would only update one, it would not delete the other from the database.

I'm sorry, but your logic is incorrect. I have a passing Test Case that covers this: https://github.com/grahamearley/FirestoreGoogleAppsScript/blob/main/Tests.ts#L144-L155

Please keep in mind that the 3rd parameter, mask, needs to be falsy for this to work in "overwrite mode" as presented in the README: https://github.com/grahamearley/FirestoreGoogleAppsScript#updating-documents

To update (overwrite) the document at this location, we can use the updateDocument function:

firestore.updateDocument("FirstCollection/FirstDocument", data);

Yuvix25 commented 2 years ago

No, that is still not what I meant, in your example here you show how you can delete a field, by simply setting all the other fields except this one. This would require a second read call to the database in order to retrieve all of the other fields. My PR allows you to simply delete a single field, without retrieving the rest of them. I am 100% sure it does that, and am already using it as part of my project. I would very much appreciate it if you could take a look at my PR, or I can also add the feature (FieldValue) which I mentioned in my previous comment.

YuvAIR commented 2 years ago

Ok, so say you create a document as follows:

const original = {
  field1: "value 1",
  field2: "value 2",
  field3: "value 3,
};
firestore.createDocument(path, original);

And then, you want to remove field1 from that document. As for now, you would need to perform two API calls, like so:

const data = firestore.getDocument(path).obj;
delete data.field1;
firestore.updateDocument(path, data, false);

And having to do 2 calls like this is obviously far from ideal in terms of performance. (Of course you already know how the document looks like here, so you don't really need the first getDocument call in this particular example, but in most cases you do not know the value of that document, so you do need that API call) What my PR allows you to do instead is this:

firstore.updateDocument(path, {}, ["field1"]);

As according to Google's documentation:

Fields referenced in the mask, but not present in the input document, are deleted from the document on the server.

And therefore it will remove field1, and keep everything else the same. You then suggested to add a different method to do so, removeDocumentFields. This will indeed work, but if for example I want to edit one field, and remove the other, it would still require 2 API calls, one for updateDocument, and one for removeDocumentFields, but with my solution you could easily do:

firestore.updateDocument(path, {field2: "new value 2"}, ["field1", "field2"]);

I do however agree that this is not the most elegant way to do so, but Firestore's official API has a very nice way of doing that. They have defined an enum-like object, FieldValue, which has a method delete(), that returns a FieldValue object of type delete. If you set that value as a value of some field in an updateDocument call, it would remove that specific field (using the mask). So to remove field1 and edit field2, you would do:

firestore.updateDocument(path, {
  field1: FieldValue.delete(),
  field2: "new value 2",
}, true);

which would be translated to:

firestore.updateDocument(path, {field2: "new value 2"}, ["field1", "field2"]);

I plan on adding the FieldValue object to my PR (as I have already implemented it internally in one of my projects), although I first want to consult with you on how would it be best to implement that.

I hope I am now clear, and sorry for previously not providing a full example to best explain myself.


Many Thanks Yuval

LaughDonor commented 2 years ago

I have looked at your PR from the start, just wanted to consolidate discussion here first.

Can you link me to the Firestore documentation you're referring to using FieldValue.delete()?

Yuvix25 commented 2 years ago

FieldValue
As you can see here, it has a lot of values other than delete. So maybe in the future I could add some of them as well.
Also, for delete this is not necessary, but for the other FieldValues you need to use FieldTransform in the REST API.

YuvAIR commented 2 years ago

Should I then add this to the pull request as well?

LaughDonor commented 2 years ago

The documentation link you provided is to an Android SDK based on Java. It has no bearing on how this library interfaces with the Cloud Firestore API. This library is based on the REST API of Firestore and all possible capabilities are within that section only. What could you add?

Yuvix25 commented 2 years ago

Oops, you are correct. FileValue however does exist in the web API, and is implemented in the REST API using FieldTransform. It is however not necessary for the delete method, this one I can implement with only minor tweaks to my current PR, I was currently only asking about whether should I add FieldValue.delete() (only delete for now), or not.

LaughDonor commented 2 years ago

It's been a long time since I've looked at that documentation, but FieldTransform is new, and same with some of the other options on that page. I would definitely like to expand this library to bring those features to light, thanks!

You can add firestore.removeDocumentFields("path", ["list", "of", "fields", "to", remove"]) to extend the library to delete a field without fetching it first as the update-to-delete field functionality already exists.

Yuvix25 commented 2 years ago

Again, removeDocumentFields is nice, but sometimes I need to both update fields and remove fields at the same time. Can I maybe add a advancedDocumentUpdate function, which would act like how updateDocument acts in my PR? (So that it accepts an actual mask array and not just a boolean)

LaughDonor commented 2 years ago

As I mentioned before and again, updating and removing fields at the same time is already supported by the library natively. Just update the object you send.

The only new functionality that doesn't exist is deleting a field without having the data in hand. But even this is not possible with the REST API. You would need to have the object data available in order to even delete a field. If you think I've overlooked the API, please reference a REST API function that can accomplish this. Otherwise if we create this function, it would have to internally get the document, delete the field, and then update the Firestore document, which would be nothing more than a pure shortcut, and outside the scope of this library for now.

Yuvix25 commented 2 years ago

I'm sorry, but I don't agree with you. This functionality is not available, because if for example I have 3 fields, and I want to update one, delete another, and keep the third one the same, without knowing it's value, is not possible. I would have to read the entire document, remove one field, update another, and keep the third one the same, and then update the entire document, just as I explained here.


If you think it is currently possible, I would love you to show me how to solve this exact problem, as I really need it:

There's a given document with 3 fields, field1, field2, and field3, you don't know the value of any of them. Without reading any information from the database, I need to remove field1, set field2's value to "some value", and keep field3 as it is - all in a single API call. How would I do that?

LaughDonor commented 2 years ago

Ok, I apologize, since I guess I didn't process the documentation properly with regards to the following highlighted statement: image

Alright, please correct me if this scenario isn't what you are describing:

// Setup
const props = PropertiesService.getScriptProperties()
const [email, key, projectId] = [props.getProperty('client_email'), props.getProperty('private_key'), props.getProperty('project_id')];
const firestore = FirestoreApp.getFirestore(email, key, projectId);

const path = "MyCollection/MyDoc";
firestore.createDocument(path, {field1: "field1", field2: "field2", field3: "field3");

firestore.updateDocument(path, {field2: "some value"}, {field1: null, field2: null});

According to how this library works, the last statement should update the document with field1 removed, and field2 updated. And that's the PR you suggested, right? I believe the field values in the mask doesn't matter, so a list should suffice here.

Yuvix25 commented 2 years ago

Yes, that is indeed what my PR does, and also the mask is a list, just forgot to update that it is a list in one JSDoc, I would update it right now.

Yuvix25 commented 2 years ago

Could you re-open my PR so that I would be able to add new commits to it?

LaughDonor commented 2 years ago

Closed with #143