[jifty-devel] Proposed change to SchemaGenerator

Fri Feb 23 12:31:21 EST 2007

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
 * Jifty::Everything loads Jifty::Model::Metadata
 * 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