[jifty-devel] Proposed change to SchemaGenerator

[Jesse Vincent]

> 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