db-torque-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Thomas Fox <Thomas....@seitenbau.net>
Subject Re: BasePeerImpl, was: Re: RFD: RecordMappers, Peers and MapBuilders
Date Mon, 20 Aug 2012 03:27:01 GMT


Thomas Vandahl wrote:

> On 16.08.12 14:26, Thomas Fox wrote:

> > 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.

done.

> > 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.

Already done. I had to touch most of the relevant places anyway.
If you want to inspect the changes, please look for usages of the
Constructor BasePeerImpl().
BasePeer is now a deprecated class.

> > 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.

done.

> > 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 disagree here. If we leave the old criteria class in (see below), the
methods with the old semantics should be there,
as with the other methods using the old criteria object having the old
semantics.
If we remove the old criteria object, obviously the methods will go.
Even if we would remove the methods still there is a semantics change from
3.3 to 4.0, which should be mentioned.
I added the remark for now; the remark can be updated when we have
agreement here.

> > 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.

Good point. I missed that.

> > We may reduce their visibility to protected to avoid misuse.

There are good reasons to use other Record Mapper (joins, partial selects
etc).
I'd leave the visibility as is.

> > 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
>
> Agreed.

done.

> > 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.
>
> Agreed.

Done. This has also lead to removing the table map from the ColumnValues
object.

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

Done.

> > 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.

Fixed in BasePeerImpl. Reverted to CriteriaInterface, pending decision to
remove old Criteria (see below).

> > 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.

This was already discussed back in Dec 2011.
See
http://mail-archives.apache.org/mod_mbox/db-torque-dev/201112.mbox/%3COFC7FA947A.C74689DF-ONC1257963.0067B77A-C1257963.00687CFC%40seitenbau.net%3E

and follow-ups.

Do we want to change this decision now ? I'm open to a new discussion, but
this should be done in a new mail thread. Feel free to open one.

     Thomas


---------------------------------------------------------------------
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