firebase / firebase-unity-sdk

The Firebase SDK for Unity
http://firebase.google.com
Apache License 2.0
226 stars 37 forks source link

Database: Please add Push function into MutableData #275

Open Thaina opened 6 years ago

Thaina commented 6 years ago

When I use RunTransaction function I would like to add child node like I could normally push. For simplicity the MutableData should also have Push that would generate it's key in transaction

Is it possible?

stewartmiles commented 5 years ago

@Thaina this bug was opened a while ago. Do you have an example of the API change you would like to see?

Thaina commented 5 years ago

I don't know is it possible now but I want something like this

reference.RunTransaction((mutableData) => {
    var child = mutableData.Push(); // generated new unique key, so if there was a collision it will retry
    child.Value = SomeValue;
});
chkuang-g commented 5 years ago

Hi Thaina,

The unique key is actually calculated on the client side. https://firebase.googleblog.com/2015/02/the-2120-ways-to-ensure-unique_68.html

While it does not guarantee no collision, it is very unlikely to happen (timestamp + randomness). Also, it does not depends on the location.

MutableData does not contain the path. It is just a container of the data at the given location for you to change. If you really like to get a unique key, you can do the following:

reference.RunTransaction((mutableData) => {
    var uniqueKey = reference.Push().Key;
    var child = mutableData.Child(uniqueKey);
    child.Value = SomeValue;
});

Note that if what you want to do is to add a child with unique key, you actually do not need RunTransaction(). Since it is very very very unlikely to collide, you can just use reference.Push().SetValue(SomeValue);

This is why Push() exists and it helps the performance as well!

Hope this helps!

Shawn

chkuang-g commented 5 years ago

BTW, DatabaseReference.Push() does not regenerate a new unique key when collide. ;)

Thaina commented 5 years ago

@chkuang-g But the RunTransaction should repeat when it collide and so doesn't it generate new key until it stop colliding?

I just put simple sample code but in the actual scenario I would do many logic that need transaction, and need a unique key to be generated and kept onto other record to link it. that's why I request this feature

Please reopen it

chkuang-g commented 5 years ago

@Thaina

RunTransaction() does merge or override. It does not prevent child key collision. For instance, if there are two clients trying to write to the same location using the following code:

reference.RunTransaction((mutableData) => {
    var child = mutableData.Child("SameUniqueKey");
    child.Value = SomeValue;
    return TransactionResult.Success(mutableData);
});

The client who runs the transaction later is going to override the value at the same location, which is "SameUniqueKey" in this case.

If you really want to prevent the collision, here is what you can do:

reference.RunTransaction((mutableData) => {
    var uniqueKey = reference.Push().Key;
    // Reroll until there is no collision
    while (mutableData.HasChild(uniqueKey)) uniqueKey = reference.Push().Key;
    var child = mutableData.Child(uniqueKey);
    child.Value = SomeValue;
    return TransactionResult.Success(mutableData);
});

What RunTransaction() can do is to make sure mutableData is the same to what is in the database. If it is not, the server would reject the write request and the function would be rerun again, and in the new round, you would get another latest mutableData.

Using this code, you can make sure that you always write to a unique location when you do RunTransaction().

And you still do not need Push() in MutableData. It is not supported in any other SDK and it does not make sense to support it in Unity SDK while MutableData just a data container.

Does that make sense?

Thaina commented 5 years ago

@chkuang-g

What RunTransaction() can do is to make sure mutableData is the same to what is in the database. If it is not, the server would reject the write request and the function would be rerun again, and in the new round, you would get another latest mutableData.

And so if the key was generated by push but got collision, it should be considered that the mutableData is not the same and just rerun the transaction. And so it would call push again, it should kept repeating until it generate a key that not exist, isn't it?

chkuang-g commented 5 years ago

@Thaina It is this part that would try to find a child key that does not exist yet. And it all happens on the client side.

    var uniqueKey = reference.Push().Key;
    // Reroll until there is no collision
    while (mutableData.HasChild(uniqueKey)) uniqueKey = reference.Push().Key;

What I meant by rerunning the transaction is: when the client try to send write request to the server through RunTransaction, it sends both

  1. What it think the value was before
  2. What the value it wants to change to

When the server got this request, it is possible that the value this client think was before has already been changed because other client is writing at the same location as well. In this case, the server reject the write request and ask the client to rerun the transaction with the latest data again.

That is, on the client side, it would rerun the transaction until mutableData represent the latest data on the server side. Therefore you can check if the child key exists when you are running the transaction.

Note that if you have many children in this location and each child contains deep nested data, this round trip can take long and demand bandwidth. This is why we recommend to just use Push() and SetValue() for efficiency. But if integrity is what you need, then you need to wait until no other client is trying to write to this location.

Firebase SDK and backend does not prevent key collision. We only provide an SDK which generate a unique key which is very unlikely to collide.

Thaina commented 5 years ago

@chkuang-g You don't understand that I understand it

What I meant by rerunning the transaction is: when the client try to send write request to the server through RunTransaction, it sends both

  1. What it think the value was before
  2. What the value it wants to change to

Yes, and the point is, if we use push to create key. The transaction should send What it think the value was before as null. Because we just push a new key into it, it must be null. The transaction should be something like this

{ "newUniqueKey" : { "before" : null,"after" : newValue } }

And so if this is possible. It will detect collision, even if we don't check and loop with the while (mutableData.HasChild(uniqueKey)). And it could also detect the collision from 2 client trying to push a new key but unfortunately generate a collided key. There would be one client need to repeat transaction and it would generate the new key. Even if it would be astronomically low chance it could happen anyway

That's the reason why I try to propose this function to exist

chkuang-g commented 5 years ago

@Thaina You are correct that if both client is trying to push { "newUniqueKey" : { "before" : null,"after" : newValue } }, the later one would be rejected and need to rerun the transaction again.

It is almost impossible to generate the same unique key using Push() in the second round-trip, assuming the timestamp calculated on all client is correct. If you want to 100% certain about 0 collision, you can always to check if the value at "newUniqueKey" is empty or not.

I can see your point to add Push() to MutableData. I can forward your opinion to the team. Since this API does not exists in iOS/Android SDK, we cannot simply add this to Unity SDK. https://firebase.google.com/docs/reference/android/com/google/firebase/database/MutableData

At the meantime, you can always use Push() from the captured DatabaseReference.

Hope this helps! Shawn

Thaina commented 5 years ago

@chkuang-g Thank you very much

I don't require that it must be implemented like I have state above. But the MutableData in transation should have some api that could do this same functionality I wish more easily

Thaina commented 2 months ago

Additionally to push, I think we should also have remove in mutableData object