thisbejim / Pyrebase

A simple python wrapper for the Firebase API.
2.06k stars 527 forks source link

Suggestion: don't modify parent when chaining db methods #167

Open jlowin opened 7 years ago

jlowin commented 7 years ago

Make sure these boxes are checked before submitting your issue:

[x] Check that your version of Python is 3.4+ [x] Check that you are on the newest version of Pyrebase [x] Check that Email/password provider is enabled in your Firebase dashboard under Auth -> Sign In Method.

This is just a suggestion, but as a new user I found it surprising that calling methods like child() actually modified the parent rather than returning a new object. This seems "unpythonic"; typically chained methods that return objects would leave the parent untouched.

For example:

db = firebase.database()
ref = db.child('key1').child('key2')
print(ref.path) # key1/key2, as expected
print(db.path) # key1/key2 -- unexpected! 

The reason this is surprising is that if I assume the parent is unmodified, I might try to reuse it:

ref2 = db.child('key3').child('key4')
print(db.path) # key1/key2/key3/key4 -- unexpected!
print(ref.path) # key1/key2/key3/key4 -- unexpected!
print(ref2.path) # key1/key2/key3/key4 -- unexpected!

Complicating matters further, is seems that calling set() or get() clears the path. So there is a hidden state being managed behind the scenes.

My suggestion is to leave the API just as it is, but instead of setting path on self when calling child(), create a copy of the database service, set the path on that copy, and return the copy.

ashemah commented 7 years ago

I keep getting bitten by this behaviour.

jlowin commented 7 years ago

Here is a worst-case scenario. With an admin service account (or poorly set permissions), this LOOKS like I'm just erasing two child nodes, but I'm actually erasing the entire database****

db = firebase.database()

ref1 = db.child(some_path)
ref2 = db.child(some_other_path)

ref1.set({})
ref2.set({})
naure commented 7 years ago

This is indeed broken at the moment. Coming from the official javascript library and lured by the identical method names, you're almost guaranteed to erase the entire database or get other surprising behaviors depending on the order of method calls.

The self.path of Database/Reference objects should never change.

steliosrammos commented 4 years ago

This is quite frustrating, is any work being done to fix it?