db-torque-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Thomas Vandahl ...@apache.org>
Subject Re: BasePeerImpl, was: Re: RFD: RecordMappers, Peers and MapBuilders
Date Thu, 16 Aug 2012 16:55:07 GMT
On 16.08.12 14:26, Thomas Fox wrote:
> First of all, the generification of BasePeerImpl is a good thing as it
> helps reducing the amount of generated code. Thanks a lot, Thomas!
> I have created a jira issue for it (TORQUE-220)

Thanks for that. I still need to get used to that method.

> The best solution for this I can come up with is to move all methods to the
> generated static wrapper and thus only have one instance.

Inheritance with static classes is always a bit of a challenge. Although
your suggestion will again increase the amount of generated code, it's
probably the best solution. At least I don't know any better.

> Also then we should remove all references to BasePeer in the runtime and
> either deprecate the class or totally remove it.
> What do you think ?

As you can see from my "proof of concept" within LargeSelect, the peer
instance objects can be quite useful. It should not be that difficult to
remove these references. I volunteer.

> I am not happy with the getters of TableMap, databaseMap and RecordMapper
> in BasePeerImpl throwing runtime exceptions.

That's just the usual method I use. These exceptions are not expected to
occur in a regular application and mean that you did not initialize the
objects accordingly. The exceptions are thrown to remind you of that
fact. I would not expect a simple getter to throw a regular exception.
That said, I'm fine with changing the exceptions to TorqueException
because I don't see any usage of them outside a database operation.

> We should mention the semantic difference between the doDelete
> (util.Criteria) and doDelete(criteria.Criteria) in the migration guide.
> The doDelete(util.Criteria) and doDelete(util.Criteria, Connection) methods
> guess the table name from the Criteria,
> whereas the doDelete(criteria.Criteria) and doDelete(criteria.Criteria,
> Connection) use the table name from the injected table map.

I'd remove the doDelete(util.Criteria) and doDelete(util.Criteria,
Connection) method from the class. They are bound to fail and we have
working replacements.

> I am wondering whether the doSelect(..., RecordMapper, ...) methods should
> be still template methods (meaning using <TT> instead of <T>).

The doSelectJoinXXX() methods call these with different RecordMappers so
they might return a different data type. We may reduce their visibility
to protected to avoid misuse.

> Also the methods
> doSelectSingleRecord(Criteria, RecordMapper<T> mapper, TableMap)
> and
> doSelectSingleRecord(Criteria, RecordMapper<T> mapper, TableMap,
> Connection)
> are not template methods. At least there should be consistency


> Should there still be methods where a table map can be passed in
> explicitly ?
> In my opinion we can remove those and use the internal table map only.


> The throws clause from addSelectColumns can be removed as no
> TorqueException can occur in the current implementation.


> Is there a hard reason to tie the largeSelect class to the new Criteria
> object and not CriteriaInterface<?>

There was one select case I failed to manage with the old type Criteria.

> I would also add the methods BasePeerImpl.addSelectColumns(util.Criteria)
> and BasePeerImpl.correctBooleans(util.Criteria) for they are necessary when
> working with the old-style criteria
> (setting the generation option torque.om.criteriaClass =
> org.apache.torque.util.Criteria and torque.om.criterionClass =
> org.apache.torque.util.Criteria.Criterion)

I'd like to strongly suggest again that we don't publish Torque 4 with
two different Criteria implementations. That will cause headaches with
both, usage and support, I promise.

Bye, Thomas.

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

View raw message