[Dbix-class] Patch for SQL::Translator::Parser::DBIx::Class

[Matt S Trout]

Sebastian Willert wrote:
> On Sun, 2006-06-25 at 20:39 +0100, Matt S Trout wrote:
>> Sebastian Willert wrote:
>>> Hi all,
>>>
>>> after some late-night discussion with castaway about
>>> DBIC::Schema->deploy() we noticed that
>>> SQL::Translator::Parser::DBIx::Class tries to query the connected
>>> database. This shouldn't be done IMO because this might change the info
>>> provided in the schema and might not be stable between different
>>> underlying RDBMs. 
>>>
>>> This are the reasons why I've produced the patch below, the changes
>>> should be self-explainatory. The patch has been tested on the
>>> command-line, using the programmatic interface of SQLT and using
>>> DBIC::Schema->deploy() but could need some additional testing with more
>>> complex schema then I have readily available.
>> The changes are clearly harmful. A schema class simply acts as a prototype for 
>>   objects of that class, it is not guaranteed to contain all the requisite data.
> 
> That depends on the intention of SQLT::Parser::DBIC, i think. If the
> intention is to dump a running schema instance, then you are perfectly
> correct. But if the intention is to build databases according to the
> hard-coded info (i.e. the prototype) in the schema then relying on
> run-time info gathered from a database is harmful IMO.

You're misunderstanding the nature of schema class versus schema instance, 
though. There's no reason whatsoever why somebody might not construct a schema 
object out of resultsources themselves for whatever reason - the class-based 
setup normally used is strictly convenience syntax, not anything key to the 
nature of the system. Hence DBIx::Class::DB simply creates a 
DBIx::Class::Schema object and stores it as class data on the base class, 
registering sources against the object as the classes are loaded. While DB is 
deprecated, the use of schema objects this way is perfectly allowed, and your 
patch would break that (since it would require the sources to have been 
registered against the DBIx::Class::Schema class itself).

 > I was basically interested in automatic test database creation for RDBMs
 > agnostic apps and in this setting it is sensible to not rely on any info
 > gathered from a specific DB, so I naively assumed the second intention.

Being able to avoid getting run-time-loaded data is something that may be 
important on occasion; I'm not disagreeing with this, I'm simply pointing out 
that your patch doesn't actually help (since a connection can be set in-place 
on the class should you so desire) and potentially breaks other code.

 > If I was wrong, I'll happily retract my patch, though.

I'm not sure I see the point; if you don't want run-time connected info, don't 
hand SQLT a connected schema. A documentation patch pointing this out would 
seem far more useful to me.

-- 
      Matt S Trout       Offering custom development, consultancy and support
   Technical Director    contracts for Catalyst, DBIx::Class and BAST. Contact
Shadowcat Systems Ltd.  mst (at) shadowcatsystems.co.uk for more information

+ Help us build a better perl ORM: http://dbix-class.shadowcatsystems.co.uk/ +