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 <lgaw...@apache.org> 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
>
