Wicket
  1. Wicket
  2. WICKET-1175

IDataProvider-Overflow with size()

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.3.0-rc1
    • Fix Version/s: 6.0.0-beta1
    • Component/s: wicket
    • Labels:
      None
    • Environment:
      any

      Description

      Hi,

      I get an Integer-overflow with my Dataprovider (yeah, there are a couple of entries in the database). Is there a reason why size() and iterator( first, count ) are limited to Integer?

      Regards, — Jan.

        Activity

        Hide
        Johan Compagner added a comment -

        we could change the interface to use all longs..
        But this is a pretty big api change so i guess it has to wait for 1.4?

        But does a size of larger then 2.1G items really make sense?

        Show
        Johan Compagner added a comment - we could change the interface to use all longs.. But this is a pretty big api change so i guess it has to wait for 1.4? But does a size of larger then 2.1G items really make sense?
        Hide
        Jan Kriesten added a comment -

        hi johan,

        i don't think it's a question of sense, but to be able to handle such situations. as stated elsewhere - jpa also delivers longs, so currently you strip those down to int, too, which is not a nice thing at all imho.

        regards, — jan.

        Show
        Jan Kriesten added a comment - hi johan, i don't think it's a question of sense, but to be able to handle such situations. as stated elsewhere - jpa also delivers longs, so currently you strip those down to int, too, which is not a nice thing at all imho. regards, — jan.
        Hide
        Martin Grigorov added a comment -

        Igor, is it time to make this change ?
        I just changed the signature of there two methods and it seems many places will need the change 'int -> long'.

        Show
        Martin Grigorov added a comment - Igor, is it time to make this change ? I just changed the signature of there two methods and it seems many places will need the change 'int -> long'.
        Hide
        Igor Vaynberg added a comment -

        i would wait for 1.6. ive tried this refactor before and it touches a hell of a lot of places.

        Show
        Igor Vaynberg added a comment - i would wait for 1.6. ive tried this refactor before and it touches a hell of a lot of places.
        Hide
        Igor Vaynberg added a comment -

        this will be a pita to migrate :/

        Show
        Igor Vaynberg added a comment - this will be a pita to migrate :/
        Hide
        Emond Papegaaij added a comment -

        The attached patch changes Loop back to int. Loop with a long does not work (you cannot add that many components to the parent). It also isn't needed for other components to function properly and it makes ListItem's constructors inconsistent.

        Show
        Emond Papegaaij added a comment - The attached patch changes Loop back to int. Loop with a long does not work (you cannot add that many components to the parent). It also isn't needed for other components to function properly and it makes ListItem's constructors inconsistent.
        Hide
        Emond Papegaaij added a comment -

        New patch. PagingNavigation is based on Loop and needed some work to keep it working with ints.

        Show
        Emond Papegaaij added a comment - New patch. PagingNavigation is based on Loop and needed some work to keep it working with ints.
        Hide
        Martin Grigorov added a comment -

        reopening to consider Emond's patch about Loop

        Show
        Martin Grigorov added a comment - reopening to consider Emond's patch about Loop
        Hide
        vineet semwal added a comment - - edited

        this change will cause a lot of trouble in migration ,jpa/hibernate still expects int for setfirstresult and setmaxresult ,i think this change wasnt even necessary but since its already done ..i think IdataProvider#iterator should be changed to expect ints for first and count and idataprovider#size() can still return long ,the idea is its totally impractical to think some one will iterate over the capability of int but yes a developer can still retrieve long Idataprovider#size()

        thanks

        Show
        vineet semwal added a comment - - edited this change will cause a lot of trouble in migration ,jpa/hibernate still expects int for setfirstresult and setmaxresult ,i think this change wasnt even necessary but since its already done ..i think IdataProvider#iterator should be changed to expect ints for first and count and idataprovider#size() can still return long ,the idea is its totally impractical to think some one will iterate over the capability of int but yes a developer can still retrieve long Idataprovider#size() thanks
        Hide
        Jesse Long added a comment -

        The "first" argument to IDataProvider.iterator() cannot be int, if size() returns long. If it was int, you would be unable to retrieve a page starting at offset Integer.MAX_VALUE + 1.

        I cant imagine needing more than Integer.MAX_VALUE (2.1 billion) per page, so maybe the count argument could be an int?

        Show
        Jesse Long added a comment - The "first" argument to IDataProvider.iterator() cannot be int, if size() returns long. If it was int, you would be unable to retrieve a page starting at offset Integer.MAX_VALUE + 1. I cant imagine needing more than Integer.MAX_VALUE (2.1 billion) per page, so maybe the count argument could be an int?
        Hide
        Martin Grigorov added a comment -

        Welcome to the Big Data era

        I agree with Jesse - 'count' could be an 'int'.

        Another way is to change the API to:

        Iterator<? extends T> iterator(java.lang.Number first, java.lang.Number count);
        Number size();

        This way we will leave the impl to decide whether to use an Int or a Long: first.longValue() , count.intValue().
        Wicket core repeaters will use the 'long' view as they do now but custom components may use 'short' if they want.
        But this will be even more typing than the cast that you need to use now.

        Show
        Martin Grigorov added a comment - Welcome to the Big Data era I agree with Jesse - 'count' could be an 'int'. Another way is to change the API to: Iterator<? extends T> iterator(java.lang.Number first, java.lang.Number count); Number size(); This way we will leave the impl to decide whether to use an Int or a Long: first.longValue() , count.intValue(). Wicket core repeaters will use the 'long' view as they do now but custom components may use 'short' if they want. But this will be even more typing than the cast that you need to use now.
        Hide
        vineet semwal added a comment -

        jesse : i agree with that but my point was no one will ever retrieve the page that will cause overflow but i later realized it can still be broken with navigator when the last page is calculated, i agree with you count argument can be int but it wont solve every problem, user will still have to cast from long to int first argument as jpa/hibernate still expects int for setfirstresult

        martin-g: your suggestion is really nice..

        Show
        vineet semwal added a comment - jesse : i agree with that but my point was no one will ever retrieve the page that will cause overflow but i later realized it can still be broken with navigator when the last page is calculated, i agree with you count argument can be int but it wont solve every problem, user will still have to cast from long to int first argument as jpa/hibernate still expects int for setfirstresult martin-g: your suggestion is really nice..
        Hide
        Igor Vaynberg added a comment -

        what we really need is IDataProvider<T,O extends Number,S extends Number>

        { iterator<? extends T>(O first, O count); S size(); }

        that way a JPADataProvider can be IDataProvider<T, Integer, Long>

        dont know how much noise the other two variables will introduce into the codebase though.

        the problem with Number is that all impls then have to cast to O which is rather annoying. unless method covariance would allow them to be overridden via a subclass - havent tried.

        Show
        Igor Vaynberg added a comment - what we really need is IDataProvider<T,O extends Number,S extends Number> { iterator<? extends T>(O first, O count); S size(); } that way a JPADataProvider can be IDataProvider<T, Integer, Long> dont know how much noise the other two variables will introduce into the codebase though. the problem with Number is that all impls then have to cast to O which is rather annoying. unless method covariance would allow them to be overridden via a subclass - havent tried.
        Hide
        Martin Grigorov added a comment -

        Let me repeat Jesse:

        "O first" with "S size" where O is smaller than S is broken.

        How I can reach (O.max+1)th element ?

        It should be:
        iterator<? extends T>(S first, O count);
        S size();

        Show
        Martin Grigorov added a comment - Let me repeat Jesse: "O first" with "S size" where O is smaller than S is broken. How I can reach (O.max+1)th element ? It should be: iterator<? extends T>(S first, O count); S size();
        Hide
        Igor Vaynberg added a comment -

        that wasnt Jesse, that was me

        yes, it is broken, in theory. however, this is how hibernate and jpa work - which together probably account for over 90% of persistence providers used with wicket.

        the point is to allow the dataprovider to work with the persistence api cleanly - even if that api is broken.

        Show
        Igor Vaynberg added a comment - that wasnt Jesse, that was me yes, it is broken, in theory. however, this is how hibernate and jpa work - which together probably account for over 90% of persistence providers used with wicket. the point is to allow the dataprovider to work with the persistence api cleanly - even if that api is broken.
        Hide
        Martin Grigorov added a comment -

        I meant Jesse's comment: https://issues.apache.org/jira/browse/WICKET-1175?focusedCommentId=13254549&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13254549

        I'm not sure that I like the new noisier (more typing needed) approach better than the current need to down-cast from long to int.

        Show
        Martin Grigorov added a comment - I meant Jesse's comment: https://issues.apache.org/jira/browse/WICKET-1175?focusedCommentId=13254549&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13254549 I'm not sure that I like the new noisier (more typing needed) approach better than the current need to down-cast from long to int.
        Hide
        Igor Vaynberg added a comment -

        meh. i just looked at it in detail and it will be too damn noisy to do this, and there is no good way to translate the types anyways. so i say we keep it as is - stick to long which can be cast down to an int if need be.

        Show
        Igor Vaynberg added a comment - meh. i just looked at it in detail and it will be too damn noisy to do this, and there is no good way to translate the types anyways. so i say we keep it as is - stick to long which can be cast down to an int if need be.

          People

          • Assignee:
            Igor Vaynberg
            Reporter:
            Jan Kriesten
          • Votes:
            1 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development