Uploaded image for project: 'Directory Client API'
  1. Directory Client API
  2. DIRAPI-241

new GeneralizedTime(String) fails for fraction close to one

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 1.0.0-M30
    • Fix Version/s: 1.0.0-M32
    • Labels:
      None

      Description

      When parsing a time with a resolution of nanos (like generated by the openldap ppolicy overlay) it results in a Time that fails to be parsed by org.apache.directory.api.util.GeneralizedTime.GeneralizedTime(String)

      Test:

      Test.java
          static DateFormat FORMAT = new SimpleDateFormat( "dd/MM/yyyy HH:mm:ss.SSSS z" );
          
          @Test
          public void fractionCloseToOne() throws ParseException
          {
              GeneralizedTime close = new GeneralizedTime( "20000101000000.9994Z" );
              
              assertThat( close.getDate(), is( equalTo( FORMAT.parse( "01/01/2000 00:00:00.999 GMT" ) ) ) );
              
              GeneralizedTime closer = new GeneralizedTime( "20000101000000.9995Z" );
              
              assertThat( closer.getDate(), is( equalTo( FORMAT.parse( "01/01/2000 00:00:01 GMT" ) ) ) );
          }
      

        Activity

        Hide
        elecharny Emmanuel Lecharny added a comment -

        Woot, good catch !!!

        At some point, when parsing the date, we parse the fraction of seconds with this method :

            private void parseFractionOfSecond() throws ParseException
            {
                parseFractionDelmiter( 14 );
                String fraction = getFraction( 14 + 1 );
                upFractionLength = fraction.length();
        
                double fract = Double.parseDouble( "0." + fraction ); <<<<----------- Problem !!!
                int millisecond = ( int ) Math.round( fract * 1000 );
        
                calendar.set( Calendar.MILLISECOND, millisecond );
            }
        

        The rational for the round is that the Calendar only support milliseconds precision, while the generalizedTime gowd down to a tenth of a millisecond. 9995 get rounded to 1000, which is out of the possible values ([0..999]).

        This is a bit of a problem as we have to cut the corner a bit (loosing some precision), and more critical, if we have to round 999x to 1000 (when x >= 5), then we have to propagate the extra time to seconds, and then minutes if we were at 59s, then to hours, etc.

        In any case, it has to be fixed.

        (FTR, the issue is present since the very beginning, 7 years ago...)

        Show
        elecharny Emmanuel Lecharny added a comment - Woot, good catch !!! At some point, when parsing the date, we parse the fraction of seconds with this method : private void parseFractionOfSecond() throws ParseException { parseFractionDelmiter( 14 ); String fraction = getFraction( 14 + 1 ); upFractionLength = fraction.length(); double fract = Double .parseDouble( "0." + fraction ); <<<<----------- Problem !!! int millisecond = ( int ) Math .round( fract * 1000 ); calendar.set( Calendar.MILLISECOND, millisecond ); } The rational for the round is that the Calendar only support milliseconds precision, while the generalizedTime gowd down to a tenth of a millisecond. 9995 get rounded to 1000, which is out of the possible values ( [0..999] ). This is a bit of a problem as we have to cut the corner a bit (loosing some precision), and more critical, if we have to round 999x to 1000 (when x >= 5), then we have to propagate the extra time to seconds, and then minutes if we were at 59s, then to hours, etc. In any case, it has to be fixed. (FTR, the issue is present since the very beginning, 7 years ago...)
        Hide
        MaxFichtelmann Max Fichtelmann added a comment -

        How about using Math.floor for rounding? reduces the precision from +-0.5ms to +-1ms if that is acceptable.

        Show
        MaxFichtelmann Max Fichtelmann added a comment - How about using Math.floor for rounding? reduces the precision from +-0.5ms to +-1ms if that is acceptable.
        Hide
        elecharny Emmanuel Lecharny added a comment - - edited

        Even simpler :

            private void parseFractionOfSecond() throws ParseException
            {
                parseFractionDelmiter( 14 );
                String fraction = getFraction( 14 + 1 );
                upFractionLength = fraction.length();
                int milliseconds =  Integer.parseInt( fraction );
        
                switch ( upFractionLength )
                {
                    case 1:
                        milliseconds *= 10;
                        
                    case 2:
                        milliseconds *= 10;
                        
                    case 3 :
                        break;
                        
                    case 4 :
                        milliseconds /= 10;
                        break;
                        
                }
        
                calendar.set( Calendar.MILLISECOND, milliseconds );
            }
        

        No more Double, precision is 1ms with a potential delta of 0.5ms in the past. Note that the previous method precision was also 1ms, not 0.5ms : the difference is that everything between nnn5 and nn(n+1)4 was rounded to nn(n+1).

        Show
        elecharny Emmanuel Lecharny added a comment - - edited Even simpler : private void parseFractionOfSecond() throws ParseException { parseFractionDelmiter( 14 ); String fraction = getFraction( 14 + 1 ); upFractionLength = fraction.length(); int milliseconds = Integer .parseInt( fraction ); switch ( upFractionLength ) { case 1: milliseconds *= 10; case 2: milliseconds *= 10; case 3 : break ; case 4 : milliseconds /= 10; break ; } calendar.set( Calendar.MILLISECOND, milliseconds ); } No more Double, precision is 1ms with a potential delta of 0.5ms in the past. Note that the previous method precision was also 1ms, not 0.5ms : the difference is that everything between nnn5 and nn(n+1)4 was rounded to nn(n+1).
        Hide
        MaxFichtelmann Max Fichtelmann added a comment -

        fraction of length 4 was just for demonstration btw.
        It may be any length according to the spec (6 would be expected for nano seconds - the problematic time was "20150701082305.999909Z" in my case)

        Show
        MaxFichtelmann Max Fichtelmann added a comment - fraction of length 4 was just for demonstration btw. It may be any length according to the spec (6 would be expected for nano seconds - the problematic time was "20150701082305.999909Z" in my case)
        Hide
        elecharny Emmanuel Lecharny added a comment -

        We can make it work up to 6 digits (and that would be microseconds, not nanoseconds). Actully, I chcked at the ISO8601 specs that does not give any limit to the number of digits.

        Clearly, my proposed fix is not enough.

        Show
        elecharny Emmanuel Lecharny added a comment - We can make it work up to 6 digits (and that would be microseconds, not nanoseconds). Actully, I chcked at the ISO8601 specs that does not give any limit to the number of digits. Clearly, my proposed fix is not enough.
        Hide
        elecharny Emmanuel Lecharny added a comment -

        Math.floor(xxx) is the way to go, AFAICT, considering the fact that the umber of digits in the fraction is unknown.

        Show
        elecharny Emmanuel Lecharny added a comment - Math.floor(xxx) is the way to go, AFAICT, considering the fact that the umber of digits in the fraction is unknown.
        Hide
        elecharny Emmanuel Lecharny added a comment -

        I have committed the replacement of the Math.round() method by Math.floor : http://svn.apache.org/r1688811

        I'm not pleased by this change : I do think we should use Math.round(), and if we get 1000 ms as a result, then we should change it to 0, and propagate the value to seconds, minutes, hours, days, months, year.

        We could also avoid using Double and only use a long, which is likely to be faster.

        Show
        elecharny Emmanuel Lecharny added a comment - I have committed the replacement of the Math.round() method by Math.floor : http://svn.apache.org/r1688811 I'm not pleased by this change : I do think we should use Math.round() , and if we get 1000 ms as a result, then we should change it to 0, and propagate the value to seconds, minutes, hours, days, months, year. We could also avoid using Double and only use a long, which is likely to be faster.
        Hide
        elecharny Emmanuel Lecharny added a comment -

        Note : the last suggestion is marginaly faster : 5%. useless.

        Show
        elecharny Emmanuel Lecharny added a comment - Note : the last suggestion is marginaly faster : 5%. useless.
        Hide
        elecharny Emmanuel Lecharny added a comment -

        Fixed in 1.0.0-32

        Show
        elecharny Emmanuel Lecharny added a comment - Fixed in 1.0.0-32

          People

          • Assignee:
            Unassigned
            Reporter:
            MaxFichtelmann Max Fichtelmann
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development