Code review comment for lp:~salgado/launchpad/remove-auth-store

On Thu, 2010-04-01 at 15:12 +0000, Stuart Bishop wrote:
> Review: Approve
> On Wed, Mar 31, 2010 at 2:00 AM, Guilherme Salgado
> <email address hidden> wrote:
>
> > Remove the auth store, together with c-i-p.
> >
> > Had to grant some extra DB permissions on security.cfg because some
> > things that used to be done using the launchpad_auth user (the one
> > always used for the auth store, regardless of what was given to
> > initZopeless) are now done using whichever user is used by the
> > script/test.
>
> Yay. This is more than I expected.

More than I expected too. :/

And to make things worse, there are a couple extra revisions that
weren't in the diff you reviewed (I'm assuming you reviewed the diff
that was sent out when the m-p was created):

http://bazaar.launchpad.net/~salgado/launchpad/remove-auth-store/revision/10563
http://bazaar.launchpad.net/~salgado/launchpad/remove-auth-store/revision/10564

They're trivial, though.

>
>
>
> > === modified file 'lib/canonical/launchpad/database/tests/test_oauth.py'
> > --- lib/canonical/launchpad/database/tests/test_oauth.py 2009-06-25 05:30:52 +0000
> > +++ lib/canonical/launchpad/database/tests/test_oauth.py 2010-03-30 19:00:44 +0000
> > @@ -22,7 +22,7 @@
> > """Base tests for the OAuth database classes."""
> > layer = DatabaseFunctionalLayer
> >
> > - def test__get_store_should_return_the_auth_master_store(self):
> > + def test__get_store_should_return_the_main_master_store(self):
> > """We want all OAuth classes to use the master store.
> > Otherwise, the OAuth exchanges will fail because the authorize
> > screen won't probably find the new request token on the slave store.
>
> Does this test serve any purpose any more? Maybe remove it entirely.
>

AIUI, it's necessary because we want to make sure that we use the master
store (by default) when dealing with OAuth-related objects. (The name
of the test method was actually wrong; this is not related to the other
changes on this branch)

>
> > === modified file 'lib/canonical/launchpad/webapp/adapter.py'
> > try:
> > + # XXX: Salgado, 2010-03-22: We can get rid of the realm now that
> > + # the auth DB is gone, but I'm not doing it in this branch to keep
> > + # the diff simple.
> > config_section, realm, flavor = self._uri.database.split('-')
> > except ValueError:
> > raise AssertionError(
> > 'Connection uri %s does not match section-realm-flavor format'
> > % repr(self._uri.database))
>
> I don't think this XXX is warranted. We may well have more realms in
> the future, for example if we move translations or codehosting to
> their own database for scalability reasons.

Fair enough; I thought we'd remove that because I had never heard of
these plans.

>
> > === modified file 'lib/canonical/launchpad/webapp/ftests/test_adapter_permissions.txt'
> > -
> > -A MASTER_FLAVOR Store does not allow writes to tables outside of that
> > -Store's replication set.
> > -
> > - >>> t = transaction.begin()
> > - >>> person = main_master.find(Person, name='no-priv').one()
> > - >>> account = person.account
> > - >>> account.displayname = "Ben Dover"
> > - >>> main_master.flush()
> > - Traceback (most recent call last):
> > - ...
> > - ProgrammingError: permission denied for relation account
> > - >>> transaction.abort()
> > -
> > - >>> t = transaction.begin()
> > - >>> auth_master = getUtility(IStoreSelector).get(AUTH_STORE, MASTER_FLAVOR)
> > - >>> person = auth_master.find(Person, name='no-priv').one()
> > - >>> account = person.account
> > - >>> account.displayname = "Ben Dover"
> > - >>> auth_master.flush()
> > - >>> transaction.abort()
>
> But this crap can certainly go. If we split the database in the
> future, there will be no crossover. What I've learned from the
> authdb/lpmain split is that the split needs to be clean, or we end up
> with confusing code (is my email address from the authdb Store or the
> lpmain Store) and a complicated and tricky to maintain replication
> setup. The underlying approach mapping the database class to a Storm
> store is a suitable approach to the sort of sharding that Launchpad
> will need one day.

At this point I can't even remember why we had EmailAddress (& co) on
both databases?

>
>
>
> > + # XXX: 2010-03-24, Guilherme Salgado: The 'name' argument will always be
> > + # MAIN_STORE here, so it can be removed. Not doing it now as this branch
> > + # will be CPed into production.
> > def getStore(name, flavor):
> > """Retrieve a Store.
>
> I think this XXX can go do. I'd rather not re implement this next year :)
>
>
> > @@ -843,6 +843,9 @@
> > def get_current():
> > """Return the currently installed `IDatabasePolicy`."""
> >
> > + # XXX: 2010-03-24, Guilherme Salgado: The 'name' argument will always be
> > + # MAIN_STORE here, so it can be removed. Not doing it now as this branch
> > + # will be CPed into production.
> > def get(name, flavor):
> > """Retrieve a Storm Store.
>
> And here.

Sure, removed both.

>
>
> > === modified file 'lib/canonical/launchpad/webapp/tests/test_dbpolicy.py'
> > --- lib/canonical/launchpad/webapp/tests/test_dbpolicy.py 2010-02-24 23:18:40 +0000
> > +++ lib/canonical/launchpad/webapp/tests/test_dbpolicy.py 2010-03-30 19:00:44 +0000
> > @@ -25,7 +25,7 @@
> > ReadOnlyLaunchpadDatabasePolicy, SlaveDatabasePolicy,
> > SlaveOnlyDatabasePolicy)
> > from canonical.launchpad.webapp.interfaces import (
> > - ALL_STORES, AUTH_STORE, DEFAULT_FLAVOR, DisallowedStore, IDatabasePolicy,
> > + DEFAULT_FLAVOR, DisallowedStore, IDatabasePolicy,
> > IStoreSelector, MAIN_STORE, MASTER_FLAVOR, ReadOnlyModeDisallowedStore,
> > SLAVE_FLAVOR)
>
> I'm tempted to say revert the changes in this file and put back
> ALL_STORES, which would be a list of length one. It will also reduce a
> lot off diff lines in this CP scary branch :)

Sounds good to me, but I'm sure it's still scary for a CP branch.

>
>
>
> > def test_dbusers(self):
> > store_selector = getUtility(IStoreSelector)
> > main_store = store_selector.get(MAIN_STORE, DEFAULT_FLAVOR)
> > self.failUnlessEqual(self.getDBUser(main_store), 'launchpad_main')
> >
> > - auth_store = store_selector.get(AUTH_STORE, DEFAULT_FLAVOR)
> > - self.failUnlessEqual(self.getDBUser(auth_store), 'launchpad_auth')
>
> Well... not all the changes. Explicit references to AUTH_STORE need to
> go, but the ALL_STORES loops seem fine.

The two lines above were actually the only removals that I kept.

> Surprisingly few comments for a large diff. Thanks :)

Thank you for the quick review! Attached is the incremental diff.

--
Guilherme Salgado <email address hidden>