Issue 6852082: Supporting NDB in oauth2client/appengine, and beginning *slow* transition to this datastore API. (Closed)
|
Created:
12 years, 7 months ago by dhermes Modified:
12 years, 7 months ago CC:
google-api-python-client_googlegroups.com Visibility:
Public. |
Patch Set 1 #
Total comments: 16
Patch Set 2 : Addressing some review comments. #
Total comments: 8
Patch Set 3 : Adding test for XSRF NDB stuff and moving implicit storage in OAuth deco back to DB. #Patch Set 4 : Updating docstrings #
Total comments: 8
Patch Set 5 : Fixing typo and adding note in StorageByKeyName about cache with NDB models. #Patch Set 6 : Fixing import order in test and adding extra assert to test_get_and_put_mixed_db_storage_ndb_get #Patch Set 7 : Add assert to test_get_and_put_ndb #MessagesTotal messages: 17
Hi Joe and Ali, This is support for NDB at parity with the DB models and properties provided. By using custom _get_kind for NDB models and using the same underlying pb types for the properties, these DB and NDB models can be used interchangeably to retrieve the same data (as the tests show). StorageByKeyName is changed to support both DB and NDB and when StorageByKeyName is used implicitly within OAuth2Decorator, I have replaced the DB model with the NDB model to initiate a slow transition away from DB. This can be reverted if you like as the change in caching behavior at the instance and memcache level may break certain existing implementations. If we provided this in release notes, I don't see why this would be an issue.
Sign in to reply to this message.
Overall, very nice. https://codereview.appspot.com/6852082/diff/1/oauth2client/appengine.py File oauth2client/appengine.py (right): https://codereview.appspot.com/6852082/diff/1/oauth2client/appengine.py#newco... oauth2client/appengine.py:34: from google.appengine.ext import ndb Is there any situation (old runtimes) where this import will file? https://codereview.appspot.com/6852082/diff/1/oauth2client/appengine.py#newco... oauth2client/appengine.py:380: if isinstance(self._model, type): What is the purpose of this, just to check it is a class? Is it ok that it fails for old style classes? Are you checking it is a type so that issubclass succeeds without error? Wouldn't an exception feel more natural here? you will be raising a TypeError anyway in that case. The control flow is duplicated from _get_entity. I would be tempted to extract the test to a separate function. https://codereview.appspot.com/6852082/diff/1/tests/test_oauth2client_appengi... File tests/test_oauth2client_appengine.py (right): https://codereview.appspot.com/6852082/diff/1/tests/test_oauth2client_appengi... tests/test_oauth2client_appengine.py:328: It's full of blank lines! https://codereview.appspot.com/6852082/diff/1/tests/test_oauth2client_appengi... tests/test_oauth2client_appengine.py:361: storage2 = StorageByKeyName( Separate test?
Sign in to reply to this message.
https://codereview.appspot.com/6852082/diff/1/oauth2client/appengine.py File oauth2client/appengine.py (right): https://codereview.appspot.com/6852082/diff/1/oauth2client/appengine.py#newco... oauth2client/appengine.py:34: from google.appengine.ext import ndb On 2012/11/22 06:01:17, Ali Afshar wrote: > Is there any situation (old runtimes) where this import will file? I assume you mean fail. Both py25 and py27 allow it. There is no such thing as "old" runtime versions in production, if that is what you meant.. If they have an SDK older than 1.6.4 (it seems this is when ndb was added http://code.google.com/p/googleappengine/wiki/SdkReleaseNotes) this will fail locally/while testing, but that is a great impetus to upgrade. https://codereview.appspot.com/6852082/diff/1/oauth2client/appengine.py#newco... oauth2client/appengine.py:380: if isinstance(self._model, type): On 2012/11/22 06:01:17, Ali Afshar wrote: > What is the purpose of this, just to check it is a class? Is it ok that it fails > for old style classes? Are you checking it is a type so that issubclass succeeds > without error? Wouldn't an exception feel more natural here? you will be raising > a TypeError anyway in that case. The control flow is duplicated from > _get_entity. I would be tempted to extract the test to a separate function. Yes, it was to prevent issubclass from failing, which it does if either of the arguments are not classes. It is quite OK that it fails for new style classes, we only care about db.Model and ndb.Model subclasses, which must be new-style. I was taking my lead from parts of the Python SDK. Contemplating what a separate function would look like, it feels like returning a bool like is_ndb doesn't allow future datastore APIs to live in harmony. This may not be such a big deal though. If you think something which will return a bool and we can use it in a simple if-else, I can change it to that. https://codereview.appspot.com/6852082/diff/1/tests/test_oauth2client_appengi... File tests/test_oauth2client_appengine.py (right): https://codereview.appspot.com/6852082/diff/1/tests/test_oauth2client_appengi... tests/test_oauth2client_appengine.py:328: On 2012/11/22 06:01:17, Ali Afshar wrote: > It's full of blank lines! Acknowledged. https://codereview.appspot.com/6852082/diff/1/tests/test_oauth2client_appengi... tests/test_oauth2client_appengine.py:361: storage2 = StorageByKeyName( On 2012/11/22 06:01:17, Ali Afshar wrote: > Separate test? No. This was intended to show that a mix of DB and NDB works fine "test_get_and_put_db_ndb_mixed".
Sign in to reply to this message.
https://codereview.appspot.com/6852082/diff/1/oauth2client/appengine.py File oauth2client/appengine.py (right): https://codereview.appspot.com/6852082/diff/1/oauth2client/appengine.py#newco... oauth2client/appengine.py:34: from google.appengine.ext import ndb On 2012/11/22 18:01:42, dhermes wrote: > On 2012/11/22 06:01:17, Ali Afshar wrote: > > Is there any situation (old runtimes) where this import will file? > > I assume you mean fail. Both py25 and py27 allow it. There is no such thing as > "old" runtime versions in production, if that is what you meant.. > > If they have an SDK older than 1.6.4 (it seems this is when ndb was added > http://code.google.com/p/googleappengine/wiki/SdkReleaseNotes) this will fail > locally/while testing, but that is a great impetus to upgrade. What about someone with an old SDK installed on their machine? Prod won't be a problem. https://codereview.appspot.com/6852082/diff/1/oauth2client/appengine.py#newco... oauth2client/appengine.py:380: if isinstance(self._model, type): On 2012/11/22 18:01:42, dhermes wrote: > On 2012/11/22 06:01:17, Ali Afshar wrote: > > What is the purpose of this, just to check it is a class? Is it ok that it > fails > > for old style classes? Are you checking it is a type so that issubclass > succeeds > > without error? Wouldn't an exception feel more natural here? you will be > raising > > a TypeError anyway in that case. The control flow is duplicated from > > _get_entity. I would be tempted to extract the test to a separate function. > > Yes, it was to prevent issubclass from failing, which it does if either of the > arguments are not classes. It is quite OK that it fails for new style classes, > we only care about db.Model and ndb.Model subclasses, which must be new-style. > > I was taking my lead from parts of the Python SDK. Contemplating what a separate > function would look like, it feels like returning a bool like is_ndb doesn't > allow future datastore APIs to live in harmony. This may not be such a big deal > though. If you think something which will return a bool and we can use it in a > simple if-else, I can change it to that. Well, I would: 1. obviously extract the duplicated code (unarguable) - it's not a boolean either, there are 3 (maybe 4) possible results here. 2. use something obvious to catch the failing issubclass check. I guessed what it was there for, but then I shouldn't have to guess. try, except is the most obvious. https://codereview.appspot.com/6852082/diff/1/tests/test_oauth2client_appengi... File tests/test_oauth2client_appengine.py (right): https://codereview.appspot.com/6852082/diff/1/tests/test_oauth2client_appengi... tests/test_oauth2client_appengine.py:328: On 2012/11/22 18:01:42, dhermes wrote: > On 2012/11/22 06:01:17, Ali Afshar wrote: > > It's full of blank lines! > > Acknowledged. So, remove them, maybe? Or explain a consistent strategy for when you are adding them. Because right now it looks like they are just when you feel like it. https://codereview.appspot.com/6852082/diff/1/tests/test_oauth2client_appengi... tests/test_oauth2client_appengine.py:361: storage2 = StorageByKeyName( On 2012/11/22 18:01:42, dhermes wrote: > On 2012/11/22 06:01:17, Ali Afshar wrote: > > Separate test? > > No. This was intended to show that a mix of DB and NDB works fine > "test_get_and_put_db_ndb_mixed". Fine if the tests are as atomic as you can make them.
Sign in to reply to this message.
https://codereview.appspot.com/6852082/diff/1/oauth2client/appengine.py File oauth2client/appengine.py (right): https://codereview.appspot.com/6852082/diff/1/oauth2client/appengine.py#newco... oauth2client/appengine.py:34: from google.appengine.ext import ndb I'm not too worried about someone with an SDK that is 6+ months old. It is built to nag by default. On 2012/11/23 07:21:53, Ali Afshar wrote: > On 2012/11/22 18:01:42, dhermes wrote: > > On 2012/11/22 06:01:17, Ali Afshar wrote: > > > Is there any situation (old runtimes) where this import will file? > > > > I assume you mean fail. Both py25 and py27 allow it. There is no such thing as > > "old" runtime versions in production, if that is what you meant.. > > > > If they have an SDK older than 1.6.4 (it seems this is when ndb was added > > http://code.google.com/p/googleappengine/wiki/SdkReleaseNotes) this will fail > > locally/while testing, but that is a great impetus to upgrade. > > What about someone with an old SDK installed on their machine? Prod won't be a > problem. https://codereview.appspot.com/6852082/diff/1/oauth2client/appengine.py#newco... oauth2client/appengine.py:380: if isinstance(self._model, type): 1. There are more than 2 states, but not more than 2 return values, as the rest can be raised as exceptions. See code. 2. Since we're only worried about (n)db.Model subclasses, it's easy enough to do this and still have control over the message in the TypeError. On 2012/11/23 07:21:53, Ali Afshar wrote: > On 2012/11/22 18:01:42, dhermes wrote: > > On 2012/11/22 06:01:17, Ali Afshar wrote: > > > What is the purpose of this, just to check it is a class? Is it ok that it > > fails > > > for old style classes? Are you checking it is a type so that issubclass > > succeeds > > > without error? Wouldn't an exception feel more natural here? you will be > > raising > > > a TypeError anyway in that case. The control flow is duplicated from > > > _get_entity. I would be tempted to extract the test to a separate function. > > > > Yes, it was to prevent issubclass from failing, which it does if either of the > > arguments are not classes. It is quite OK that it fails for new style classes, > > we only care about db.Model and ndb.Model subclasses, which must be new-style. > > > > I was taking my lead from parts of the Python SDK. Contemplating what a > separate > > function would look like, it feels like returning a bool like is_ndb doesn't > > allow future datastore APIs to live in harmony. This may not be such a big > deal > > though. If you think something which will return a bool and we can use it in a > > simple if-else, I can change it to that. > > Well, I would: > > 1. obviously extract the duplicated code (unarguable) - it's not a boolean > either, there are 3 (maybe 4) possible results here. > 2. use something obvious to catch the failing issubclass check. I guessed what > it was there for, but then I shouldn't have to guess. try, except is the most > obvious. https://codereview.appspot.com/6852082/diff/1/tests/test_oauth2client_appengi... File tests/test_oauth2client_appengine.py (right): https://codereview.appspot.com/6852082/diff/1/tests/test_oauth2client_appengi... tests/test_oauth2client_appengine.py:328: On 2012/11/23 07:21:53, Ali Afshar wrote: > On 2012/11/22 18:01:42, dhermes wrote: > > On 2012/11/22 06:01:17, Ali Afshar wrote: > > > It's full of blank lines! > > > > Acknowledged. > > So, remove them, maybe? Or explain a consistent strategy for when you are adding > them. Because right now it looks like they are just when you feel like it. Harsh. Added comments for distinction. https://codereview.appspot.com/6852082/diff/1/tests/test_oauth2client_appengi... tests/test_oauth2client_appengine.py:361: storage2 = StorageByKeyName( On 2012/11/23 07:21:53, Ali Afshar wrote: > On 2012/11/22 18:01:42, dhermes wrote: > > On 2012/11/22 06:01:17, Ali Afshar wrote: > > > Separate test? > > > > No. This was intended to show that a mix of DB and NDB works fine > > "test_get_and_put_db_ndb_mixed". > > Fine if the tests are as atomic as you can make them. Done. Separated them into db->ndb and ndb->db.
Sign in to reply to this message.
LGTM On 2012/11/23 23:17:53, dhermes wrote: > https://codereview.appspot.com/6852082/diff/1/oauth2client/appengine.py > File oauth2client/appengine.py (right): > > https://codereview.appspot.com/6852082/diff/1/oauth2client/appengine.py#newco... > oauth2client/appengine.py:34: from google.appengine.ext import ndb > I'm not too worried about someone with an SDK that is 6+ months old. It is built > to nag by default. > > On 2012/11/23 07:21:53, Ali Afshar wrote: > > On 2012/11/22 18:01:42, dhermes wrote: > > > On 2012/11/22 06:01:17, Ali Afshar wrote: > > > > Is there any situation (old runtimes) where this import will file? > > > > > > I assume you mean fail. Both py25 and py27 allow it. There is no such thing > as > > > "old" runtime versions in production, if that is what you meant.. > > > > > > If they have an SDK older than 1.6.4 (it seems this is when ndb was added > > > http://code.google.com/p/googleappengine/wiki/SdkReleaseNotes) this will > fail > > > locally/while testing, but that is a great impetus to upgrade. > > > > What about someone with an old SDK installed on their machine? Prod won't be a > > problem. > > https://codereview.appspot.com/6852082/diff/1/oauth2client/appengine.py#newco... > oauth2client/appengine.py:380: if isinstance(self._model, type): > 1. There are more than 2 states, but not more than 2 return values, as the rest > can be raised as exceptions. See code. > > 2. Since we're only worried about (n)db.Model subclasses, it's easy enough to do > this and still have control over the message in the TypeError. > > On 2012/11/23 07:21:53, Ali Afshar wrote: > > On 2012/11/22 18:01:42, dhermes wrote: > > > On 2012/11/22 06:01:17, Ali Afshar wrote: > > > > What is the purpose of this, just to check it is a class? Is it ok that it > > > fails > > > > for old style classes? Are you checking it is a type so that issubclass > > > succeeds > > > > without error? Wouldn't an exception feel more natural here? you will be > > > raising > > > > a TypeError anyway in that case. The control flow is duplicated from > > > > _get_entity. I would be tempted to extract the test to a separate > function. > > > > > > Yes, it was to prevent issubclass from failing, which it does if either of > the > > > arguments are not classes. It is quite OK that it fails for new style > classes, > > > we only care about db.Model and ndb.Model subclasses, which must be > new-style. > > > > > > I was taking my lead from parts of the Python SDK. Contemplating what a > > separate > > > function would look like, it feels like returning a bool like is_ndb doesn't > > > allow future datastore APIs to live in harmony. This may not be such a big > > deal > > > though. If you think something which will return a bool and we can use it in > a > > > simple if-else, I can change it to that. > > > > Well, I would: > > > > 1. obviously extract the duplicated code (unarguable) - it's not a boolean > > either, there are 3 (maybe 4) possible results here. > > 2. use something obvious to catch the failing issubclass check. I guessed what > > it was there for, but then I shouldn't have to guess. try, except is the most > > obvious. > > https://codereview.appspot.com/6852082/diff/1/tests/test_oauth2client_appengi... > File tests/test_oauth2client_appengine.py (right): > > https://codereview.appspot.com/6852082/diff/1/tests/test_oauth2client_appengi... > tests/test_oauth2client_appengine.py:328: > On 2012/11/23 07:21:53, Ali Afshar wrote: > > On 2012/11/22 18:01:42, dhermes wrote: > > > On 2012/11/22 06:01:17, Ali Afshar wrote: > > > > It's full of blank lines! > > > > > > Acknowledged. > > > > So, remove them, maybe? Or explain a consistent strategy for when you are > adding > > them. Because right now it looks like they are just when you feel like it. > > Harsh. Added comments for distinction. > > https://codereview.appspot.com/6852082/diff/1/tests/test_oauth2client_appengi... > tests/test_oauth2client_appengine.py:361: storage2 = StorageByKeyName( > On 2012/11/23 07:21:53, Ali Afshar wrote: > > On 2012/11/22 18:01:42, dhermes wrote: > > > On 2012/11/22 06:01:17, Ali Afshar wrote: > > > > Separate test? > > > > > > No. This was intended to show that a mix of DB and NDB works fine > > > "test_get_and_put_db_ndb_mixed". > > > > Fine if the tests are as atomic as you can make them. > > Done. Separated them into db->ndb and ndb->db.
Sign in to reply to this message.
So can I commit? Shall I wait for Joe's review (I know it's a holiday, not trying to rush anyone)? On Fri, Nov 23, 2012 at 7:09 PM, <afshar@google.com> wrote: > LGTM > > > On 2012/11/23 23:17:53, dhermes wrote: > > https://codereview.appspot.**com/6852082/diff/1/** > oauth2client/appengine.py<https://codereview.appspot.com/6852082/diff/1/oauth2client/appengine.py> > >> File oauth2client/appengine.py (right): >> > > > https://codereview.appspot.**com/6852082/diff/1/** > oauth2client/appengine.py#**newcode34<https://codereview.appspot.com/6852082/diff/1/oauth2client/appengine.py#newcode34> > >> oauth2client/appengine.py:34: from google.appengine.ext import ndb >> I'm not too worried about someone with an SDK that is 6+ months old. >> > It is built > >> to nag by default. >> > > On 2012/11/23 07:21:53, Ali Afshar wrote: >> > On 2012/11/22 18:01:42, dhermes wrote: >> > > On 2012/11/22 06:01:17, Ali Afshar wrote: >> > > > Is there any situation (old runtimes) where this import will >> > file? > >> > > >> > > I assume you mean fail. Both py25 and py27 allow it. There is no >> > such thing > >> as >> > > "old" runtime versions in production, if that is what you meant.. >> > > >> > > If they have an SDK older than 1.6.4 (it seems this is when ndb >> > was added > >> > > http://code.google.com/p/**googleappengine/wiki/**SdkReleaseNotes<http://code... >> ) >> > this will > >> fail >> > > locally/while testing, but that is a great impetus to upgrade. >> > >> > What about someone with an old SDK installed on their machine? Prod >> > won't be a > >> > problem. >> > > > https://codereview.appspot.**com/6852082/diff/1/** > oauth2client/appengine.py#**newcode380<https://codereview.appspot.com/6852082/diff/1/oauth2client/appengine.py#newcode380> > >> oauth2client/appengine.py:380: if isinstance(self._model, type): >> 1. There are more than 2 states, but not more than 2 return values, as >> > the rest > >> can be raised as exceptions. See code. >> > > 2. Since we're only worried about (n)db.Model subclasses, it's easy >> > enough to do > >> this and still have control over the message in the TypeError. >> > > On 2012/11/23 07:21:53, Ali Afshar wrote: >> > On 2012/11/22 18:01:42, dhermes wrote: >> > > On 2012/11/22 06:01:17, Ali Afshar wrote: >> > > > What is the purpose of this, just to check it is a class? Is it >> > ok that it > >> > > fails >> > > > for old style classes? Are you checking it is a type so that >> > issubclass > >> > > succeeds >> > > > without error? Wouldn't an exception feel more natural here? you >> > will be > >> > > raising >> > > > a TypeError anyway in that case. The control flow is duplicated >> > from > >> > > > _get_entity. I would be tempted to extract the test to a >> > separate > >> function. >> > > >> > > Yes, it was to prevent issubclass from failing, which it does if >> > either of > >> the >> > > arguments are not classes. It is quite OK that it fails for new >> > style > >> classes, >> > > we only care about db.Model and ndb.Model subclasses, which must >> > be > >> new-style. >> > > >> > > I was taking my lead from parts of the Python SDK. Contemplating >> > what a > >> > separate >> > > function would look like, it feels like returning a bool like >> > is_ndb doesn't > >> > > allow future datastore APIs to live in harmony. This may not be >> > such a big > >> > deal >> > > though. If you think something which will return a bool and we can >> > use it in > >> a >> > > simple if-else, I can change it to that. >> > >> > Well, I would: >> > >> > 1. obviously extract the duplicated code (unarguable) - it's not a >> > boolean > >> > either, there are 3 (maybe 4) possible results here. >> > 2. use something obvious to catch the failing issubclass check. I >> > guessed what > >> > it was there for, but then I shouldn't have to guess. try, except is >> > the most > >> > obvious. >> > > > https://codereview.appspot.**com/6852082/diff/1/tests/test_** > oauth2client_appengine.py<https://codereview.appspot.com/6852082/diff/1/tests/test_oauth2client_appengine.py> > >> File tests/test_oauth2client_**appengine.py (right): >> > > > https://codereview.appspot.**com/6852082/diff/1/tests/test_** > oauth2client_appengine.py#**newcode328<https://codereview.appspot.com/6852082/diff/1/tests/test_oauth2client_appengine.py#newcode328> > >> tests/test_oauth2client_**appengine.py:328: >> On 2012/11/23 07:21:53, Ali Afshar wrote: >> > On 2012/11/22 18:01:42, dhermes wrote: >> > > On 2012/11/22 06:01:17, Ali Afshar wrote: >> > > > It's full of blank lines! >> > > >> > > Acknowledged. >> > >> > So, remove them, maybe? Or explain a consistent strategy for when >> > you are > >> adding >> > them. Because right now it looks like they are just when you feel >> > like it. > > Harsh. Added comments for distinction. >> > > > https://codereview.appspot.**com/6852082/diff/1/tests/test_** > oauth2client_appengine.py#**newcode361<https://codereview.appspot.com/6852082/diff/1/tests/test_oauth2client_appengine.py#newcode361> > >> tests/test_oauth2client_**appengine.py:361: storage2 = StorageByKeyName( >> On 2012/11/23 07:21:53, Ali Afshar wrote: >> > On 2012/11/22 18:01:42, dhermes wrote: >> > > On 2012/11/22 06:01:17, Ali Afshar wrote: >> > > > Separate test? >> > > >> > > No. This was intended to show that a mix of DB and NDB works fine >> > > "test_get_and_put_db_ndb_**mixed". >> > >> > Fine if the tests are as atomic as you can make them. >> > > Done. Separated them into db->ndb and ndb->db. >> > > > > https://codereview.appspot.**com/6852082/<https://codereview.appspot.com/6852... > -- Danny Hermes Developer Programs Engineer
Sign in to reply to this message.
Please always wait for Joe's review. Thanks.
Sign in to reply to this message.
https://codereview.appspot.com/6852082/diff/6001/oauth2client/appengine.py File oauth2client/appengine.py (right): https://codereview.appspot.com/6852082/diff/6001/oauth2client/appengine.py#ne... oauth2client/appengine.py:85: class SiteXsrfSecretKeyNDB(ndb.Model): This isn't used anywhere, is it really just another name for SiteXsrfSecretKey? If so that should be explained in the PyDoc and there should be unit test that depends on that relationship, even if the test is trivial. https://codereview.appspot.com/6852082/diff/6001/oauth2client/appengine.py#ne... oauth2client/appengine.py:610: CredentialsNDBModel, user.user_id(), 'credentials').get() Why bother making this change here? If we supply a CredentialsNDBModel for users if they want to do that, and they can access the model either via CredentialsModel or CredentialsNDBModel, then why bother changing it here? https://codereview.appspot.com/6852082/diff/6001/tests/test_oauth2client_appe... File tests/test_oauth2client_appengine.py (right): https://codereview.appspot.com/6852082/diff/6001/tests/test_oauth2client_appe... tests/test_oauth2client_appengine.py:365: self.assertEqual('bar', credmodel.credentials.access_token) Test equality by using to_json() and comparing the results.
Sign in to reply to this message.
https://codereview.appspot.com/6852082/diff/6001/oauth2client/appengine.py File oauth2client/appengine.py (right): https://codereview.appspot.com/6852082/diff/6001/oauth2client/appengine.py#ne... oauth2client/appengine.py:85: class SiteXsrfSecretKeyNDB(ndb.Model): As with the FlowProperty, this are intended to be secondary options for people who wish to use NDB instead of DB, even if they aren't explicitly used in this update. It is not just another name, as using NDB enables instance and memcache caching of values which in turn results in faster entity retrieval. As with credentials model below, since they use the same kind, they can be used interchangeably. Updated the PyDoc to explain that it is intended as an NDB alternative and added a small test of the two models interacting. On 2012/11/25 03:12:08, jcgregorio_google wrote: > This isn't used anywhere, is it really just another name for SiteXsrfSecretKey? > If so that should be explained in the PyDoc and there should be unit test that > depends on that relationship, even if the test is trivial. https://codereview.appspot.com/6852082/diff/6001/oauth2client/appengine.py#ne... oauth2client/appengine.py:610: CredentialsNDBModel, user.user_id(), 'credentials').get() You're right. I was trying to begin a transition to NDB with a gentle nudge, but as you pointed out, it won't matter as the NDB model will still work elsewhere. On 2012/11/25 03:12:08, jcgregorio_google wrote: > Why bother making this change here? If we supply a CredentialsNDBModel for users > if they want to do that, and they can access the model either via > CredentialsModel or CredentialsNDBModel, then why bother changing it here? https://codereview.appspot.com/6852082/diff/6001/tests/test_oauth2client_appe... File tests/test_oauth2client_appengine.py (right): https://codereview.appspot.com/6852082/diff/6001/tests/test_oauth2client_appe... tests/test_oauth2client_appengine.py:365: self.assertEqual('bar', credmodel.credentials.access_token) On 2012/11/25 03:12:08, jcgregorio_google wrote: > Test equality by using to_json() and comparing the results. Done.
Sign in to reply to this message.
https://codereview.appspot.com/6852082/diff/6001/oauth2client/appengine.py File oauth2client/appengine.py (right): https://codereview.appspot.com/6852082/diff/6001/oauth2client/appengine.py#ne... oauth2client/appengine.py:85: class SiteXsrfSecretKeyNDB(ndb.Model): I'd like a longer explanation in the PyDoc, particularly with SiteXsrfSecretKeyNDB, as it claims it's a singleton, but there is also SiteXsrfSecretKey, which claims the same thing. I'd also like PyDoc comments about the relationship between the NDB and non-NDB classes in all the additions here. On 2012/11/25 19:45:08, dhermes wrote: > As with the FlowProperty, this are intended to be secondary options for people > who wish to use NDB instead of DB, even if they aren't explicitly used in this > update. > > It is not just another name, as using NDB enables instance and memcache caching > of values which in turn results in faster entity retrieval. As with credentials > model below, since they use the same kind, they can be used interchangeably. > > Updated the PyDoc to explain that it is intended as an NDB alternative and added > a small test of the two models interacting. > > On 2012/11/25 03:12:08, jcgregorio_google wrote: > > This isn't used anywhere, is it really just another name for > SiteXsrfSecretKey? > > If so that should be explained in the PyDoc and there should be unit test that > > depends on that relationship, even if the test is trivial. >
Sign in to reply to this message.
https://codereview.appspot.com/6852082/diff/6001/oauth2client/appengine.py File oauth2client/appengine.py (right): https://codereview.appspot.com/6852082/diff/6001/oauth2client/appengine.py#ne... oauth2client/appengine.py:85: class SiteXsrfSecretKeyNDB(ndb.Model): On 2012/11/26 04:20:09, jcgregorio_google wrote: > I'd like a longer explanation in the PyDoc, particularly with > SiteXsrfSecretKeyNDB, as it claims it's a singleton, but there is also > SiteXsrfSecretKey, which claims the same thing. I'd also like PyDoc comments > about the relationship between the NDB and non-NDB classes in all the additions > here. > > > On 2012/11/25 19:45:08, dhermes wrote: > > As with the FlowProperty, this are intended to be secondary options for people > > who wish to use NDB instead of DB, even if they aren't explicitly used in this > > update. > > > > It is not just another name, as using NDB enables instance and memcache > caching > > of values which in turn results in faster entity retrieval. As with > credentials > > model below, since they use the same kind, they can be used interchangeably. > > > > Updated the PyDoc to explain that it is intended as an NDB alternative and > added > > a small test of the two models interacting. > > > > On 2012/11/25 03:12:08, jcgregorio_google wrote: > > > This isn't used anywhere, is it really just another name for > > SiteXsrfSecretKey? > > > If so that should be explained in the PyDoc and there should be unit test > that > > > depends on that relationship, even if the test is trivial. > > > Done.
Sign in to reply to this message.
https://codereview.appspot.com/6852082/diff/9003/oauth2client/appengine.py File oauth2client/appengine.py (right): https://codereview.appspot.com/6852082/diff/9003/oauth2client/appengine.py#ne... oauth2client/appengine.py:296: Since CredentialsProperty stores data as s blob and this inherits from as a blob https://codereview.appspot.com/6852082/diff/9003/oauth2client/appengine.py#ne... oauth2client/appengine.py:367: cache: memcache, a write-through cache to put in front of the datastore Wouldn't the cache property would be redundant if using an ndb.Model? If so add that to the pydoc comments.
Sign in to reply to this message.
https://codereview.appspot.com/6852082/diff/9003/oauth2client/appengine.py File oauth2client/appengine.py (right): https://codereview.appspot.com/6852082/diff/9003/oauth2client/appengine.py#ne... oauth2client/appengine.py:296: Since CredentialsProperty stores data as s blob and this inherits from On 2012/11/26 18:12:13, jcgregorio_google wrote: > as a blob Done. https://codereview.appspot.com/6852082/diff/9003/oauth2client/appengine.py#ne... oauth2client/appengine.py:367: cache: memcache, a write-through cache to put in front of the datastore On 2012/11/26 18:12:13, jcgregorio_google wrote: > Wouldn't the cache property would be redundant if using an ndb.Model? If so add > that to the pydoc comments. Done.
Sign in to reply to this message.
https://codereview.appspot.com/6852082/diff/9003/tests/test_oauth2client_appe... File tests/test_oauth2client_appengine.py (right): https://codereview.appspot.com/6852082/diff/9003/tests/test_oauth2client_appe... tests/test_oauth2client_appengine.py:62: from oauth2client.appengine import FlowNDBProperty import order https://codereview.appspot.com/6852082/diff/9003/tests/test_oauth2client_appe... tests/test_oauth2client_appengine.py:381: self.assertEqual('bar', credmodel.credentials.access_token) Use self.assertEqual(self.credentials.to_json(), credmodel.credentials.to_json()) here and also in test_get_and_put_ndb()
Sign in to reply to this message.
https://codereview.appspot.com/6852082/diff/9003/tests/test_oauth2client_appe... File tests/test_oauth2client_appengine.py (right): https://codereview.appspot.com/6852082/diff/9003/tests/test_oauth2client_appe... tests/test_oauth2client_appengine.py:62: from oauth2client.appengine import FlowNDBProperty On 2012/11/26 18:34:39, jcgregorio_google wrote: > import order Done. https://codereview.appspot.com/6852082/diff/9003/tests/test_oauth2client_appe... tests/test_oauth2client_appengine.py:381: self.assertEqual('bar', credmodel.credentials.access_token) On 2012/11/26 18:34:39, jcgregorio_google wrote: > Use self.assertEqual(self.credentials.to_json(), > credmodel.credentials.to_json()) here and also in test_get_and_put_ndb() Done.
Sign in to reply to this message.
|
