You are viewing a plain text version of this content. The canonical link for it is here.
Posted to torque-dev@db.apache.org by Thomas Fox <Th...@seitenbau.net> on 2011/05/09 22:02:51 UTC

Torque 4 next steps

I will hopefully have some time the next month to go further towards a
torque 4 release. My next future plan would be the following (detailed
below)
- split peer classes ito normal objects plus static wrappers
- remove remnants from village usage from om classes
- remove the inheritance of criteria from hashtable
- remove Criteria.Criterion.db
For each of these points, I need opinions on how to proceed

- split peer classes ito normal objects plus static wrappers
The idea would be to make all methods non-static and rename the classes to
XXXPeerImpl. Then there would be a static wrapper class named XXXPeer which
can create an instance of  XXXPeerImpl on demand (or it can get an instance
injected). This wrapper class has the same static methods as the current
Peer class but delegates each method call to its XXXPeerImpl instance.
There would be a getter and a setter for the XXXPeerImpl instance.
My question now is where the constants (e.g. for table name and column
names) should go to ? For source compatiblility, they should be in the
static wrapper class. Logically, they should be in the XXXPeerImpl class
because everything should also work without using the static wrappers.
I do not see another solution than keeping the constants in both the
XXXPeer and XXXPeerImpl classes, but maybe I have missed something ?

- remove remnants from village usage from om classes
This would mean removing  the getOMClass() method, the CLASS_DEFAULT
constant and the initClass() method in the generated peers. They were
needed to create instances of the database object classes but this is not
necessary any more with the mapper classes. Any objections here ?

- remove the inheritance of criteria from hashtable
The reasons for suggesting this are:
- Criteria is not a logical specialisation of a Hash map.
- To retain current signatures and comply with java5 generics, Criteria
would have to be declared as HashMap<String, Object>, while technically it
is a HashMap<String, Criteria.Criterion>
- the current "put" method does not follow the contract of a Map's put
method:
  Criteria.put() returns the Criteria itself, whereas Map.put() returns the
value which previously had the passed key.
  Also, if you put() an object into a criteria and then get() the same key,
you do not receive the object you put() but a Criteria.Criterion containing
the object you put in. This is also not the behaviour expected by a map.

If we remove the inheritance from Hashtable, we might want to consider the
semantics of the Criteria.add() methods. Currently, if one adds a column
twice, the first assignment is overwritten by the second. For example:
Criteria criteria = new Criteria();
criteria.add(AuthorPeer.AUTHOR_ID, 100, Criteria.GREATER_THAN);
criteria.add(AuthorPeer.AUTHOR_ID, 200, Criteria.LESS_THAN);
ends up in the SQL
select ... from author where author.author_id < 200;
which is not at all what one would expect by reading the code
(I would expect select ... from author where author.author_id > 100 and
author.author_id < 200).

I see the following possiblilities
1) leave the semantics as it is
  1a) possibly deprecating the add() methods and encourage users to use and
() instead
2) change the semantics such that add() does not override existing keys
   (i.e. they would do the same as the and() methods)
  2a) as 2) but additionally deprecating the add() methods
3) remove the add() method altogether

I am not sure what to do. In a clean room implementation one would want to
go for 2) or 3) but 3) severely breaks current applications and 2) may
introduce
bugs in applications depending on the current behaviour. 1) is the least
pain for existing users but the maximum pain for new users. 1b) might be a
good alternative, maybe with the possibility to remove the add methods
later.
Also, we might either deprecate or remove the put, putAll and get methods,
as they do not make sense for a criteria.
What do you think ?

- remove Criteria.Criterion.db
The db field of the Criterion specifies which database dialect is used for
rendering this Criterion in SQL. As a criterion is no standalone class, but
an inner class of a Criteria, it always has a criteria attached and its SQL
is only rendered in the context of this criteria. As the criteria has a
field dbName by which the db adapter can be found out via Torque.getDB
(criteria.getDbName()), I propose to remove the db field from Criterion
together with its getters and setters and use the dbName field
of the criteria instead to determine which Adapter should be used. Any
objections ?

   Thanks for your opinions,

       Thomas


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


Re: Torque 4 next steps

Posted by Thomas Vandahl <tv...@apache.org>.
On 17.05.11 21:55, Thomas Fox wrote:
> Is this your only objection against removing the method or do you think
> this method should stay anyway ?

Yes, my only objection. Thanks for considering this. Go ahead.

Bye, Thomas.

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


Re: Torque 4 next steps

Posted by Thomas Fox <Th...@seitenbau.net>.
> > On 09.05.11 22:02, Thomas Fox wrote:
> > > - remove remnants from village usage from om classes
> > > This would mean removing  the getOMClass() method, the CLASS_DEFAULT
> > > constant and the initClass() method in the generated peers. They were
> > > needed to create instances of the database object classes but this is
> not
> > > necessary any more with the mapper classes. Any objections here ?
> >
> > Sorry for not replying earlier, but yes. The getOMClass() method is
used
> > for the inheritance feature of Torque which I happen to use. How would
> > that work with the mapper classes?
>
> The idea is that the record mapper class looks into the inheritance key
> column and knows the mapping from keys to classes to instantiate.
> But looking at the generated code (
> org.apache.torque.test.BaseInheritanceClassnameTestRecordMapper in the
test
> project), it seems that the mapping feature is currently not working, No
> mapping takes place but the inheritance key column is taken to contain
the
> om class name directly.
> Obviously, also a test case is missing because junit tests are green at
the
> moment.
> I'll look into it.

I was mistaken. The inheritance with keys works, and a test case exists in
the test project. I looked at the wrong class (should have looked at
org.apache.torque.test.BaseInheritanceTestRecordMapper instead).
Inheritance with classname can be improved to work out of the box, I'll do
that and add a test case.

      Thomas


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


Re: Torque 4 next steps

Posted by Thomas Fox <Th...@seitenbau.net>.
> On 09.05.11 22:02, Thomas Fox wrote:
> > - remove remnants from village usage from om classes
> > This would mean removing  the getOMClass() method, the CLASS_DEFAULT
> > constant and the initClass() method in the generated peers. They were
> > needed to create instances of the database object classes but this is
not
> > necessary any more with the mapper classes. Any objections here ?
>
> Sorry for not replying earlier, but yes. The getOMClass() method is used
> for the inheritance feature of Torque which I happen to use. How would
> that work with the mapper classes?

The idea is that the record mapper class looks into the inheritance key
column and knows the mapping from keys to classes to instantiate.
But looking at the generated code (
org.apache.torque.test.BaseInheritanceClassnameTestRecordMapper in the test
project), it seems that the mapping feature is currently not working, No
mapping takes place but the inheritance key column is taken to contain the
om class name directly.
Obviously, also a test case is missing because junit tests are green at the
moment.
I'll look into it.

Is this your only objection against removing the method or do you think
this method should stay anyway ?

     Thomas


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


Re: Torque 4 next steps

Posted by Thomas Vandahl <tv...@apache.org>.
On 09.05.11 22:02, Thomas Fox wrote:
> - remove remnants from village usage from om classes
> This would mean removing  the getOMClass() method, the CLASS_DEFAULT
> constant and the initClass() method in the generated peers. They were
> needed to create instances of the database object classes but this is not
> necessary any more with the mapper classes. Any objections here ?

Sorry for not replying earlier, but yes. The getOMClass() method is used
for the inheritance feature of Torque which I happen to use. How would
that work with the mapper classes?

Bye, Thomas.

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


Torque 4 next steps status

Posted by Thomas Fox <Th...@seitenbau.net>.
> I will hopefully have some time the next month to go further towards a
> torque 4 release. My next future plan would be the following (detailed
> below)
> - split peer classes into normal objects plus static wrappers

Done. Please look at the solution and if you have questions or objections
please raise them.

> - remove remnants from village usage from om classes

To be done.

> - remove the inheritance of criteria from hashtable

Done. As nobody entered the discussion I did the minimal possible change,
i.e. leave the behaviour of the add methods as they are. Again, please
comment if there are any questions or issues.

> - remove Criteria.Criterion.db

To be done.

I created a Jira issue for each of the completed tasks so we can creeate
the 4.0 change list from jira.

    Thomas


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


Re: Torque 4 next steps

Posted by Thomas Vandahl <tv...@apache.org>.
Hi Thomas,

sorry again for not acting earlier. As always: time constraints.

On 09.05.11 22:02, Thomas Fox wrote:
> If we remove the inheritance from Hashtable, we might want to consider the
> semantics of the Criteria.add() methods. Currently, if one adds a column
> twice, the first assignment is overwritten by the second. For example:
> Criteria criteria = new Criteria();
> criteria.add(AuthorPeer.AUTHOR_ID, 100, Criteria.GREATER_THAN);
> criteria.add(AuthorPeer.AUTHOR_ID, 200, Criteria.LESS_THAN);
> ends up in the SQL
> select ... from author where author.author_id < 200;
> which is not at all what one would expect by reading the code
> (I would expect select ... from author where author.author_id > 100 and
> author.author_id < 200).

If you write something like

criteria.add(AuthorPeer.AUTHOR_ID, 100, Criteria.GREATER_THAN);
criteria.or(AuthorPeer.NAME, "Hemingway");

you end up with a "select ... where author.author_id > 100 *and*
autor.name='Hemingway'"

> 2) change the semantics such that add() does not override existing keys
>    (i.e. they would do the same as the and() methods)
>   2a) as 2) but additionally deprecating the add() methods
+1 for 2a

However how to handle my example in the general case. I have no good
idea how to solve this.

Bye, Thomas.

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


RE: Torque 4 next steps

Posted by Thomas Fox <Th...@seitenbau.net>.
> > - split peer classes ito normal objects plus static wrappers
> > The idea would be to make all methods non-static and rename the classes
> to
> > XXXPeerImpl. Then there would be a static wrapper class named XXXPeer
> which
> > can create an instance of  XXXPeerImpl on demand (or it can get an
> instance
> > injected). This wrapper class has the same static methods as the
current
> > Peer class but delegates each method call to its XXXPeerImpl instance.
> > There would be a getter and a setter for the XXXPeerImpl instance.
> > My question now is where the constants (e.g. for table name and column
> > names) should go to ? For source compatiblility, they should be in the
> > static wrapper class. Logically, they should be in the XXXPeerImpl
class
> > because everything should also work without using the static wrappers.
> > I do not see another solution than keeping the constants in both the
> > XXXPeer and XXXPeerImpl classes, but maybe I have missed something ?
>
> After more thought and a test implemetation, the PeerImpl classes need to
> have access to the other peer classes (e.g. for the doSelectJoinYYY
> methods). The easiest way to achieve this is via the other Peer's static
> wrappers (the other possibility would be an external dependency injection
> framework, which is not overwise needed and too heavyweight for our
> problem).
> So my new insight is that the implementations cannot exist without the
> static wrappers, and in this case it would make no sense to duplicate the
> static constants in the Peer and PeerImpl classes, they would stay in the
> Peer (static wrapper).
>
> Then, another question arose, and this is where the RecordMapper classes
> (which are responsible for converting resultSets into DB Objects) should
> reside. In the first shot when replacing village, they were made inner
> static classes of the Peer classes. Now the question is should they be
> inner classes of the Peer or the PeerImpl class, or should they be
defined
> in their own class. As there is no reason to make them inner classes of
> either Peer or PeerImpl, my opinion is they should best be defined
outside
> the Peers and in their own class files.
>
> Are there objections against committing a discussion basis to trunk or
> should I create a branch for it ?

The division between static wrappers and the implementation in Peers is now
in SVN. It should now be possible to exchange Peer implemetations by using
the setter in the respective Peer class.
If you sislike something, please email.

    Thomas


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


RE: Torque 4 next steps

Posted by Thomas Fox <Th...@seitenbau.net>.
> - split peer classes ito normal objects plus static wrappers
> The idea would be to make all methods non-static and rename the classes
to
> XXXPeerImpl. Then there would be a static wrapper class named XXXPeer
which
> can create an instance of  XXXPeerImpl on demand (or it can get an
instance
> injected). This wrapper class has the same static methods as the current
> Peer class but delegates each method call to its XXXPeerImpl instance.
> There would be a getter and a setter for the XXXPeerImpl instance.
> My question now is where the constants (e.g. for table name and column
> names) should go to ? For source compatiblility, they should be in the
> static wrapper class. Logically, they should be in the XXXPeerImpl class
> because everything should also work without using the static wrappers.
> I do not see another solution than keeping the constants in both the
> XXXPeer and XXXPeerImpl classes, but maybe I have missed something ?

After more thought and a test implemetation, the PeerImpl classes need to
have access to the other peer classes (e.g. for the doSelectJoinYYY
methods). The easiest way to achieve this is via the other Peer's static
wrappers (the other possibility would be an external dependency injection
framework, which is not overwise needed and too heavyweight for our
problem).
So my new insight is that the implementations cannot exist without the
static wrappers, and in this case it would make no sense to duplicate the
static constants in the Peer and PeerImpl classes, they would stay in the
Peer (static wrapper).

Then, another question arose, and this is where the RecordMapper classes
(which are responsible for converting resultSets into DB Objects) should
reside. In the first shot when replacing village, they were made inner
static classes of the Peer classes. Now the question is should they be
inner classes of the Peer or the PeerImpl class, or should they be defined
in their own class. As there is no reason to make them inner classes of
either Peer or PeerImpl, my opinion is they should best be defined outside
the Peers and in their own class files.

Are there objections against committing a discussion basis to trunk or
should I create a branch for it ?

     Thomas


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