[jifty-devel] Proposed change to SchemaGenerator

Jesse Vincent jesse at bestpractical.com
Fri Feb 23 13:05:31 EST 2007




On Fri, Feb 23, 2007 at 11:31:21AM -0600, Andrew Sterling Hanenkamp wrote:
> Wow. I tracked it down, at least for tests using Jifty::Test:
> 
>  * Jifty::Test loads Jifty::Config
>  * Jifty::Config loads Jifty::Util
>  * Jifty::Util loads Jifty
>  * Jifty loads Jifty::Everything
>  * Jifty::Everything loads Jifty::Record

Since all our model classes use a base of Jifty::Record, I bet this can
be eliminated.

>  * Jifty::Everything loads Jifty::Model::Metadata

I bet that this step can be deferred until we actually want to look at
the metadata model class.

>  * FAIL
> 
> Jifty::Model::Metadata calls schema { ... } to define its two columns.
> This leads to a call of _init_methods_for_column for the first column,
> which ends up caling Jifty::Record::schema_version, which needs
> Jifty::Config::new, but Jifty::Config new hasn't finished loading yet.
> Kaboom.
> 
> I can make all tests succeed by redefining schema_version in
> J::M::Metadata and J::M::Session as long as I define that method BEFORE
> the call to schema { ... } (otherwise it's not loaded and reverts to the
> Jifty::Record one which doesn't work until Jifty::Config is loaded and
> returns the wrong information anyway). I also have to make sure that
> $Jifty::VERSION is initialized in a BEGIN block to give it a value in
> time.
> 
> Thus, I've gotten it to work without changing the bootsrap order, but
> it's a little clunky.
> 
> --
> Andrew Sterling Hanenkamp
> Interaction Developer
> Boomer Consulting, Inc.
>  
> 1.785.537.2358 ext. 17
> 1.888.266.6375 ext. 17
> 1.785.537.4545 (fax)
>  
> 610 Humboldt
> Manhattan, KS 66502
>  
> http://www.boomer.com/about/team/andrew-hanenkamp.html
> andrew.hanenkamp at boomer.com
> 
> -----Original Message-----
> From: jifty-devel-bounces at lists.jifty.org
> [mailto:jifty-devel-bounces at lists.jifty.org] On Behalf Of Andrew
> Sterling Hanenkamp
> Sent: Friday, February 23, 2007 10:44 AM
> To: Nifty apps in a Jiffy
> Subject: RE: [jifty-devel] Proposed change to SchemaGenerator
> 
> > I'm missing it. Why does loading the config file need the schema 
> > declarations?
> 
> You're right it shouldn't, but there seems to be a dependency loop
> somewhere at BEGIN time. I thought I knew where it was, but I'm not sure
> now so I've reverted that part of the changes to see if I can trace it
> and give a better report on it. 
> 
> I do know that when I have to load Jifty::Config in the call to schema {
> ... }, nearly all tests in Jifty fail with:
> 
> Can't locate object method "new" via package "Jifty::Config" at
> /Users/andrew/Documents/code/perl/jifty-trunk/blib/lib/Jifty/Record.pm
> line 497, <DATA> line 16.
> 
> > This makes me somewhat twitchy. I think I'd rather a more explicit 
> > method that initializes all accessors and mutators.
> 
> Agreed, though I'd prefer to say it gives me the jibblies. Scenarios
> come to mind where one might want to run can(), which could fail unless
> this is done well. Which is why I created _init_methods_for_columns for
> that purpose. This is documented as an "internal method", but this
> should probably be changed to a public method to be called when needed.
> That is, assuming this is the way to go on this. It might not be a bad
> idea to have anyway, if we want to allow on-the-fly schema changes, as
> is the case with the virtual-models branch.
> 
> However, I think this explicit method might be called implicitly at some
> point. I'm still thinking of good places to put it, assuming this is the
> right route to follow.
> 
> > One minor request: Could you commit your perltidying separately? 
> > (Doing it first is fine.)  That will help make what you're actually 
> > changing easier to read.
> 
> Done.
> 
> I've also renamed app_version to schema_version in my working copy.
> 
> I'm of the opinion that trying to force the configuration to load too
> early might be difficult for a couple reasons. The biggest is that if
> someone wants to use JDBI in some other application, I can see forcing
> them to load their configuration too early being a frustrating snag.
> Loading the configuration during BEGIN for a model might be too early in
> some cases in Jifty as well.
> 
> At this point, I'm trying to track down the Jifty::Config load problems.
> I'm also seeing if I can find a way to run _init_methods_for_columns
> just a little bit later, but earlier than my patch currently offers.
> 
> --
> Andrew Sterling Hanenkamp
> Interaction Developer
> Boomer Consulting, Inc.
>  
> 1.785.537.2358 ext. 17
> 1.888.266.6375 ext. 17
> 1.785.537.4545 (fax)
>  
> 610 Humboldt
> Manhattan, KS 66502
>  
> http://www.boomer.com/about/team/andrew-hanenkamp.html
> andrew.hanenkamp at boomer.com
> 
> -----Original Message-----
> From: jifty-devel-bounces at lists.jifty.org
> [mailto:jifty-devel-bounces at lists.jifty.org] On Behalf Of Jesse Vincent
> Sent: Thursday, February 22, 2007 5:37 PM
> To: Nifty apps in a Jiffy
> Cc: audreyt at audreyt.org
> Subject: Re: [jifty-devel] Proposed change to SchemaGenerator
> 
> 
> 
> 
> > Since much of the magic behind how columns are built and the tables 
> > are deployed happens in JDBI, it seems that JDBI needs at least a 
> > rudimentary understanding of versions. I think the concept of what the
> 
> > database/schema version is should be left up to the implementation, 
> > but some of the actual handling almost has to be moved into JDBI to 
> > some extent (and already has with the introduction of "till" and
> "since").
> 
> I'm ok with that. 100% ok with it.
> 
> > 
> > The solution I've ended up with is not an app_version in 
> > SchemaGenerator, but an app_version that may be implemented in by a 
> > subclass of JDBI::Record. I can make the change to "schema_version", I
> 
> > just picked "app_version" because the docs in JDBI use the term 
> > "application version", but it should probably be either "schema
> version"
> > or "database version". That's an easy change.
> 
> I think that we want to keep track of two different things:
> 
>  * version of the schema in our running code.
>  * version of the schema in our database.  
> 
>  I'm open to good naming.
> 
> > My latest changes allow both the creation of accessor/mutator methods 
> > and the deployment of the schema to be based upon the notion of the 
> > database version that the implementation wishes to express. For 
> > example, I've made it so that Jifty passes back the
> > framework->Database->Version number via the Jifty::Record
> implementation of app_version.
> 
> 
> *nod*
> 
> > To make this work with Jifty, I did change when method initialization 
> > takes place. The creation of custom accessors and mutators (i.e., the 
> > COLUMN and set_COLUMN methods) were generated at compile time while 
> > declaring the schema { ... }. Under the new regime, this would happen 
> > later during the very first call to JDBI::Record->new. Without this 
> > change I was running into a chicken-egg problem because schema 
> > declaration needed to know the version, which needed the 
> > configuration, which needed the schema declarations.
> 
> I'm missing it. Why does loading the config file need the schema
> declarations?
> 
> > An alternative would be to delay
> > model loading until after Jifty::Config has been established, but this
> 
> > was a simpler solution for now.
> 
> This makes me somewhat twitchy. I think I'd rather a more explicit
> method that initializes all accessors and mutators.
> 
> > Overall, the patches make this work in all circumstances I've tested 
> > so
> > far:
> > 
> > in App::Model::Foo:
> > 
> > column foo => type is 'text', till '0.2.5'; column foo2 => type is 
> > 'text', since '0.2.5';
> > 
> > in App::Upgrade:
> > 
> > since '0.2.5' => sub {
> >   rename table => 'App::Model::Foo',
> >          column => 'foo',
> >          to => 'foo2';
> > };
> > 
> > I've attached patches to both jifty/trunk and Jifty-DBI/trunk. I don't
> 
> > really feel comfortable committing a change like this back to the 
> > trunk without having other eyes look at it first and tell me it's a 
> > good idea to commit.
> 
> I'd really like Audrey's eyes on this one.  I hugely appreciated the
> work you're doing. But getting it wrong, you're right, could be scary.
> 
> One minor request: Could you commit your perltidying separately? (Doing
> it first is fine.)  That will help make what you're actually changing
> easier to read.
> 
> 
> > I have added tests and all tests pass, but it's still a decent piece 
> > of surgery. I was thinking of applying the changes to virtual-models, 
> > though, since that's where must of my work is and is
> > (slightly) related. I'm not really comfortable doing that without an 
> > okay either.</Hedging>
> 
> I think I'd rather see this on trunk. Pushing too much change that's not
> directly related down into a branch just makes merging that much more
> painful later ;)
> 
> Jesse
> 
> _______________________________________________
> jifty-devel mailing list
> jifty-devel at lists.jifty.org
> http://lists.jifty.org/cgi-bin/mailman/listinfo/jifty-devel
> _______________________________________________
> jifty-devel mailing list
> jifty-devel at lists.jifty.org
> http://lists.jifty.org/cgi-bin/mailman/listinfo/jifty-devel
> _______________________________________________
> jifty-devel mailing list
> jifty-devel at lists.jifty.org
> http://lists.jifty.org/cgi-bin/mailman/listinfo/jifty-devel
> 

-- 


More information about the jifty-devel mailing list