Remove unnecessary checks on update with etag #63

andyfangdz · 2017-08-20T07:48:32Z

This PR fixes #62 by removing unnecessary checks in function _update_with_etag.

Why remove the checks instead of updating the docs?

To be more consistent with Java/Node API
It is specified in the docs that transaction is a set() operation, however, the current code checks the value as if it's an update() operation.

Code of set(), notice how it doesn't do this check at all and is using put like transaction()

firebase-admin-python/firebase_admin/db.py

Lines 146 to 161 in 99c48ef

    
               def set(self, value): 
        
                   """Sets the data at this location to the given value. 
        
                   The value must be JSON-serializable and not None. 
        
                   Args: 
        
                     value: JSON-serialable value to be set at this location. 
        
                   Raises: 
        
                     ValueError: If the value is None. 
        
                     TypeError: If the value is not JSON-serializable. 
        
                     ApiCallError: If an error occurs while communicating with the remote database server. 
        
                   """ 
        
                   if value is None: 
        
                       raise ValueError('Value must not be None.') 
        
                   self._client.request_oneway('put', self._add_suffix(), json=value, params='print=silent')

Code of update(), instead of using put, it's using patch. The original author might have borrowed the code from here:

firebase-admin-python/firebase_admin/db.py

Lines 186 to 200 in 99c48ef

    
               def update(self, value): 
        
                   """Updates the specified child keys of this Reference to the provided values. 
        
                   Args: 
        
                     value: A dictionary containing the child keys to update, and their new values. 
        
                   Raises: 
        
                     ValueError: If value is empty or not a dictionary. 
        
                     ApiCallError: If an error occurs while communicating with the remote database server. 
        
                   """ 
        
                   if not value or not isinstance(value, dict): 
        
                       raise ValueError('Value argument must be a non-empty dictionary.') 
        
                   if None in value.keys() or None in value.values(): 
        
                       raise ValueError('Dictionary must not contain None keys or values.') 
        
                   self._client.request_oneway('patch', self._add_suffix(), json=value, params='print=silent')

/cc @alexanderwhatley @hiranya911

andyfangdz · 2017-08-20T08:09:13Z

I noticed that there's already a PR that changes the API in #59. I think #59 fixes this problem, but it makes sense to merge this PR if public API review takes more time because currently there's no way to update a single leaf element.

…ython#63 to work

hiranya911 · 2017-08-20T22:17:04Z

Thanks @andyfangdz. This looks pretty good. Would you be able to provide an integration test for this as well? See integration/test_db.py and the integration testing guide for more information.

hiranya911

LGTM 👍

andyfangdz · 2017-08-20T22:45:37Z

@hiranya911 Indeed! Here's the integration test. The test passed on my local env. I also took the liberty of bumping a minor version since this change technically does not break anything already working with 2.2.0.

hiranya911 · 2017-08-21T17:41:09Z

Thanks @andyfangdz for the contribution.

andyfangdz · 2017-08-24T03:39:10Z

@hiranya911 Can you do a release for 2.2.1 on pypi? Thanks!

hiranya911 · 2017-08-24T20:51:00Z

@andyfangdz I was waiting to see if we can get #59 also approved, and do a 2.3.0 release sometime next week. I will do a release next week regardless of the status of #59. Sorry about the delay.

andyfangdz · 2017-08-25T05:52:17Z

@hiranya911 No worries! Thanks for the fast review 😄. Looking forward to #59!

hiranya911 · 2017-08-30T18:52:22Z

This was released today.

andyfangdz · 2017-08-30T18:53:48Z

Thanks!

andyfangdz added 2 commits August 20, 2017 03:33

Remove unnecessary checks on update with etag

13f06ed

Add test of scalar transaction

0ed11a7

andyfangdz added a commit to scc-gatech/infra-nebula that referenced this pull request Aug 20, 2017

Add an example job. Note that this requires firebase/firebase-admin-p…

fc5376a

…ython#63 to work

hiranya911 self-requested a review August 20, 2017 22:14

hiranya911 self-assigned this Aug 20, 2017

hiranya911 added the api: database label Aug 20, 2017

hiranya911 approved these changes Aug 20, 2017

View reviewed changes

andyfangdz added 2 commits August 20, 2017 18:43

Add integration test for scalar transactions

90001ef

bump version

702a049

hiranya911 merged commit 1a937b0 into firebase:master Aug 21, 2017

andyfangdz deleted the patch-1 branch August 21, 2017 17:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove unnecessary checks on update with etag #63

Remove unnecessary checks on update with etag #63

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

	def set(self, value):
	"""Sets the data at this location to the given value.

	The value must be JSON-serializable and not None.

	Args:
	value: JSON-serialable value to be set at this location.

	Raises:
	ValueError: If the value is None.
	TypeError: If the value is not JSON-serializable.
	ApiCallError: If an error occurs while communicating with the remote database server.
	"""
	if value is None:
	raise ValueError('Value must not be None.')
	self._client.request_oneway('put', self._add_suffix(), json=value, params='print=silent')

	def update(self, value):
	"""Updates the specified child keys of this Reference to the provided values.

	Args:
	value: A dictionary containing the child keys to update, and their new values.

	Raises:
	ValueError: If value is empty or not a dictionary.
	ApiCallError: If an error occurs while communicating with the remote database server.
	"""
	if not value or not isinstance(value, dict):
	raise ValueError('Value argument must be a non-empty dictionary.')
	if None in value.keys() or None in value.values():
	raise ValueError('Dictionary must not contain None keys or values.')
	self._client.request_oneway('patch', self._add_suffix(), json=value, params='print=silent')

Remove unnecessary checks on update with etag #63

Remove unnecessary checks on update with etag #63

Uh oh!

Conversation

Uh oh!

Why remove the checks instead of updating the docs?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!