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.

        Issue Links

          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