Low Level ETag API #59
Conversation
firebase_admin/db.py
Outdated
| @@ -125,40 +125,91 @@ def child(self, path): | |||
| full_path = self._pathurl + '/' + path | |||
| return Reference(client=self._client, path=full_path) | |||
|
|
|||
| def get(self): | |||
| """Returns the value at the current location of the database. | |||
| def _get_with_etag(self): | |||
There was a problem hiding this comment.
How about we just move this logic straight into the get() method, and get rid of this helper? transaction() can call self.get(etag=True).
firebase_admin/db.py
Outdated
| etag = headers.get('ETag') | ||
| return etag, data | ||
|
|
||
| def get_etag(self): |
There was a problem hiding this comment.
Perhaps call it etag()? That sounds more consistent with the other methods we have in this module like child() and reference().
firebase_admin/db.py
Outdated
| headers={'X-Firebase-ETag' : 'true'}, | ||
| resp_headers=True, | ||
| params='print=silent') | ||
| etag = headers.get('ETag') |
There was a problem hiding this comment.
return headers.get('ETag')
firebase_admin/db.py
Outdated
| headers={'if-match': etag}) | ||
| except ApiCallError as error: | ||
| detail = error.detail | ||
| if detail.response.headers and 'ETag' in detail.response.headers: |
There was a problem hiding this comment.
Check for detail.response is not None here (check the code in master now).
firebase_admin/db.py
Outdated
|
|
||
| def delete(self): | ||
| """Deletes this node from the database. | ||
| """Deleted this node from the database. |
firebase_admin/db.py
Outdated
| headers={'X-Firebase-ETag' : 'true'}, | ||
| resp_headers=True) | ||
| etag = headers.get('ETag') | ||
| return etag, data |
There was a problem hiding this comment.
Recommend changing the order here to return data, etag
firebase_admin/db.py
Outdated
|
|
||
| Returns: | ||
| object: Decoded JSON value of the current database Reference. | ||
| object: ETag of the current database Reference. |
firebase_admin/db.py
Outdated
| def _set_with_etag(self, value, etag): | ||
| """Sets the data at this location to the specified value, if the etag matches. | ||
| """ | ||
| if not value or not isinstance(value, dict): |
There was a problem hiding this comment.
The value doesn't have to be a dict right? I think any JSON-serializable value should work here.
firebase_admin/db.py
Outdated
| success = True | ||
| snapshot = value | ||
| try: | ||
| self._client.request_oneway('put', self._add_suffix(), json=value, |
There was a problem hiding this comment.
I had done a bit of refactoring here. Make sure some of those changes remain.
firebase_admin/db.py
Outdated
| 'get', self._add_suffix(), headers={'X-Firebase-ETag' : 'true'}, resp_headers=True) | ||
| etag = headers.get('ETag') | ||
| return etag, data | ||
| def _set_with_etag(self, value, etag): |
There was a problem hiding this comment.
I wonder if this should also be made part of the set() method. That would allow callers of set(value, etag=True) to access the return values.
|
Looks pretty good overall. Just a few things to improve, and a couple of questions. |
|
Ok pushed some changes. There are a couple of lint errors in db.py that I'm not sure how to fix, perhaps you can weigh in? Thanks. |
firebase_admin/db.py
Outdated
|
|
||
| Returns: | ||
| object: Tuple of False, current location's etag, and snapshot of location's data | ||
| if passed in etag does not match. |
There was a problem hiding this comment.
Indent by 4 spaces. Lint error is possibly due to a trailing whitespace at the end of the line.
firebase_admin/db.py
Outdated
| TypeError: If the value is not JSON-serializable. | ||
| ApiCallError: If an error occurs while communicating with the remote database server. | ||
| ApiCallError: If an error occurs while communicating with the remote database server, | ||
| or if the ETag does not match. |
There was a problem hiding this comment.
Indent by 4 spaces. Might want to add a lint ignore directive here to fix the 2nd lint error.
# pylint: disable=missing-raises-doc
Might have to experiment a bit to get the placement of the directive right.
firebase_admin/db.py
Outdated
| @@ -235,38 +249,33 @@ def transaction(self, transaction_update): | |||
| Unlike a normal `set()`, which just overwrites the data regardless of its previous state, | |||
| `transaction()` is used to modify the existing value to a new value, ensuring there are | |||
| no conflicts with other clients simultaneously writing to the same location. | |||
|
|
|||
There was a problem hiding this comment.
Please add these lines back. Here and below.
firebase_admin/db.py
Outdated
| """Returns the value at the current location of the database. | ||
| def etag(self): | ||
| """Returns the ETag at the current location of the database. | ||
| Returns: |
There was a problem hiding this comment.
Add an empty line before Returns:.
firebase_admin/db.py
Outdated
|
|
||
| Returns: | ||
| object: Decoded JSON value of the current database Reference. | ||
| object: Decoded JSON value of the current database Reference if etag=False, otherwise | ||
| the decoded JSON value and the corresponding ETag. |
| F438 resp = self._do_request(method, urlpath, **kwargs) | ||
| if resp_headers: | ||
| if resp_headers and params == 'print=silent': |
There was a problem hiding this comment.
Can we just return resp.json(), resp.headers in this case? Does that trigger an error? If not I'd say simply return the tuple, and ignore the payload value (which I assume would get set to None).
There was a problem hiding this comment.
No, if print=silent, resp.json() throws an error message.
There was a problem hiding this comment.
Ok. Lets leave it as it is for now. I'll try to think of a better way to refactor this bit out.
|
Made some more changes, please take a look. |
There was a problem hiding this comment.
LGTM with a few open comments. I will get the internal review process for this started. Thanks @alexanderwhatley for another awesome contribution.
firebase_admin/db.py
Outdated
| params='print=silent') | ||
| return headers.get('ETag') | ||
|
|
||
| def get(self, etag=False): |
There was a problem hiding this comment.
Document the etag parameter.
Args:
etag: A boolean indicating whether to return the ETag of the current location along with the value (optional).
firebase_admin/db.py
Outdated
| """Sets the data at this location to the given value. | ||
|
|
firebase_admin/db.py
Outdated
|
|
||
| Args: | ||
| value: JSON-serialable value to be set at this location. | ||
| etag: Value of ETag that we want to check. |
There was a problem hiding this comment.
Add (optional). at the end.
| resp = self._do_request(method, urlpath, **kwargs) | ||
| if resp_headers: | ||
| if resp_headers and params == 'print=silent': |
There was a problem hiding this comment.
Ok. Lets leave it as it is for now. I'll try to think of a better way to refactor this bit out.
|
After thinking about this API a bit more deeply, I think I've arrived at the following design: I didn't much like the fact that I will use this as a starting point for our internal reviews. But lets not make any code changes just yet. Lets wait for the review process to come to an end. In either case I believe this PR has all the bits of logic necessary to support the functionality we wish to expose. So it should be easy to refactor the public API in anyway we want. |
|
Hi @alexanderwhatley. Sorry about the delay, but I finally have an update to share with you. After our internal review process, we have reached consensus on the following API: The argument was And after some time... I believe you already have code necessary for |
|
Sure, I'll try to get something out by tonight.
…On Thu, Aug 24, 2017 at 1:44 PM Hiranya Jayathilaka < ***@***.***> wrote:
Hi @alexanderwhatley <https://github.com/alexanderwhatley>. Sorry about
the delay, but I finally have an update to share with you. After our
internal review process, we have reached consensus on the following API:
ref.get(etag=False)
ref.update_if_unchanged(expected_etag, value) => (success, value, etag)
ref.get_if_changed(etag) => (success, value, etag)
The argument was get_if_changed() is more useful in practice than a
method that only returns the etag string. For example, in your caching
scenario:
# Initial read to populate cache
value, etag = ref.get(etag=True)
update_cache(ref.key, value, etag)
And after some time...
success, value, etag = ref.get_if_changed(etag_from_cache)
if success:
update_cache(ref.key, value, etag)
I believe you already have code necessary for get() and
update_if_unchanged(). To implement get_if_changed() you need to use the
if-none-match
<https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/If-None-Match>
header as mentioned by @caseycrogers <https://github.com/caseycrogers> in
#55 <#55>. Are
you in a position to make these changes in the PR? Otherwise I can try to
make the necessary changes on top of what you have implemented so far.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#59 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ANiP1rF_Lnvdaer_65kRCEvVM_IoMc2Pks5sbe7GgaJpZM4O4bJ1>
.
|
|
@hiranya911, I pushed some changes for the new API. It's not clear to me how to handle the case where the ETag matches in the |
|
@alexanderwhatley I suppose, for now, you can directly call |
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
|
Hi @hiranya911, I pushed some more changes. It still seems like if the if-none-match header matches the ETag provided, then the server does not return any of the data stored in the current location. |
ab0760f to
6f9d71a
Compare
|
CLAs look good, thanks! |
|
That is expected with a |
firebase_admin/db.py
Outdated
| @@ -125,31 +125,70 @@ def child(self, path): | |||
| full_path = self._pathurl + '/' + path | |||
| return Reference(client=self._client, path=full_path) | |||
|
|
|||
| def get(self): | |||
| """Returns the value at the current location of the database. | |||
| def etag(self): | |||
There was a problem hiding this comment.
Lets remove this method. Our API review team decided to implement get_if_changed() instead of etag().
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
eb21db2 to
2d48a78
Compare
|
CLAs look good, thanks! |
|
Done. I also made some changes to the unit tests. |
|
@alexanderwhatley Thank you for patiently seeing this through. Great to have you as a contributor. |
|
@alexanderwhatley I'm in the process of refactoring the DB module. However, I'm seeing a weird behavior with the new Steps:
At least in my test environment, after executing Update:Actually I see it now in master branch too. The integration test suite hangs after |
|
@hiranya911, hmm yeah, I'm getting it hanging too. It actually happened yesterday before I pushed the code as well, but I assumed it was due to the slow hotel wifi... |
|
Thanks for checking. I'm looking into it. This might be a server-side issue. |
|
This was partially released today. We removed the |
Implemented low level ETag API discussed in #55. I decided not to implement
check_etag, as it's easiest just to callget_etagand then check. I also renamed_update_with_etagto_set_with_etagas the name makes more sense.