Well, the connection is being managed by spring transactional resource
management classes. A transaction is demarcated with
@Transactional(readOnly=true) on a particular method. All
transactional resources are bound to thread during transaction
execution and released when transaction finishes. One transactional
resource is of course a connection. I have extended this framework to
register temporary table names which are being automatically deleted
on transaction finish.
Each time you invoke a transactional method you perform following steps:
- acquire a connection from the pool
- bind connection to thread making all subsequent queries use same connection
- start transaction
- do some work
- finish transaction (of course for read only there is nothing to be done)
- release transactional resources
- unbind connection from thread
- release connection to the pool.
using spring framework without spring managed transactions means you
end up using different connection for EVERY sql query.
My IDataProviders usually wrap spring bean with
@Transactional(readOnly=true) methods.
Unfortunatelly two method calls: size() and iterator() mean two
different transaction and two different connections. I am unable to
wrap any single point of entry with @Transactional - that probably
would have to be some generic Wicket method call. I cannot bind a
connection to current request thread preemptively because that would
mean a huge resource hog - I would have to obtain a connection from
pool for every request being made.
Temporary tables have connection scope so not deleting the table on
connection release means polluting the database server with lots of
temporary tables as long as connection stays in pool (which could be
indefinite).
The temporary tables would be probably stale all the time - any
ongoing transaction may probably alter what the temporary table should
contain.
Anyway any approach still feels like a hack and not resolving the
problem at source.
A single point of entry in IDataProvider:
- allows the user to manage resources more efficiently.
- gives user the option to resign from providing size() information -
do you really go from 1st to 15th page that often or do you usually
click "next results"?
- gives user the option to provide "has more results" information
without providing actual dataset size.
Having a single point of entry IDataSource means also you can do:
new IDataProviderSingleEntry<T( oldDataProvider ) {
public Page<T> getPage( long first, long count ) {
return new PageImpl<T>( oldDataProvider.interator( first, count
), oldDataProvider.size() );
}
}
This trivial decorator means IDataProviderSingleEntry and
IDataProvider may even coexist in wicket-core and wicket-extensions.
Some variation of this approach is actually used by inmethod-grid.
Maybe we could unify the way all pageables access resources.
On Tue, May 6, 2014 at 8:06 PM, tetsuo <[email protected]> wrote:
> Since it's the same request, isn't (or couldn't be) the db connection the
> same in size() and iterator() calls? If you created the temporary table in
> one, isn't it still available to the other (so you could avoid recreating
> it)?
>
> “The truth is rarely pure and never simple.” ― Oscar Wilde
>
>
> On Tue, May 6, 2014 at 11:15 AM, Leszek Gawron <[email protected]> wrote:
>
>> hello,
>>
>> I would like to propose a major change in IDataProvider so I thougt
>> this topic is better suited for dev list.
>>
>> Currently IDataProvider looks like this:
>>
>> public interface IDataProvider<T> extends IDetachable
>> {
>> Iterator<? extends T> iterator(long first, long count);
>> long size();
>> IModel<T> model(T object);
>> }
>>
>> this looks OK but is actuall suboptimal in some cases.
>>
>>
>> It would be a lot better (at least for me :)) ) if IDataProvider
>> looked somewhat like:
>>
>> public interface IDataProvider<T> extends IDetachable
>> {
>> Page<T> getPage(long first, long count);
>> IModel<T> model(T object);
>> }
>>
>> or similarly to IDataSource from inmethod-grid:
>>
>> public interface IDataSource<T> extends IDetachable, IClusterable
>> {
>>
>> public void query(IQuery query, IQueryResult<T> result);
>>
>> public IModel<T> model(T object);
>>
>> public interface IQuery
>> {
>> public long getFrom();
>> public long getCount();
>>
>> public static final long UNKNOWN_TOTAL_COUNT = -1L;
>>
>> public long getTotalCount();
>>
>> public <S> IGridSortState<S> getSortState();
>> }
>>
>> public interface IQueryResult<T>
>> {
>> public static final int MORE_ITEMS = -1;
>> public static final int NO_MORE_ITEMS = -2;
>>
>> public void setTotalCount(long count);
>> public void setItems(Iterator<? extends T> items);
>> };
>> }
>>
>>
>> Following arguments prove that two latter proposals are better than
>> current IDataProvider:
>>
>> 1. You have your data "page" generated by one method, not two.
>>
>> Something like "show me a DataTable of contractors that either me or
>> any of my subordinates (tree) has access to" is impossible without
>> using temporary tables.
>> With a temporary table it is as simple as: select * from contractor c
>> inner join #temporary_user_contractors_88 t on c.id = t.id
>>
>> Temporary tables are usually quite costly to build (in the example you
>> execute as many queries as you have nodes in subordinate tree), they
>> also required proper cleanup before the connection is released back to
>> the pool.
>> Having two methods on IDataProvider means you have to:
>>
>> - invoke dataProvider.size()
>> - create a temporary table
>> - query for count
>> - release a temporary table
>> - invoke dataProvider.iterator(first,count)
>> - create a temporary table
>> - query for items
>> - release temporary table
>>
>> being able to compute both data in one pass saves a lot of unnecessary
>> queries being made.
>>
>> 2. Usually select for items and select for count(*) differ only with
>> columns fetched:
>>
>> select * from some_joined_tables where condition1 and condition2 and
>> condition3;
>> select count(*) from some_joined_tables where condition1 and
>> condition2 and condition3;
>>
>> What users usually do is create some template methods for filling in
>> the same conditions in both cases.
>> Having one method would make the code clearer:
>>
>> @Override
>> @Transactional(readOnly = true)
>> public Page<T> list( Pageable pageable, F filter, Map<String,
>> Object> context ) {
>> ValueListQueryConfig<F> queryConfig = getQueryConfig( pageable,
>> filter,
>> context );
>> queryConfig.setDialect( dialect );
>> Select<Record> resultsQuery = queryConfig.generateResultsQuery();
>> List<T> results = getJdbcTemplate().query(
>> resultsQuery.getSQL(),
>>
>> resultsQuery.getBindValues().toArray(),
>> getRowMapper() );
>>
>> Select<Record> countQuery = queryConfig.generateCountQuery();
>> long total = getJdbcTemplate().queryForLong(
>> countQuery.getSQL(),
>>
>> countQuery.getBindValues().toArray() );
>>
>> return new PageImpl<T>( results, pageable, total );
>> }
>>
>>
>> 3. Your code may do some optimizations and not fetch the count from
>> data source at all.
>>
>> List<T> results = fetch( first, pageSize );
>> count = ( results.size() < pageSize ) ? first + results.size() :
>> calculateSize();
>>
>> 4. You may choose not to provide total count AT ALL, simply providing
>> information if there is a next page (something inmethod-grid's
>> IDataSource) already does.
>>
>>
>> I tried to wrap this functionality in my own code:
>> public abstract class NewValueListDataProviderAdapter<T, F> extends
>> SortableDataProvider<T, String> {
>> public NewValueListDataProviderAdapter( Map<String, Object> context ) {
>> Injector.get().inject( this );
>> this.context = context;
>> }
>>
>> private final Map<String, Object> context;
>>
>> private transient Page<T> currentPage;
>>
>> protected abstract ValueList<T, F> getValueList();
>>
>> protected F getFilter() {
>> return null;
>> }
>>
>> @Override
>> public Iterator< ? extends T> iterator( long first, long count ) {
>> Sort sort = new Sort( new Order( getSort().isAscending() ?
>> Direction.ASC : Direction.DESC, getSort().getProperty() ) );
>> PageRequest pageRequest = new PageRequest( (int) ( first /
>> count ), (int) count, sort );
>> currentPage = getValueList().list( pageRequest,
>> getFilter(),
>> context );
>>
>> return currentPage.iterator();
>> }
>>
>> @Override
>> public long size() {
>> Preconditions.checkNotNull( currentPage );
>> return (int) currentPage.getTotalElements();
>> }
>>
>> @Override
>> public void detach() {
>> currentPage = null;
>> super.detach();
>> }
>> }
>>
>> unfortunately IDataProvider.size() is being called first and this
>> whole code simply throws NPE. I tried forcing the contract "call
>> iterator() first" in DataTable code and failed.
>>
>> WDYT? I know this is a big change but most pageable components could
>> take both "old and new" IDataProvider wrapping the old one with some
>> kind of decorator.
>>
>>
>>
>> One more thing:
>> What is the purpose of this logic in AbstractPageableView:
>>
>> public long getViewSize() {
>> return Math.min(getItemsPerPage(), getRowCount() -
>> getFirstItemOffset());
>> }
>>
>> this code actually triggers IDataProvider.size(). There is no actual
>> need for this. The only place this method is used is:
>>
>> @Override
>> protected Iterator<IModel<T>> getItemModels()
>> {
>> long offset = getFirstItemOffset();
>> long size = getViewSize();
>>
>> Iterator<IModel<T>> models = getItemModels(offset, size);
>>
>> models = new CappedIteratorAdapter<T>(models, size);
>>
>> return models;
>> }
>>
>> which is redundant anyway because of CappedIteratorAdapter use.
>>
>> One could simply do:
>>
>> @Override
>> protected Iterator<IModel<T>> getItemModels()
>> {
>> long offset = getFirstItemOffset();
>> long size = getItemsPerPage();
>> // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> Iterator<IModel<T>> models = getItemModels(offset, size);
>>
>> models = new CappedIteratorAdapter<T>(models, size);
>>
>> return models;
>> }
>>
>> and not trigger IDataProvider.size() without any harm to the rest of the
>> code.
>>
>>
>> Seems like I'm not the only one trying to go around IDataProvider
>> contract: https://issues.apache.org/jira/browse/WICKET-2618 (also
>> overriding getViewSize() - unfortunately in my case this moves NPE
>> just a little bit further to DataTables NavigationToolbar which calls
>> size() on page selection)
>>
>> --
>> Leszek Gawron
>>
--
Leszek Gawron http://www.mobilebox.pl/krs.html
CTO at MobileBox S.A.
