Wicket
  1. Wicket
  2. WICKET-1784

Enhance IDataProvider to support applications using the Transfer Object J2EE pattern

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.3.3, 1.4-M3
    • Fix Version/s: 1.4-RC2
    • Component/s: wicket
    • Labels:
      None
    • Environment:
      Wicket 1.3.3 and 1.4-M3

      Description

      In some environments searches are performed in 'single call' fashion, using a transfer object.
      E.g. two queries performed by the data services tier before returning combined results to the UI tier:
      i. Query for paged search results
      ii. Query for a 'count' value representing total possible results

      The contract between DataView and IDataProvider does not support a 'single call' environment as the give/take relationship between these classes is biased towards DataView.
      DataView expects IDataProvider to provide it's size before providing IDataProvider with its offset and count.

      • DataView may have good reasons for needing size before it can provide offset/count.
      • But IDataProvider has equally good reasons for needing offset/count before it can provide size.

      The circular dependency:
      1. DataView calls IDataProvider.size()
      2. IDataProvider cannot return size as it cannot start a query until it receives offset/count from DataView
      3. These it does not receive until DataView calls IDataProvider.iterator() later on

      Others who experienced this problem (with CODE examples):


      The suggested solution of caching the combined search results and count value does not work if the search cannot begin until offset and count are available. And writing a custom DataView is not feasible either time wise as I understand that it cannot be done without needing to write a number of other classes too.

        Activity

        Hide
        R. Goodwin added a comment -

        Bit of theory to support the ticket description:

        The Transfer Object Pattern
        -------------------------------------
        Core J2EE Patterns, Alur et al, 2nd edition
        Page 415

        Forces:

        • You want to reduce remote requests across a network
          Solution
        • Use a Transfer Object to carry multiple data elements across a tier

        In terms of IDataProvider in the UI tier communicating with a session facade or business object on a remote server we want to make a single call that returns a single object containing both:
        1. Subset of search results (paged)
        2. The 'count' of total results


        The example below illustrates the difficulty of implementing the Transfer Object pattern with Wicket.
        The implementation of IDataProvider below does not work.

        class SearchResultsDataProvider implements IDataProvider

        private SearchObject searchObject;

        public int size()

        { // About to call search service, but where is offset and count, we need those now searchObject.setOffset( ?? ); searchObject.setPerPage( ?? ); int total = getSearchService().performSearch(searchObject).getTotalResults(); return total; }

        public Iterator iterator(int offset, int count)

        { // Okay to call search service here, but already (tried to) do this in size() method? searchObject.setOffset(offset); searchObject.setPerPage(count); List<T> results = lookupSearchService().performSearch(searchObject).getSearchResults(); return results.iterator(); }

        }

        class SearchObject

        { // Class to represent the transfer object, // Is exchanged between UI tier running on one box and the data tier running on another box. // Search criteria private int offset; private int resultsPerPage; private Object otherSearchCriteria; // Search results private List<T> searchResults; private int totalResults; }

        @Stateless
        class SearchService {

        // This runs on another server, possibly in another server cluster

        // Takes a SearchObject containing search criteria, populates with results and total,
        // then returns it to UI tier.

        public SearchObject performSearch(SearchObject searchObject)

        { int offset = searchObject.getOffset(); int perPage = searchObject.getResultsPerPage(); // Could be a db search, // Or could be a Lucene index search etc ... List<T> results = doActualSearch(offset, perPage, otherSearchCriteria); searchObject.setResults(results); // Do another search to get total results, // or just read from Hits object if was a Lucene search int total = doCountQuery(otherSearchCriteria); searchObject.setTotalResults(total); return searchObject; }

        }

        Show
        R. Goodwin added a comment - Bit of theory to support the ticket description: The Transfer Object Pattern ------------------------------------- Core J2EE Patterns, Alur et al, 2nd edition Page 415 Forces: You want to reduce remote requests across a network Solution Use a Transfer Object to carry multiple data elements across a tier In terms of IDataProvider in the UI tier communicating with a session facade or business object on a remote server we want to make a single call that returns a single object containing both: 1. Subset of search results (paged) 2. The 'count' of total results The example below illustrates the difficulty of implementing the Transfer Object pattern with Wicket. The implementation of IDataProvider below does not work. class SearchResultsDataProvider implements IDataProvider private SearchObject searchObject; public int size() { // About to call search service, but where is offset and count, we need those now searchObject.setOffset( ?? ); searchObject.setPerPage( ?? ); int total = getSearchService().performSearch(searchObject).getTotalResults(); return total; } public Iterator iterator(int offset, int count) { // Okay to call search service here, but already (tried to) do this in size() method? searchObject.setOffset(offset); searchObject.setPerPage(count); List<T> results = lookupSearchService().performSearch(searchObject).getSearchResults(); return results.iterator(); } } class SearchObject { // Class to represent the transfer object, // Is exchanged between UI tier running on one box and the data tier running on another box. // Search criteria private int offset; private int resultsPerPage; private Object otherSearchCriteria; // Search results private List<T> searchResults; private int totalResults; } @Stateless class SearchService { // This runs on another server, possibly in another server cluster // Takes a SearchObject containing search criteria, populates with results and total, // then returns it to UI tier. public SearchObject performSearch(SearchObject searchObject) { int offset = searchObject.getOffset(); int perPage = searchObject.getResultsPerPage(); // Could be a db search, // Or could be a Lucene index search etc ... List<T> results = doActualSearch(offset, perPage, otherSearchCriteria); searchObject.setResults(results); // Do another search to get total results, // or just read from Hits object if was a Lucene search int total = doCountQuery(otherSearchCriteria); searchObject.setTotalResults(total); return searchObject; } }
        Hide
        Igor Vaynberg added a comment -

        so what happens if

        you have a dataview that is set to show 5 items per page
        during the last request you had 10 items and you are curently on page 2 (5-9)

        the navigator is showing that you have 3 pages

        someone deletes one item

        you hit page 3 (items 10-14) on the navigator

        the dataview requests iterator(10,14)

        your service layer returns datawindow

        {count:9, iterator:empty}

        the dataview renders, but how?

        it cannot render page 3 in the navigator because according to size there is no such page, it also cannot render any other page as selected because it does not have data for that page

        so it has to requery?

        Show
        Igor Vaynberg added a comment - so what happens if you have a dataview that is set to show 5 items per page during the last request you had 10 items and you are curently on page 2 (5-9) the navigator is showing that you have 3 pages someone deletes one item you hit page 3 (items 10-14) on the navigator the dataview requests iterator(10,14) your service layer returns datawindow {count:9, iterator:empty} the dataview renders, but how? it cannot render page 3 in the navigator because according to size there is no such page, it also cannot render any other page as selected because it does not have data for that page so it has to requery?
        Hide
        R. Goodwin added a comment -

        Yes, on the rare occasion where offset exceeds the total count, some kind of additional behaviour needs performing to self-correct.

        Perhaps having something like this in the topmost superclass that deals with paging like:

        enum UnexpectedResultsAction

        { REDIRECT_TO_PREVIOUS, // go back to previous page, may still fail though REDIRECT_TO_LAST, // you'd have an updated size, so navigate to last possible page REDIRECT_TO_PAGE // go somewhere else to provide custom feedback message }

        setBehaviourOnUnexpectedResults(UnexpectedResultsAction action);

        I understand Its not very nice to pollute your framework with code for dealing with edge cases.
        But I think you need to encode into Wicket the concept and means of resolving the mismatch between 'expected' results and 'actual' results. Currently, you circumvent this mismatch problem at the expense of restricting how and when IDataProvider is able to get its data.

        The Transfer Object pattern is used in unexpected ways.
        A Lucene query returns both results and count in one query.
        I don't think you'll be able to convince teams using Lucene that they need to switch to making separate calls instead. And as said, caching results doesn't work as offset and count aren't known at the time search is executed.

        Personally I'd find this interface more flexible:

        interface IDataProvider

        { public void onRequestsData(int offset, int count); public long size(); public Iterator iterator(); // other methods ... }
        • onRequestsData() would always be called first and is the 'heavy' call
        • Subsequent calls to size() and iterator() would be 'light' calls
        • size() could be called any number of times in a single request without needing to cache the value higher up the chain, as the data retrieval stage would already have completed.

        This fits with the expected behaviour of a size() method call as well. Can't think of any class I ever used (e.g. List, Map) that performs a heavy operation on invocation of its size() method. Have always felt free to call it one or more times without concern for performance.

        Don't have any ideas about backwards compatibility issues with existing Wicket apps though.

        Show
        R. Goodwin added a comment - Yes, on the rare occasion where offset exceeds the total count, some kind of additional behaviour needs performing to self-correct. Perhaps having something like this in the topmost superclass that deals with paging like: enum UnexpectedResultsAction { REDIRECT_TO_PREVIOUS, // go back to previous page, may still fail though REDIRECT_TO_LAST, // you'd have an updated size, so navigate to last possible page REDIRECT_TO_PAGE // go somewhere else to provide custom feedback message } setBehaviourOnUnexpectedResults(UnexpectedResultsAction action); I understand Its not very nice to pollute your framework with code for dealing with edge cases. But I think you need to encode into Wicket the concept and means of resolving the mismatch between 'expected' results and 'actual' results. Currently, you circumvent this mismatch problem at the expense of restricting how and when IDataProvider is able to get its data. — The Transfer Object pattern is used in unexpected ways. A Lucene query returns both results and count in one query. I don't think you'll be able to convince teams using Lucene that they need to switch to making separate calls instead. And as said, caching results doesn't work as offset and count aren't known at the time search is executed. — Personally I'd find this interface more flexible: interface IDataProvider { public void onRequestsData(int offset, int count); public long size(); public Iterator iterator(); // other methods ... } onRequestsData() would always be called first and is the 'heavy' call Subsequent calls to size() and iterator() would be 'light' calls size() could be called any number of times in a single request without needing to cache the value higher up the chain, as the data retrieval stage would already have completed. This fits with the expected behaviour of a size() method call as well. Can't think of any class I ever used (e.g. List, Map) that performs a heavy operation on invocation of its size() method. Have always felt free to call it one or more times without concern for performance. Don't have any ideas about backwards compatibility issues with existing Wicket apps though.
        Hide
        Johan Compagner added a comment -

        but the call to
        public void onRequestsData(int offset, int count);

        is calculated based on size()

        size() can cache (until detach) anything they want..

        You could even do the full query, the problem is you dont know the offset/count in that part but that is calculated by the size()

        and find the interface what you define a bit weird.

        if i call onRequestData(10,10)

        what does size() then return if there are only 9 elements total?
        what does the iterator return? just nothing?

        Show
        Johan Compagner added a comment - but the call to public void onRequestsData(int offset, int count); is calculated based on size() size() can cache (until detach) anything they want.. You could even do the full query, the problem is you dont know the offset/count in that part but that is calculated by the size() and find the interface what you define a bit weird. if i call onRequestData(10,10) what does size() then return if there are only 9 elements total? what does the iterator return? just nothing?
        Hide
        Igor Vaynberg added a comment -

        >I don't think you'll be able to convince teams using Lucene that they need to switch to making
        >separate calls instead. And as said, caching results >doesn't work as offset and count aren't
        >known at the time search is executed.

        we are not looking to convince anybody. we provide repeaters that do not use idataprovider, eg refreshingview that can be used instead.

        Show
        Igor Vaynberg added a comment - >I don't think you'll be able to convince teams using Lucene that they need to switch to making >separate calls instead. And as said, caching results >doesn't work as offset and count aren't >known at the time search is executed. we are not looking to convince anybody. we provide repeaters that do not use idataprovider, eg refreshingview that can be used instead.
        Hide
        R. Goodwin added a comment -

        Johan,
        If only IDataProvider was altered, then yes the call to onRequestsData(offset,count) requires size() first as the problem has not been fixed throughout.
        I was thinking more of a redesign of the exchange throughout to cope with services providing an all-in-one Transfer Object.
        A good start might be looking again at the order in which things are done in AbstractPageableView.getItemModels().

        Igor,
        Subclassing RefreshingView can only be a short term solution, as one loses the paging functionality from AbstractPageableView downwards.
        And anyway, IDataProvider is looks very much like it was meant to be persistence method agnostic, whether that be a hookup with a database, Lucene search engine etc.

        I am confident with persisting with this request, as we've been running a number of succesful servlet/JSP web apps rendering Lucene results with paging for the last few years. And the exchange of data between UI tier and services tier is not unlike the 'single call' Transfer Object model.

        Show
        R. Goodwin added a comment - Johan, If only IDataProvider was altered, then yes the call to onRequestsData(offset,count) requires size() first as the problem has not been fixed throughout. I was thinking more of a redesign of the exchange throughout to cope with services providing an all-in-one Transfer Object. A good start might be looking again at the order in which things are done in AbstractPageableView.getItemModels(). Igor, Subclassing RefreshingView can only be a short term solution, as one loses the paging functionality from AbstractPageableView downwards. And anyway, IDataProvider is looks very much like it was meant to be persistence method agnostic, whether that be a hookup with a database, Lucene search engine etc. I am confident with persisting with this request, as we've been running a number of succesful servlet/JSP web apps rendering Lucene results with paging for the last few years. And the exchange of data between UI tier and services tier is not unlike the 'single call' Transfer Object model.
        Hide
        Johan Compagner added a comment -

        size() is not just called just before getItemModels()

        but also just for paging navigator isVisible() check and all kinds of stuff
        So what should that paging navigator call? not size() but Iterator(x,y) and then the size??
        I dont want the data at that time.

        Data is only relevant when really rendering the list items not before. But size() is used a lot more before/around that get data..
        so looking at getItemModels() isnt what you should do, Look what size() does everywhere.. thats the thing to look at.

        What i dont get is what if we had your interface that we first call onRequestsData(offset,count)
        if we would call that before size() where in that method do you calculate the total size??
        if you do there a real query to get just that data then you still have to do a separate query to do the size...

        Show
        Johan Compagner added a comment - size() is not just called just before getItemModels() but also just for paging navigator isVisible() check and all kinds of stuff So what should that paging navigator call? not size() but Iterator(x,y) and then the size?? I dont want the data at that time. Data is only relevant when really rendering the list items not before. But size() is used a lot more before/around that get data.. so looking at getItemModels() isnt what you should do, Look what size() does everywhere.. thats the thing to look at. What i dont get is what if we had your interface that we first call onRequestsData(offset,count) if we would call that before size() where in that method do you calculate the total size?? if you do there a real query to get just that data then you still have to do a separate query to do the size...
        Hide
        R. Goodwin added a comment -

        Perhaps rename to:

        onWantsPopulating(offset, count)

        This is more indicative of what I intended, which is to act as a early warning prompt to the IDataProvider that data needs loading as the owner of IDataProvider is about to call size() and iterator().

        Implementors of this method would need to retrieve both data and size simultaneously.
        The owner of IDataProvider can then freely call size() and iterator() at will, as it is understood that the heavy call has finished.

        But I understand there is the problem about size() being needed up front by paging and nothing I've suggest so far solves this. I'll have a look more closely when I get time.

        Really, a persistence provider should throw an OffsetOutOfBoundsException when given an offset that exceeds total results, so that paging components can catch problems that occur when when trying to re-use a size value from a previous request that may be out of date.

        Show
        R. Goodwin added a comment - Perhaps rename to: onWantsPopulating(offset, count) This is more indicative of what I intended, which is to act as a early warning prompt to the IDataProvider that data needs loading as the owner of IDataProvider is about to call size() and iterator(). Implementors of this method would need to retrieve both data and size simultaneously. The owner of IDataProvider can then freely call size() and iterator() at will, as it is understood that the heavy call has finished. — But I understand there is the problem about size() being needed up front by paging and nothing I've suggest so far solves this. I'll have a look more closely when I get time. Really, a persistence provider should throw an OffsetOutOfBoundsException when given an offset that exceeds total results, so that paging components can catch problems that occur when when trying to re-use a size value from a previous request that may be out of date.
        Hide
        Igor Vaynberg added a comment -

        instead of wasting all this time talking, why dont you create a repeater that works with an interface you like. doing that will flush out any problems in the thinking. than share it with the rest of us and we will see where we are.

        Show
        Igor Vaynberg added a comment - instead of wasting all this time talking, why dont you create a repeater that works with an interface you like. doing that will flush out any problems in the thinking. than share it with the rest of us and we will see where we are.
        Hide
        R. Goodwin added a comment -

        Hi, I've uploaded a small Wicket app to illustrate my point.
        It runs a simple search for items with paged results.
        To avoid db or file config hassles the result items are stored in-memory.

        Of interest are classes:

        • ISinglePassDataProvider
        • SinglePassDataView

        You should be able to drop the WAR straight into JBoss or Tomcat.
        Only these are needed on the classpath:

        • wicket-1.4-SNAPSHOT.jar
        • slf4j-api-1.4.2.jar
        • slf4j-log4j12-1.4.2.jar
        • log4j.jar

        Context is at:
        http://localhost:8080/wicket-paging-1/

        Show
        R. Goodwin added a comment - Hi, I've uploaded a small Wicket app to illustrate my point. It runs a simple search for items with paged results. To avoid db or file config hassles the result items are stored in-memory. Of interest are classes: ISinglePassDataProvider SinglePassDataView You should be able to drop the WAR straight into JBoss or Tomcat. Only these are needed on the classpath: wicket-1.4-SNAPSHOT.jar slf4j-api-1.4.2.jar slf4j-log4j12-1.4.2.jar log4j.jar Context is at: http://localhost:8080/wicket-paging-1/
        Hide
        R. Goodwin added a comment -

        I did find that creating the replacement DataView and DataProvider was pretty straightforward.

        But going down the path of subclassing higher up the hierarchy (i.e. RefreshingView) still concerns me:

        • The PagingNavigationLink classes are trying to get the data provider size before data is loaded, so a full solution would seem to require writing alternatives to those classes and PagingNavigator too. Until then, my version of DataView is flawed.
        • We lose out on functionality added lower down in the hierarchy, i.e. the change management code in AbstractPageeableView, and anything else added in future

        Unless I'm missing a trick here ...


        By the way, every other aspect of Wicket development has been perfect by comparison. So thanks.
        Can't believe how quick it is to put together web apps.

        Show
        R. Goodwin added a comment - I did find that creating the replacement DataView and DataProvider was pretty straightforward. But going down the path of subclassing higher up the hierarchy (i.e. RefreshingView) still concerns me: The PagingNavigationLink classes are trying to get the data provider size before data is loaded, so a full solution would seem to require writing alternatives to those classes and PagingNavigator too. Until then, my version of DataView is flawed. We lose out on functionality added lower down in the hierarchy, i.e. the change management code in AbstractPageeableView, and anything else added in future Unless I'm missing a trick here ... — By the way, every other aspect of Wicket development has been perfect by comparison. So thanks. Can't believe how quick it is to put together web apps.
        Hide
        R. Goodwin added a comment -

        Hello Igor, Johan,

        Was wondering if you've had a chance to look at the code I posted to (partially) solve the problem of data loading by DTO?

        Show
        R. Goodwin added a comment - Hello Igor, Johan, Was wondering if you've had a chance to look at the code I posted to (partially) solve the problem of data loading by DTO?
        Hide
        Igor Vaynberg added a comment -

        well...it seems like you were able to easily build what you need...

        so what do you need from wicket itself?

        personally i do not like this as it is not as clean as what we have and a lot more complex (with different strategies to handle unexected results). also things like paging navigation will cause problems, as you have already realized yourself.

        what we have in wicket is simple and works for 95% of the time. i do not think we should change it to handle this rare usecase, especially not if it is already possible to implement it externally.

        but this is just my two cents, you can bring this up on the list to get a wider audience.

        Show
        Igor Vaynberg added a comment - well...it seems like you were able to easily build what you need... so what do you need from wicket itself? personally i do not like this as it is not as clean as what we have and a lot more complex (with different strategies to handle unexected results). also things like paging navigation will cause problems, as you have already realized yourself. what we have in wicket is simple and works for 95% of the time. i do not think we should change it to handle this rare usecase, especially not if it is already possible to implement it externally. but this is just my two cents, you can bring this up on the list to get a wider audience.
        Hide
        R. Goodwin added a comment -

        Hello Igor,

        Well, everything's possible to implement externally isn't it.
        But was I saying it was so easy ... for the long term?
        Subclassing RefreshingView means losing (or re-writing) the functionality in AbstractPageableView,
        including all that stuff with versioning and the Change object.
        Bit too much detail for someone just starting out with Wicket isn't it?

        And are you confident that data retrieval by DTO is a rare use case?

        It's easy to write off J2EE patterns as out of date. But the Transfer Object pattern resurfaces in mysterious ways. For example, how could one integrate Wicket with the 3-tier Command pattern for data access suggested by Bauer and King in Java Persistence with Hibernate? pp. 718-724. Their solution also behaves like a DTO-over-the-wire pattern too.

        You know, the really great frameworks like Hibernate and Spring always provide helper classes for those tricky situations. Having to extend a framework class midway through the class hierarchy is really a last resort.

        Have you seen:
        http://www.nabble.com/Paging-query-relating-to-IDataProvider-and-Lucene-search-td19083164.html

        Show
        R. Goodwin added a comment - Hello Igor, Well, everything's possible to implement externally isn't it. But was I saying it was so easy ... for the long term? Subclassing RefreshingView means losing (or re-writing) the functionality in AbstractPageableView, including all that stuff with versioning and the Change object. Bit too much detail for someone just starting out with Wicket isn't it? And are you confident that data retrieval by DTO is a rare use case? It's easy to write off J2EE patterns as out of date. But the Transfer Object pattern resurfaces in mysterious ways. For example, how could one integrate Wicket with the 3-tier Command pattern for data access suggested by Bauer and King in Java Persistence with Hibernate? pp. 718-724. Their solution also behaves like a DTO-over-the-wire pattern too. You know, the really great frameworks like Hibernate and Spring always provide helper classes for those tricky situations. Having to extend a framework class midway through the class hierarchy is really a last resort. Have you seen: http://www.nabble.com/Paging-query-relating-to-IDataProvider-and-Lucene-search-td19083164.html
        Hide
        Igor Vaynberg added a comment -

        > Well, everything's possible to implement externally isn't it.

        no, not always. a good framework always has a way out...looks like you found one

        > But was I saying it was so easy ... for the long term?
        > Subclassing RefreshingView means losing (or re-writing) the functionality in AbstractPageableView,
        > including all that stuff with versioning and the Change object.

        you are doing paging in a significantly different way, so i dont think there is anything wrong with having to rewrite paging-related components. basically you are forcing the paging navigator to depend on the dataprovider instead of a simple size() call which ipageable provide and not just the dataview implements... can you rewrite them to work the way that you want without breaking api?

        > Bit too much detail for someone just starting out with Wicket isn't it?

        this is not a trivial thing so there is nothing wrong with having to know more then trivial things about the framework you are using. you seemed to have done just fine...

        > And are you confident that data retrieval by DTO is a rare use case?

        wicket has been around for 3+ years and this is the second time this has come up. im pretty confident 100% of wicket apps out there use paging repeaters...so yeah...im sure

        > It's easy to write off J2EE patterns as out of date.

        this has nothing to do with this pattern being out of date.

        > But the Transfer Object pattern resurfaces in mysterious ways. For example, how could one integrate Wicket with the
        > 3-tier Command pattern for data access suggested by Bauer and King in Java Persistence with Hibernate? pp. 718-724.
        > Their solution also behaves like a DTO-over-the-wire pattern too.

        even 3-tier systems usually have a call to retrieve just the size of the dataset, at least all the ones ive worked with did. if you are concerned with performance you can cache the size on the clientside for x minutes.

        > You know, the really great frameworks like Hibernate and Spring always provide helper classes for those tricky situations.
        > Having to extend a framework class midway through the class hierarchy is really a last resort.

        there is absolutely nothing wrong with having to extend a framework class midway through the hiearachy. hieararchy is about layering functionality, you pick the subclass that offers the most that you can use and extend it. unfortunately for you, paging is very foundamental and is thus near the top of the hierarchy, so you do get to miss out on the bottom pieces.

        > Have you seen:
        > http://www.nabble.com/Paging-query-relating-to-IDataProvider-and-Lucene-search-td19083164.html

        what about it? that thread didnt get much traffic.

        Show
        Igor Vaynberg added a comment - > Well, everything's possible to implement externally isn't it. no, not always. a good framework always has a way out...looks like you found one > But was I saying it was so easy ... for the long term? > Subclassing RefreshingView means losing (or re-writing) the functionality in AbstractPageableView, > including all that stuff with versioning and the Change object. you are doing paging in a significantly different way, so i dont think there is anything wrong with having to rewrite paging-related components. basically you are forcing the paging navigator to depend on the dataprovider instead of a simple size() call which ipageable provide and not just the dataview implements... can you rewrite them to work the way that you want without breaking api? > Bit too much detail for someone just starting out with Wicket isn't it? this is not a trivial thing so there is nothing wrong with having to know more then trivial things about the framework you are using. you seemed to have done just fine... > And are you confident that data retrieval by DTO is a rare use case? wicket has been around for 3+ years and this is the second time this has come up. im pretty confident 100% of wicket apps out there use paging repeaters...so yeah...im sure > It's easy to write off J2EE patterns as out of date. this has nothing to do with this pattern being out of date. > But the Transfer Object pattern resurfaces in mysterious ways. For example, how could one integrate Wicket with the > 3-tier Command pattern for data access suggested by Bauer and King in Java Persistence with Hibernate? pp. 718-724. > Their solution also behaves like a DTO-over-the-wire pattern too. even 3-tier systems usually have a call to retrieve just the size of the dataset, at least all the ones ive worked with did. if you are concerned with performance you can cache the size on the clientside for x minutes. > You know, the really great frameworks like Hibernate and Spring always provide helper classes for those tricky situations. > Having to extend a framework class midway through the class hierarchy is really a last resort. there is absolutely nothing wrong with having to extend a framework class midway through the hiearachy. hieararchy is about layering functionality, you pick the subclass that offers the most that you can use and extend it. unfortunately for you, paging is very foundamental and is thus near the top of the hierarchy, so you do get to miss out on the bottom pieces. > Have you seen: > http://www.nabble.com/Paging-query-relating-to-IDataProvider-and-Lucene-search-td19083164.html what about it? that thread didnt get much traffic.
        Hide
        R. Goodwin added a comment -

        Just saw something you wrote Dec 10 2007:

        "we need to rethink how the whole size() thing is dealt with"

        http://www.nabble.com/enclosures-and-dataview-td14258879.html#a14258879

        Well, a re-think could include giving the potential error condition underlying the dependency on size() a name:

        'Offset out of bounds'

        Having now named the devilish scenario, adding code explicitly to deal with it head on seems more natural rather than a hack.

        My example app demonstrates that it's relatively easy to deal with offset out of bounds.
        Forget original (and complicated) suggestion of having different strategies for handling it.
        Re-querying to the last known 'good' page suffices,
        which will back up all the way to page 1 if necessary.

        Did you try my runnable demo, and see how it copes when items are deleted between paging requests?

        Come on, give it another chance
        There is potentially a good contribution to Wicket here.

        I think you could change DataView internally without having to add nonsense like .onWantsPopulating() to the provider interface. Just changing the interaction so that iterator() is always called before size() may be enough to work. And of course you'd have to cache the first call to size() in the constructor to keep the paging comps happy.

        Show
        R. Goodwin added a comment - Just saw something you wrote Dec 10 2007: "we need to rethink how the whole size() thing is dealt with" http://www.nabble.com/enclosures-and-dataview-td14258879.html#a14258879 Well, a re-think could include giving the potential error condition underlying the dependency on size() a name: 'Offset out of bounds' Having now named the devilish scenario, adding code explicitly to deal with it head on seems more natural rather than a hack. My example app demonstrates that it's relatively easy to deal with offset out of bounds. Forget original (and complicated) suggestion of having different strategies for handling it. Re-querying to the last known 'good' page suffices, which will back up all the way to page 1 if necessary. Did you try my runnable demo, and see how it copes when items are deleted between paging requests? Come on, give it another chance There is potentially a good contribution to Wicket here. I think you could change DataView internally without having to add nonsense like .onWantsPopulating() to the provider interface. Just changing the interaction so that iterator() is always called before size() may be enough to work. And of course you'd have to cache the first call to size() in the constructor to keep the paging comps happy.
        Hide
        Igor Vaynberg added a comment -

        whats happening with the navigators? you havent figured out how to make them work yet with ipageable....

        Show
        Igor Vaynberg added a comment - whats happening with the navigators? you havent figured out how to make them work yet with ipageable....
        Hide
        R. Goodwin added a comment -

        Huh? I'm not doing anything with the navigators. Just using the standard PagingNavigator.

        Show
        R. Goodwin added a comment - Huh? I'm not doing anything with the navigators. Just using the standard PagingNavigator.
        Hide
        Igor Vaynberg added a comment -

        i was referring to this:

        > The PagingNavigationLink classes are trying to get the data provider size before data is loaded, so a full solution would seem to require
        > writing alternatives to those classes and PagingNavigator too. Until then, my version of DataView is flawed.

        components like that can be rendered before the dataview, and they need just the size...

        Show
        Igor Vaynberg added a comment - i was referring to this: > The PagingNavigationLink classes are trying to get the data provider size before data is loaded, so a full solution would seem to require > writing alternatives to those classes and PagingNavigator too. Until then, my version of DataView is flawed. components like that can be rendered before the dataview, and they need just the size...
        Hide
        R. Goodwin added a comment -

        Looks like PagingNavigationLink is calling getPageNumber to protect against possible data deletions.
        Clever simple code, but the pain is felt for me at the data provider end.

        public void onClick()

        { pageable.setCurrentPage(getPageNumber()); }

        Looks like PagingNavigationIncrementLink is doing other stuff with page count.

        Can't see a quick win to resolve this cleanly. Would probably have to re-write these Link classes, which effectively means having to re-write all the paging comps

        Would add similar 2-phase behaviour as I did to SinglePassDataView, where 1) request is made for a page, then 2) check made afterwards to see if that's the one that was loaded and self-correct if it wasn't.

        Think I'll call this the 'ask nicely before taking' approach.

        Show
        R. Goodwin added a comment - Looks like PagingNavigationLink is calling getPageNumber to protect against possible data deletions. Clever simple code, but the pain is felt for me at the data provider end. public void onClick() { pageable.setCurrentPage(getPageNumber()); } Looks like PagingNavigationIncrementLink is doing other stuff with page count. Can't see a quick win to resolve this cleanly. Would probably have to re-write these Link classes, which effectively means having to re-write all the paging comps Would add similar 2-phase behaviour as I did to SinglePassDataView, where 1) request is made for a page, then 2) check made afterwards to see if that's the one that was loaded and self-correct if it wasn't. Think I'll call this the 'ask nicely before taking' approach.
        Hide
        Stefan Fussenegger added a comment -

        I've to admit, that I didn't read but scanned all the previous comments.

        @ R. Goodwin:
        Performing complex Lucene queries twice just to figure out the size in advance is a problem for me as well. It really doubles the load on the index and the time spent searching. I also tried to write a custom RepeatingView implementation but stopped duplicating code when I hit the paging problem as I didn't want to duplicate the whole PagingNavigationLink hierarchy.

        @ Igor (and Johan):
        Looks like a change in PagingNavigationLink.getPageNumber() would allow such a custom AbstractPageableView replacement. Currently (1.3.5), pageable.setCurrentPage(getPageNumber()) makes sure that the new page isn't outside the valid range - something which is checked inside the repeater as well. If this check (or at least the final modifier) is removed, such a data-first PageableView could be used with the default Navigator.

        Long story short ... wouldn't it be okay to remove the final modifier from PagingNavigationLink.getPageNumber() and provide another method to access the "raw" pageNumber? Or remove the check completely and rely on the IPageable - not the PagingNavigationLink itself - to handle that special case (something that default IPageable implementations already do).

        Show
        Stefan Fussenegger added a comment - I've to admit, that I didn't read but scanned all the previous comments. @ R. Goodwin: Performing complex Lucene queries twice just to figure out the size in advance is a problem for me as well. It really doubles the load on the index and the time spent searching. I also tried to write a custom RepeatingView implementation but stopped duplicating code when I hit the paging problem as I didn't want to duplicate the whole PagingNavigationLink hierarchy. @ Igor (and Johan): Looks like a change in PagingNavigationLink.getPageNumber() would allow such a custom AbstractPageableView replacement. Currently (1.3.5), pageable.setCurrentPage(getPageNumber()) makes sure that the new page isn't outside the valid range - something which is checked inside the repeater as well. If this check (or at least the final modifier) is removed, such a data-first PageableView could be used with the default Navigator. Long story short ... wouldn't it be okay to remove the final modifier from PagingNavigationLink.getPageNumber() and provide another method to access the "raw" pageNumber? Or remove the check completely and rely on the IPageable - not the PagingNavigationLink itself - to handle that special case (something that default IPageable implementations already do).
        Hide
        R. Goodwin added a comment -

        Stefan,

        I raised this issue on Nabble in a more specific Wicket-Lucene kind of way in August without getting much interest.

        http://www.nabble.com/Paging-query-relating-to-IDataProvider-and-Lucene-search-td19083164.html

        But this related post is getting much more traffic:

        http://www.nabble.com/Is-there-any-other-way--DataProviders-must-hit-the-Db-twice-for-(possible)-large-datasets-td20701684.html

        Show
        R. Goodwin added a comment - Stefan, I raised this issue on Nabble in a more specific Wicket-Lucene kind of way in August without getting much interest. http://www.nabble.com/Paging-query-relating-to-IDataProvider-and-Lucene-search-td19083164.html But this related post is getting much more traffic: http://www.nabble.com/Is-there-any-other-way--DataProviders-must-hit-the-Db-twice-for-(possible)-large-datasets-td20701684.html
        Hide
        Bruno Borges added a comment -

        I think the purpose of Transfer Object Pattern , where it says it wants to reduce remote calls between tiers over the network is related to tiers like client / server architectures.

        This means that:

        i) A Swing client requests a query on a table for a remote EJB
        ii) The EJB executes two calls on the database
        ii.a) select count
        ii.b) select * ... limit x offset y / rownum between z and k // whatever DB paging technique is being used.
        iii) Put all this information into one single Transfer Object
        iv) Send the object back to the client application over the network

        So, if your Web Application is running on the same server with your services (EJBs, Spring Services, whatever), this means that you are not going to transport data over the network between those tiers (Web and Model). And there will be an "implicit" Transfer Object to the real client (web browser).

        Following this logic: -1 for this improvement.

        Show
        Bruno Borges added a comment - I think the purpose of Transfer Object Pattern , where it says it wants to reduce remote calls between tiers over the network is related to tiers like client / server architectures. This means that: i) A Swing client requests a query on a table for a remote EJB ii) The EJB executes two calls on the database ii.a) select count ii.b) select * ... limit x offset y / rownum between z and k // whatever DB paging technique is being used. iii) Put all this information into one single Transfer Object iv) Send the object back to the client application over the network So, if your Web Application is running on the same server with your services (EJBs, Spring Services, whatever), this means that you are not going to transport data over the network between those tiers (Web and Model). And there will be an "implicit" Transfer Object to the real client (web browser). Following this logic: -1 for this improvement.
        Hide
        R. Goodwin added a comment -

        For better load balancing our web apps run in a separate cluster to our core services cluster. So lookups are often across boxes. There's also a legacy rationale behind this, where grouping web apps to a select range of boxes reduces the administration of those boxes in the load-balanced pool. Admittedly, highly coupled services are grouped on the same boxes to reduce expensive lookups and they're all running in the same data centre, but don't think that should influence the debate.

        Currently, Wicket-Lucene integration using standard Wicket components is just plain wrong.
        I'm not willing to re-write data provision classes AND re-write paging mechanism.
        And fortunately .. ha ha ... I don't have to ... because we're not using it in production.
        T'was just a wee experiment to test the cloudy waters.

        Actually, I'm -(-1) for a Wicket improvement that doesn't force anyone to have to abandon their preferred data access strategy.

        Show
        R. Goodwin added a comment - For better load balancing our web apps run in a separate cluster to our core services cluster. So lookups are often across boxes. There's also a legacy rationale behind this, where grouping web apps to a select range of boxes reduces the administration of those boxes in the load-balanced pool. Admittedly, highly coupled services are grouped on the same boxes to reduce expensive lookups and they're all running in the same data centre, but don't think that should influence the debate. Currently, Wicket-Lucene integration using standard Wicket components is just plain wrong. I'm not willing to re-write data provision classes AND re-write paging mechanism. And fortunately .. ha ha ... I don't have to ... because we're not using it in production. T'was just a wee experiment to test the cloudy waters. Actually, I'm -(-1) for a Wicket improvement that doesn't force anyone to have to abandon their preferred data access strategy.
        Hide
        Stefan Fussenegger added a comment -

        Bruno,

        I don't think the purpose of this ticket is to follow a pattern. It's about a major performance hog. If you'd be accessing Lucene through Wicket using an IDataProvider, you'd realize that you are performing every query twice (except those with an empty resultset). And hitting your search index 10 or 20 times a second is quite a difference.

        Did you read my comment (https://issues.apache.org/jira/browse/WICKET-1784?focusedCommentId=12650587#action_12650587) above? I suggested to only change the implementation of PagingNavigationLink to allow a custom Repeater that implements IPageable (in order to use the default Navigation component - or is it the Navigator? I always confuse them ...).

        As mentioned above, PagingNavigationLink.getPageNumber() checks whether the chosen page number (chosen by clicking such a link) is allowed. It does that by calculating the current number of pages (i.e. fetches the size). This check isn't needed, as any current IPageable implementation checks that special case itself (which is a Good Thing (tm)).

        Therefore, the change would be quite trivial and would have a tremendous effect for Lucene-based applications at the same time.

        Hence, +1 - or may I give more than that?

        Btw, fetching data and total amount at the same time is possible with MySQL too (See http://dev.mysql.com/doc/refman/5.1/en/information-functions.html#function_found-rows).

        Show
        Stefan Fussenegger added a comment - Bruno, I don't think the purpose of this ticket is to follow a pattern. It's about a major performance hog. If you'd be accessing Lucene through Wicket using an IDataProvider, you'd realize that you are performing every query twice (except those with an empty resultset). And hitting your search index 10 or 20 times a second is quite a difference. Did you read my comment ( https://issues.apache.org/jira/browse/WICKET-1784?focusedCommentId=12650587#action_12650587 ) above? I suggested to only change the implementation of PagingNavigationLink to allow a custom Repeater that implements IPageable (in order to use the default Navigation component - or is it the Navigator? I always confuse them ...). As mentioned above, PagingNavigationLink.getPageNumber() checks whether the chosen page number (chosen by clicking such a link) is allowed. It does that by calculating the current number of pages (i.e. fetches the size). This check isn't needed, as any current IPageable implementation checks that special case itself (which is a Good Thing (tm)). Therefore, the change would be quite trivial and would have a tremendous effect for Lucene-based applications at the same time. Hence, +1 - or may I give more than that? Btw, fetching data and total amount at the same time is possible with MySQL too (See http://dev.mysql.com/doc/refman/5.1/en/information-functions.html#function_found-rows ).
        Hide
        Igor Vaynberg added a comment -

        see pagingnavigationlink#cullPageNumber(int x)

        Show
        Igor Vaynberg added a comment - see pagingnavigationlink#cullPageNumber(int x)

          People

          • Assignee:
            Igor Vaynberg
            Reporter:
            R. Goodwin
          • Votes:
            1 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development