[JAVA-85]: refactoring of ObjectId, to store values native, and encode/decode big endian. Fixes some flaws with Increment value failing comparison.

Author: agirbal

src/main/org/bson/BSONDecoder.java

@@ -141,7 +141,8 @@ boolean decodeElement()
             break;
 
         case OID:
-            _callback.gotObjectId( name , new ObjectId( _in.readInt() , _in.readInt() , _in.readInt() ) );
+            // OID is stored as big endian
+            _callback.gotObjectId( name , new ObjectId( _in.readIntBE() , _in.readIntBE() , _in.readIntBE() ) );
             break;
 
         case REF:
@@ -325,6 +326,11 @@ int readInt()
             return Bits.readInt( _inputBuffer , _need(4) );
         }
 
+        int readIntBE()
+            throws IOException {
+            return Bits.readIntBE( _inputBuffer , _need(4) );
+        }
+
         long readLong()
             throws IOException {
             return Bits.readLong( _inputBuffer , _need(8) );

src/main/org/bson/BSONEncoder.java

@@ -352,9 +352,10 @@ private void _putString( String name , String s, byte type ){
 
     protected void putObjectId( String name , ObjectId oid ){
         _put( OID , name );
-        _buf.writeInt( oid._time() );
-        _buf.writeInt( oid._machine() );
-        _buf.writeInt( oid._inc() );
+        // according to spec, values should be stored big endian
+        _buf.writeIntBE( oid._time() );
+        _buf.writeIntBE( oid._machine() );
+        _buf.writeIntBE( oid._inc() );
     }
 
     private void putPattern( String name, Pattern p ) {

src/main/org/bson/io/Bits.java

@@ -63,6 +63,15 @@ public static int readInt( byte[] data , int offset ) {
         return x;
     }
 
+    public static int readIntBE( byte[] data , int offset ) {
+        int x = 0;
+        x |= ( 0xFF & data[offset+0] ) << 24;
+        x |= ( 0xFF & data[offset+1] ) << 16;
+        x |= ( 0xFF & data[offset+2] ) << 8;
+        x |= ( 0xFF & data[offset+3] ) << 0;
+        return x;
+    }
+
     public static long readLong( InputStream in )
         throws IOException {
         return readLong( in , new byte[8] );

src/main/org/bson/io/OutputBuffer.java

@@ -108,6 +108,13 @@ public void writeInt( int x ){
         write( x >> 24 );
     }
 
+    public void writeIntBE( int x ){
+        write( x >> 24 );
+        write( x >> 16 );
+        write( x >> 8 );
+        write( x );
+    }
+
     public void writeInt( int pos , int x ){
         final int save = getPosition();
         setPosition( pos );

src/main/org/bson/types/ObjectId.java

@@ -100,24 +100,20 @@ public static ObjectId massageToObjectId( Object o ){
     }
 
     public ObjectId( Date time ){
-        _time = _flip( (int)(time.getTime() / 1000) );
-        _machine = _genmachine;
-        _inc = _nextInc.getAndIncrement();
-        _new = false;
+        this(time, _genmachine, _nextInc.getAndIncrement());
     }
 
     public ObjectId( Date time , int inc ){
         this( time , _genmachine , inc );
     }
 
     public ObjectId( Date time , int machine , int inc ){
-        _time = _flip( (int)(time.getTime() / 1000) );
+        _time = (int)(time.getTime() / 1000);
         _machine = machine;
         _inc = inc;
         _new = false;
     }
 
-
     /** Creates a new instance from a string.
      * @param s the string to convert
      * @throws IllegalArgumentException if the string is not a valid id
@@ -133,44 +129,45 @@ public ObjectId( String s , boolean babble ){
 
         if ( babble )
             s = babbleToMongod( s );
-        
+
         byte b[] = new byte[12];
         for ( int i=0; i<b.length; i++ ){
-            b[b.length-(i+1)] = (byte)Integer.parseInt( s.substring( i*2 , i*2 + 2) , 16 );
+            b[i] = (byte)Integer.parseInt( s.substring( i*2 , i*2 + 2) , 16 );
         }
         ByteBuffer bb = ByteBuffer.wrap( b );
-        
-        _inc = bb.getInt(); 
-        _machine = bb.getInt();
         _time = bb.getInt();
-
+        _machine = bb.getInt();
+        _inc = bb.getInt();
         _new = false;
     }
 
     public ObjectId( byte[] b ){
         if ( b.length != 12 )
             throw new IllegalArgumentException( "need 12 bytes" );
-        reverse( b );
         ByteBuffer bb = ByteBuffer.wrap( b );
-        
-        _inc = bb.getInt();            
-        _machine = bb.getInt();
         _time = bb.getInt();
+        _machine = bb.getInt();
+        _inc = bb.getInt();
+        _new = false;
     }
 
-    
+    /**
+     * Creates an ObjectId
+     * @param time time in seconds
+     * @param machine machine ID
+     * @param inc incremental value
+     */
     public ObjectId( int time , int machine , int inc ){
         _time = time;
         _machine = machine;
         _inc = inc;
-        
         _new = false;
     }
 
     /** Create a new object id.
      */
     public ObjectId(){
-        _time = _curtime();
+        _time = (int) (System.currentTimeMillis() / 1000);
         _machine = _genmachine;
         _inc = _nextInc.getAndIncrement();
         _new = true;
@@ -221,20 +218,12 @@ public String toStringMongod(){
     public byte[] toByteArray(){
         byte b[] = new byte[12];
         ByteBuffer bb = ByteBuffer.wrap( b );
-        bb.putInt( _inc );
-        bb.putInt( _machine );
+        // by default BB is big endian like we need
         bb.putInt( _time );
-        reverse( b );
+        bb.putInt( _machine );
+        bb.putInt( _inc );
         return b;
     }
-
-    static void reverse( byte[] b ){
-        for ( int i=0; i<b.length/2; i++ ){
-            byte t = b[i];
-            b[i] = b[ b.length-(i+1) ];
-            b[b.length-(i+1)] = t;
-        }
-    }
 
     static String _pos( String s , int p ){
         return s.substring( p * 2 , ( p * 2 ) + 2 );
@@ -257,41 +246,61 @@ public String toString(){
         return toStringMongod();
     }
 
-    int _compare( int i , int j ){
-        i = _flip(i);
-        j = _flip(j);
-
-        final int diff = j - i;
-        
-        if ( i >= 0 ){
-            return j >= 0 ? -diff : -1;
-        }
-        
-        return j < 0 ? -diff : 1;
+    int _compareUnsigned( int i , int j ){
+        long li = 0xFFFFFFFFL;
+        li = i & li;
+        long lj = 0xFFFFFFFFL;
+        lj = j & lj;
+        long diff = li - lj;
+        if (diff < Integer.MIN_VALUE)
+            return Integer.MIN_VALUE;
+        if (diff > Integer.MAX_VALUE)
+            return Integer.MAX_VALUE;
+        return (int) diff;
     }
 
     public int compareTo( ObjectId id ){
         if ( id == null )
             return -1;
 
-        int x = _compare( _time , id._time );
+        int x = _compareUnsigned( _time , id._time );
         if ( x != 0 )
             return x;
 
-        x = _compare( _machine , id._machine );
+        x = _compareUnsigned( _machine , id._machine );
         if ( x != 0 )
             return x;
 
-        return _compare( _inc , id._inc );
+        x = _compareUnsigned( _inc , id._inc );
+        if (Math.abs(x) > Integer.MAX_VALUE / 2) {
+            // this means that for same second and process more than (max int)/2 were generated
+            // highly unlikely, most likely the counter wrapped
+            if (x < 0)
+                return 1;
+            else
+                return -1;
+        }
+        return x;
     }
 
     public int getMachine(){
         return _machine;
     }
-    
+
+    /**
+     * Gets the time of this ID, in milliseconds
+     * @return
+     */
     public long getTime(){
-        long z = _flip( _time );
-        return z * 1000;
+        return _time * 1000L;
+    }
+
+    /**
+     * Gets the time of this ID, in seconds
+     * @return
+     */
+    public int getTimeSecond(){
+        return _time;
     }
 
     public int getInc(){
@@ -316,6 +325,22 @@ public void notNew(){
         _new = false;
     }
 
+    /**
+     * Gets the generated machine ID, identifying the machine / process / class loader
+     * @return
+     */
+    public static int getGenMachineId() {
+        return _genmachine;
+    }
+
+    /**
+     * Gets the current value of the auto increment
+     * @return
+     */
+    public static int getCurrentInc() {
+        return _nextInc.get();
+    }
+
     final int _time;
     final int _machine;
     final int _inc;
@@ -331,12 +356,7 @@ public static int _flip( int x ){
         return z;
     }
 
-    private static int _curtime(){
-        return _flip( (int)(System.currentTimeMillis()/1000) );
-    }
-
     private static AtomicInteger _nextInc = new AtomicInteger( (new java.util.Random()).nextInt() );
-    private static final String _incLock = new String( "ObjectId._incLock" );
 
     private static final int _genmachine;
     static {

src/test/com/mongodb/ObjectIdTest.java

@@ -129,6 +129,25 @@ public void testFlip(){
         }
 
     }
+
+    /**
+     * Test that within same second, increment value correctly generates ordered ids
+     */
+    @Test
+    public void testInc() {
+        ObjectId prev = null;
+        Date now = new Date();
+        // need to loop more than value of byte, to check that endianness is correct
+        for (int i = 0; i < 1000; ++i) {
+            ObjectId id = new ObjectId(now);
+            assertEquals(id.getTime() / 1000, now.getTime() / 1000);
+            if (prev != null) {
+                assertTrue(prev.compareTo(id) < 0, "Wrong comparison for ids " + prev + " and " + id);
+            }
+            prev = id;
+        }
+    }
+
     public static void main( String args[] )
         throws Exception {
         (new ObjectIdTest()).runConsole();