[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