Cloud Function: Deleting from both Firestorage and Firestore in a kind of "batch"

Question

I've written code that deletes a "post" the user has sent with my Android app. A "post" is made of an image (stored in Firestorage) named A, of a Firestore document named B which is the user UID (this document contains a field that is the sum of all the posts' number of likes), and of another Firestore document named C which is the post that is going to be deleted.

So, if a user deleted a post:

1. A must be deleted
2. AND B must be updated (by decreasing the number of likes of the user by the number of likes of the post that is going to be deleted)
3. AND C must be deleted

If at least one of these changes can't complete, nothing must be deleted or updated and an error or an exception must be thrown to the Android app.

Problem: you would think it'd be very simple; I would have to simply use a batch write, put these 3 operations in it and then commit the batch. However... A is a Firestorage document, not a Firestore document, while B and C are Firestore documents. Thus, it's simply technically impossible to do that.

Question

I would want to know if the workaround I've found is correct or not, and if there is a way to correct it or improve it.

I would also want to know if the exceptions mechanism I use is correct (these exceptions are thrown if the client doesn't send values that are expected by the Cloud Functions script, or if the post could not be deleted): will my Android app be able to handle these exceptions and is it the way to do it? In other words: should I throw exceptions or return values likes -1, -2 (i.e.: error codes) for example?

The code

exports.deletePost = functions.https.onCall((data, context) => {

    if(!context.auth) {
        throw new functions.https.HttpsError('failed-precondition', 'The function must be called while authenticated.');
    }

    if(data.the_post === null || data.the_post === '') {
        throw new functions.https.HttpsError('invalid-argument', 'The function must be called with an adequat post.');
    }

    if(data.stored_image_name === null || data.stored_image_name === '') {
        throw new functions.https.HttpsError('invalid-argument', 'The function must be called with an adequat post.');
    }

    const the_post = data.the_post;
    const deleteDbEntry = admin_firestore.collection('list_of_slots').doc(the_post);
    const promise = deleteDbEntry.get().then(function(doc) {

        const stored_image_name = data.stored_image_name;
        const filePath = context.auth.uid + '/posts/' + stored_image_name;

        const batch = admin.firestore().batch();
        batch.delete(deleteDbEntry);
        if(doc.data().number_of_likes > 0) {
            const updateUserLikes = admin_firestore.collection("users").doc(context.auth.uid);
            batch.update(updateUserLikes, "likes", FieldValue.increment(-doc.data().number_of_likes));
        }
        const batch_commit = batch.commit();
        const deleteFile = storage.bucket('andr0d-XYZ-fXXXX.appspot.com').file(filePath).delete();

        return Promise.all([deleteFile, batch_commit]).then(function() {
            return 1;
        }).catch(function(error) {
            console.log(error);
            throw new functions.https.HttpsError('unknown', 'Unable to delete the post. (2)');
        });

    }).catch(function(error) { 
        console.log(error);
        throw new functions.https.HttpsError('unknown', 'Unable to delete the post. (1)');
    });

    return promise;
});

Answer 1

I must admit that I haven't really used firebase or done any android development and thus cannot answer your questions about the correctness of the code. I did wonder if using a transaction would help satisfy the need to prevent deletion of items if other items cannot be deleted, but maybe the batch handles that... Maybe the process to delete the file (i.e. deleteFile) should be added to batch, although maybe that not possible because it is the firestorage document.

Good things

The code makes good use of const for values that don't get re-assigned.

Indentation is consistent and helps readability of the code.

Functions aren't overly long.

There aren't many indentation levels because the first function will exit early in an error state.

Suggestions

I don't see any comments. The code does seem mostly explanatory as it is but it would be helpful to have some insights about the code - at least function parameter types, return types, etc. Over time it might become difficult to remember why you may have done something a certain way.

Many variables are declared and then only used once. There is little point in doing this. Many of those variables can be eliminated - e.g. the_post, promise, stored_image_name, filePath, updateUserLikes.

The variable name deleteDbEntry could be confusing to other readers of your code. A more appropriate name might be dbEntryToDelete.

The lambda function passed to .then() of Promise.all() could be simplified to an arrow function - e.g.

return Promise.all([deleteFile, batch_commit]).then(_ => 1)