db-torque-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "CG Monroe (JIRA)" <j...@apache.org>
Subject [jira] Commented: (TORQUE-22) Enhanced Database Mapping info
Date Thu, 25 May 2006 21:39:30 GMT
    [ http://issues.apache.org/jira/browse/TORQUE-22?page=comments#action_12413310 ] 

CG Monroe commented on TORQUE-22:
---------------------------------

> In my opinion, the patch should be added to Torque. The one general thing 
> I'm not convinced about is the initialize() method in the DatabaseMap. It 
> creates a link between the Torque runtime and the generated classes (which 
> is hidden by using reflection). Why not call the XXXMapInit.init() class 
> directly ? If the name of the database is not known at compile time, the 
> user may still use reflection to access it. 

IMHO, the real issue here is how to support the creation of generic code 
that can be used with anyone's else's generated OM classes.  For example, 
XML import/export ;) or a webapp like pHpMyAdmin (web admin for MySQL 
servers) that anyone could drop their generated Torque OM classes into and 
then have screens generated using TableMap info that let them walk thru the 
schema and data (even add/delete records as needed).  

In this type of situation, the developer of the generic webapp will not 
know what package the Torque OM classes will reside in.  AFAIK, there 
is no reliable/ quick way to find XXXMapInit class via Reflection unless 
you know the package it's in.

> Also, I came across a number of smallish issues:

> Code style: 
> - Please add imports after the licence comments (e.g. ColumnMap).
> - Please use a separate line for opening and closing braces.
> - Please respect the 80 columns maximum width of java files (e.g. 
>   InheritanceMap)

Column.java - OK
Database.java - OK
ColumnMap.java - Done
more- TODO: 

> ColumnMap.java
> - Could you please add the comments to the private fields as javadocs? 
>   (also TableMap.java)

ColumnMap.java - Done

> - Why do you need the useInheritance mapping ? should not rather the 
>   inheritanceType field be used to retrieve that information ?

Mainly because I was working from the Column.java class in the Generator 
code to see what attributes were parsed out of the XML and then making a 
quick decision as to what seemed useful.  I'm not tied to this, but having 
a boolean that you can quickly test to see if you need to dive deeper into 
the structure seems much more effecient than having to do a bunch of string 
compares.  E.g., we support multiple inheritance and you need to look at 
100 columns for which ones apply.  

Plus it's protection against the DTD values changing from false to none or 
some other string.  This change only needs to be dealt with at the 
Generator code level

> - This may be a question of style, but I'd prefer the inheritanceMaps be 
>   initialized when it is defined, not in the constructor, because all the 
>   other fields are also initialized where they are defined.

- Done

> - If there is no strong reason for doing otherwise, I'd find it more 
>   intuitive if the field in the database map would be named like the 
>   corresponding shema-xml attribute (inheritanceType-> inheritance, boolean 
>   usePrimitive -> String javaType). Although the javaType has just two  
>   values now, maybe in the future there might be more. 

Currently, the names match the field names used in the Generator code.  I 
did that just to make changes in restructuring the MapBuilder template 
easier and less confusing for me, e.g. setJavaType( ${col.JavaType} ).  But 
now that the bulk coding is done and it's less confusing, this sounds like 
a more logical point of veiw, since end users will be relating XML to this 
class. 

- TODO 

> - setInheritance(). If this method is retained (see above), why does it 
>   set the table's inheritance ? if this is necessary, it should at least be 
>   documented in the javadocs.

See next comment set below

> TableMap.java
> - Is the useInheritance field needed ? One could also ask the columns if 
>   their inheritance map is empty. I do not like to keep the same information 
>   in different places, becasue then synchronizing the two is always an issue. 
>   (one might forget in later code changes that if one field is changed, the 
>   other needs to be changed also)

IMHO, Inheritance actually resides at two levels.  It is actually 
implimented at the table level via the record subclasses that Torque 
generates but the mapping of subclass to records is done at a column level. 
So asking if a Table uses inheritance is a logical thing.  In addition, 
it's a lot more efficient to be able to check at the Table level for 
special processing needs.  As opposed to checking all columns of all tables 
for inheritance everytime you do something.

That said, this flag is set from below because that's were inheritance is 
defined. Maybe the change should be an inheritance attribute to the table 
level where IMHO, it really belongs, then define the specific column(s)
relating to it.

Re: The linkage problem from above... I will improve the JavaDocs and
add a check to verify this is set at the table level in the test suite.

- Done

> - why do you add the deprecated method addColumn(String name, Object 
>   type, boolean pk, String fkTable, String fkColumn, int size, int scale, 
>   String javaName) ?

Just a strand of DNA from a failed evolutionary turn... I'll splice it out.

- Done

> Peer.vm:
> - The call of getMapBuilder() in getTableMap() should not be necessary if 
>   Torque is initialized. Why did you add it ? Does it work if Torque is not 
>   initialized ?

In trying to retrace my logic, I think I saw some possible conditions where 
this might be called and a TableMap would not have been previously loaded 
(in the standard lazy initialization mode).  Since this method was being 
changed from protected to public and the conditions in which it was called 
would be more variable, it seemed like a nice safety blanket to make sure 
the TableMap was added to the DatabaseMap.  That way, you should always get 
a TableMap object and not a null.

FWIW, I think the original coding would fail if Torque was not initialized.

> MapBuilder.vm
> - Why is the initClass method needed ? Better not use reflection : 
>   ${className}.class is the thing you want.

Now that I look at it, I'm not sure... I think it was done early in my
template writing learning curve and due to using Monkey See/ Monkey Do
coding from the way the Peer.vm template got the OM class and then 
generalizing this. 

- TODO

> - I'd rather remove the PEER_CLASS, OM_CLASS and MANAGER_CLASS class 
>   variables. The values are needed only once and can be computed in local 
>   variables.

I agree.. 

-TODO

> - Why do you use ($useManagers && $table.PrimaryKey.size() > 0) instead 
>   of ($useManagers) ? (also in Control.vm) 

In order to get the Manager class name, I needed to do the same logic that 
the Control.vm does to make the filename used on the parser call.  That's 
the way Control.vm does it (repository head version).  Since I don't use 
managers, I assumed that someone with more knowledge than me had a reason 
for this. Do managers only work with tables that have primary keys?

> - Why do you generate the method getColumnName( String name ) for each 
>   class instead of putting the method in the base class ? Also, I'd rather 
>   rename this method to normalizeColumnName() or similar.

Just some quick and dirty code translation from the addColumn(<many params>)
code (which is why it's protected..).  Reviewing it, it seems like a 
moving this to ColumnMap as:

  protected String normalizeName( String Column )..

and calling it from the constructor to normalize the column name would work
the same.

- TODO:



> Enhanced Database Mapping info
> ------------------------------
>
>          Key: TORQUE-22
>          URL: http://issues.apache.org/jira/browse/TORQUE-22
>      Project: Torque
>         Type: Improvement

>   Components: Runtime, Generator, Test Project
>     Versions: 3.2.1
>     Reporter: CG Monroe
>      Fix For: 3.2.1
>  Attachments: MapEnhancements-ChangeLog.txt, MapEnhancements-ObjectWithManager.zip, MapEnhancements.zip
>
> The attached zip file makes the Database Map information much more robust and useful.
 Basic features added:
> If an application needs a fully populated  Database Map structures, this can be done
via the DatabaseMap.initialize() method.
> *Map classes will now preserve XML order and store many more XML attritbutes, including
Inheritance information.
> MapBuilder template re-done to be more readable and expandable.
> The TableMap object can be used to get the associated Peer, OM, and Manager (if applicable)
Class objects. 
> Access to TableMap structures made easier in Peer ( existing method changed to public)
and OM classes. (The theory here is that TableMap should know all this info and there should
be an easy way to get there).
> Test-project now has DatabaseMap test suite that tests most of these features.
> Some misc Javadoc/synchronizing changes.
> Has been tested with aliased tables, beans, managers, DB schema's with different packages
during development (test suite doesn't test all of these combinations yet..)

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators:
   http://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see:
   http://www.atlassian.com/software/jira


---------------------------------------------------------------------
To unsubscribe, e-mail: torque-dev-unsubscribe@db.apache.org
For additional commands, e-mail: torque-dev-help@db.apache.org


Mime
View raw message