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
