protostuff ser/der can not keep order of elements in an array which contain null value

[GoogleCodeExporter]

1.Object[] obj=new Object[]{111,222,null,"sss"}
2.after protostuff ser/der. the new object array will become 
{111,222,"sss",null}
3.it seem to protostuff will not ser/der null value,protostuff did not ser the 
index of array to rebuild the array rightly.

What version of the product are you using? On what operating system?
1.0.7

Original issue reported on code.google.com by tully...@gmail.com on 21 Feb 2013 at 3:33

[GoogleCodeExporter]

Nulls are not serialized.  It has nothing to do with ordering.
If you serialize Object[]{null,null,null,"sss"}, you will get:
Object[]{"sss",null,null,null}

Original comment by david.yu...@gmail.com on 21 Feb 2013 at 9:43

[GoogleCodeExporter]

but I hope get {null,null,null,"sss"}
especially when I use this object array to do reflection method invoke.
the order of array is quite important.

Original comment by tully...@gmail.com on 22 Feb 2013 at 11:26

[GoogleCodeExporter]

I was thinking that it is better to remain the order of array elements.
Comparing to other collections like List, Collection, it is common to rely on 
the sequence of elements in arrays, even there are null values in the array.
For example, I used protostuff in Hibernate second level cache, objects cannot 
be deserialized correctly cause some database fields mapped in a array position 
contain null values.

Hope nulls only in arrays can be remained during serialization and 
deserialization.
I have read the source code, and have had simple solution for the issue. and I 
will provide a patch here later.

Original comment by lzh0...@gmail.com on 6 Aug 2014 at 2:22

[GoogleCodeExporter]

here is the patch, just for reference,
in which, allow arrays with null values to be serialized,
keeping null values in middle position, and compact continuous null values.
based on version 1.0.8

Index: IdStrategy.java
===================================================================
--- IdStrategy.java (revision 140405)
+++ IdStrategy.java (working copy)
@@ -519,6 +519,23 @@
                             message.add(value);
                         }
                         break;
+                    case 2:
+                        final Object nullValue = 
ObjectSchema.readObjectFrom(input,
+                                this, message, IdStrategy.this);
+                        if(nullValue instanceof Number)
+                        {
+                            // fill continuous null values
+                            int count = ((Number) nullValue).intValue();
+                            for (int i = 0; i < count; i++)
+                            {
+                                message.add(null);
+                            }
+                        }
+                        else
+                        {
+                            throw new ProtostuffException("Corrupt input.");
+                        }
+                        break;
                     default:
                         throw new ProtostuffException("Corrupt input.");
                 }
@@ -603,7 +620,8 @@

         public void writeTo(Output output, Object message) throws IOException
         {
-            for(int i = 0, len = Array.getLength(message); i < len; i++)
+            int len = Array.getLength(message);
+            for(int i = 0; i < len; i++)
             {
                 final Object value = Array.get(message, i);
                 if(value != null)
@@ -610,6 +628,21 @@
                 {
                     output.writeObject(1, value, DYNAMIC_VALUE_SCHEMA, true);
                 }
+                else
+                {
+                    // counting continuous null values
+                    int count = 1;
+                    for(; i + count < len; count++)
+                    {
+                        if(Array.get(message, i + count) != null)
+                        {
+                            break;
+                        }
+                    }
+
+                    i += count - 1;
+                    output.writeObject(2, count, DYNAMIC_VALUE_SCHEMA, true);
+                }
             }
         }
     };

Original comment by lzh0...@gmail.com on 6 Aug 2014 at 3:34

[GoogleCodeExporter]

Good idea.  The problem with this approach is it will not work with the text 
formats like json where writing repeated values needs to be contiguous.
The output would be something like this when the array is [1,2,null,3,null,4,5]:
{
  1:[1,2],
  2:1,
  1:[3]
  2:1,
  1:[4,5]
}

Another approach could be record all the holes as we write the values and then 
write the holes (offsets).  This way we the output would be:
{
  1:[1,2,0,3,0,4,5]
  2:[2,4]
}
On deser, we simply replace the element at offset with null.
For backwards incompatible changes like these, 1.1.x would be a good target for 
the patch.
Take a look at ArraySchemas in 1.1.x.  They're a little bit faster to write 
arrays.
This could be applied there and also in IdStrategy (the one you just patched)

Original comment by david.yu...@gmail.com on 6 Aug 2014 at 10:40

• Changed state: Accepted

[GoogleCodeExporter]

On the other hand, there is overhead in creating an array/list just to record 
the offsets (during serialization).

I guess if you're only using/targetting the native format, your approach would 
be more efficient.

We could maybe add a flag (in RuntimeEnv) to enable this.

Original comment by david.yu...@gmail.com on 6 Aug 2014 at 12:16

[GoogleCodeExporter]

Thanks for your quick replies. I will have a look at that in 1.1.x.

For the issue, at first, I was considering if null should be treated as a basic 
type,
like types defined in com.dyuproject.protostuff.runtime.RuntimeFieldFactory.
I noticed that the zero has not been used, may it be mapped to null?

Original comment by lzh0...@gmail.com on 7 Aug 2014 at 5:58

[GoogleCodeExporter]

Zero cannot be used.  The method Input.readFieldNumber returns that on 
end-of-message.

With the way protostuff is designed you cannot add it as a basic type.

Original comment by david.yu...@gmail.com on 7 Aug 2014 at 8:36

[GoogleCodeExporter]

Yeah, I tried, and Zero did not work.
But I used another vacancy(ID_NULL = 53), to write a null in an array.
Finally, I make it work. Here is the patch, just for understanding what I did.
Sorry to post large piece of code stuff here, 
because I cannot attach a file for some reasons.
Of course, this approach generates more bytes than the one I gave previously.

Index: IdStrategy.java
===================================================================
--- IdStrategy.java (revision 141320)
+++ IdStrategy.java (working copy)
@@ -606,7 +606,7 @@
             for(int i = 0, len = Array.getLength(message); i < len; i++)
             {
                 final Object value = Array.get(message, i);
-                if(value != null)
+                //if(value != null)
                 {
                     output.writeObject(1, value, DYNAMIC_VALUE_SCHEMA, true);
                 }
Index: ObjectSchema.java
===================================================================
--- ObjectSchema.java   (revision 141320)
+++ ObjectSchema.java   (working copy)
@@ -16,6 +16,7 @@

 import static com.dyuproject.protostuff.runtime.RuntimeFieldFactory.BIGDECIMAL;
 import static com.dyuproject.protostuff.runtime.RuntimeFieldFactory.BIGINTEGER;
+import static com.dyuproject.protostuff.runtime.RuntimeFieldFactory.NULL;
 import static com.dyuproject.protostuff.runtime.RuntimeFieldFactory.BOOL;
 import static com.dyuproject.protostuff.runtime.RuntimeFieldFactory.BYTE;
 import static com.dyuproject.protostuff.runtime.RuntimeFieldFactory.BYTES;
@@ -28,6 +29,7 @@
 import static com.dyuproject.protostuff.runtime.RuntimeFieldFactory.ID_ARRAY_MAPPED;
 import static com.dyuproject.protostuff.runtime.RuntimeFieldFactory.ID_BIGDECIMAL;
 import static com.dyuproject.protostuff.runtime.RuntimeFieldFactory.ID_BIGINTEGER;
+import static com.dyuproject.protostuff.runtime.RuntimeFieldFactory.ID_NULL;
 import static com.dyuproject.protostuff.runtime.RuntimeFieldFactory.ID_BOOL;
 import static com.dyuproject.protostuff.runtime.RuntimeFieldFactory.ID_BYTE;
 import static com.dyuproject.protostuff.runtime.RuntimeFieldFactory.ID_BYTES;
@@ -403,6 +405,9 @@
         final int number = input.readFieldNumber(schema);
         switch(number)
         {
+            case ID_NULL:
+                value = NULL.readFrom(input);
+                break;
             case ID_BOOL:
                 value = BOOL.readFrom(input);
                 break;
@@ -647,7 +652,7 @@
     static void writeObjectTo(Output output, Object value,
             Schema<?> currentSchema, IdStrategy strategy) throws IOException
     {
-        final Class<Object> clazz = (Class<Object>)value.getClass();
+        final Class<Object> clazz = (Class<Object>)(value != null ? 
value.getClass() : Void.class);

         final Delegate<Object> delegate = strategy.tryWriteDelegateIdTo(output, 
                 ID_DELEGATE, clazz);
Index: RuntimeFieldFactory.java
===================================================================
--- RuntimeFieldFactory.java    (revision 141320)
+++ RuntimeFieldFactory.java    (working copy)
@@ -65,7 +65,8 @@
         ID_POLYMORPHIC_COLLECTION = 28, 
         ID_POLYMORPHIC_MAP = 29, 
         ID_DELEGATE = 30, 
-        ID_THROWABLE = 52, 
+        ID_THROWABLE = 52,
+        ID_NULL = 53,

         // pojo fields limited to 126 if not explicitly using @Tag annotations
         ID_POJO = 127;
@@ -102,6 +103,7 @@

     static final RuntimeFieldFactory<BigDecimal> BIGDECIMAL;
     static final RuntimeFieldFactory<BigInteger> BIGINTEGER;
+    static final RuntimeFieldFactory<Object> NULL;
     static final RuntimeFieldFactory<Boolean> BOOL;
     static final RuntimeFieldFactory<Byte> BYTE;
     static final RuntimeFieldFactory<ByteString> BYTES;
@@ -130,6 +132,7 @@
         {
             BIGDECIMAL = RuntimeUnsafeFieldFactory.BIGDECIMAL;
             BIGINTEGER = RuntimeUnsafeFieldFactory.BIGINTEGER;
+            NULL = RuntimeUnsafeFieldFactory.NULL;
             BOOL = RuntimeUnsafeFieldFactory.BOOL;
             BYTE = RuntimeUnsafeFieldFactory.BYTE;
             BYTES = RuntimeUnsafeFieldFactory.BYTES;
@@ -154,6 +157,7 @@
         {
             BIGDECIMAL = RuntimeReflectionFieldFactory.BIGDECIMAL;
             BIGINTEGER = RuntimeReflectionFieldFactory.BIGINTEGER;
+            NULL = null; // TODO: not finished, just to avoid compiler's 
warnings
             BOOL = RuntimeReflectionFieldFactory.BOOL;
             BYTE = RuntimeReflectionFieldFactory.BYTE;
             BYTES = RuntimeReflectionFieldFactory.BYTES;
@@ -179,6 +183,7 @@
                     RuntimeCollectionFieldFactory.getFactory() : 
                         RuntimeRepeatedFieldFactory.getFactory();

+        __inlineValues.put(Void.class.getName(), 
RuntimeUnsafeFieldFactory.NULL);
         __inlineValues.put(Integer.TYPE.getName(), INT32);
         __inlineValues.put(Integer.class.getName(), INT32);
         __inlineValues.put(Long.TYPE.getName(), INT64);
Index: RuntimeUnsafeFieldFactory.java
===================================================================
--- RuntimeUnsafeFieldFactory.java  (revision 141320)
+++ RuntimeUnsafeFieldFactory.java  (working copy)
@@ -16,6 +16,7 @@

 import static com.dyuproject.protostuff.runtime.RuntimeFieldFactory.ID_BIGDECIMAL;
 import static com.dyuproject.protostuff.runtime.RuntimeFieldFactory.ID_BIGINTEGER;
+import static com.dyuproject.protostuff.runtime.RuntimeFieldFactory.ID_NULL;
 import static com.dyuproject.protostuff.runtime.RuntimeFieldFactory.ID_BOOL;
 import static com.dyuproject.protostuff.runtime.RuntimeFieldFactory.ID_BYTE;
 import static com.dyuproject.protostuff.runtime.RuntimeFieldFactory.ID_BYTES;
@@ -331,6 +332,66 @@
         }
     };

+    public static final RuntimeFieldFactory<Object> NULL = new 
RuntimeFieldFactory<Object>(ID_NULL)
+    {
+        public <T> Field<T> create(int number, java.lang.String name, 
+                final java.lang.reflect.Field f, IdStrategy strategy)
+        {
+            final boolean primitive = f.getType().isPrimitive();
+            final long offset = us.objectFieldOffset(f);
+            return new Field<T>(FieldType.MESSAGE, number, name, 
+                    f.getAnnotation(Tag.class))
+            {
+                public void mergeFrom(Input input, T message) throws 
IOException
+                {
+                    if(primitive)
+                        us.putInt(message, offset, input.readInt32());
+                    else
+                        us.putObject(message, offset, 
Integer.valueOf(input.readInt32()));
+                }
+                public void writeTo(Output output, T message) throws 
IOException
+                {
+                    if(primitive)
+                        output.writeInt32(number, us.getInt(message, offset), 
false);
+                    else
+                    {
+                        Integer value = (Integer)us.getObject(message, offset);
+                        if(value!=null)
+                            output.writeInt32(number, value.intValue(), false);
+                    }
+                }
+                public void transfer(Pipe pipe, Input input, Output output, 
+                        boolean repeated) throws IOException
+                {
+                    output.writeInt32(number, input.readInt32(), repeated);
+                }
+            };
+        }
+        public void transfer(Pipe pipe, Input input, Output output, int 
number, 
+                boolean repeated) throws IOException
+        {
+            output.writeInt32(number, input.readInt32(), repeated);
+        }
+        public Object readFrom(Input input) throws IOException
+        {
+            Integer.valueOf(input.readInt32());
+            return null;
+        }
+        public void writeTo(Output output, int number, Object value, boolean 
repeated) 
+        throws IOException
+        {
+            output.writeInt32(number, 0, repeated);
+        }
+        public FieldType getFieldType()
+        {
+            return FieldType.MESSAGE;
+        }
+        public Class<?> typeClass()
+        {
+            return Integer.class;
+        }
+    };
+
     public static final RuntimeFieldFactory<Long> INT64 = new RuntimeFieldFactory<Long>(ID_INT64)
     {
         public <T> Field<T> create(int number, java.lang.String name, 

Original comment by lzh0...@gmail.com on 7 Aug 2014 at 11:19

[GoogleCodeExporter]

BTW: the Output object does not support writing null value to the buffer,
I used writeInt32 instead. I thought it would be needed.

And I have had a glance on ArraySchemas, it is a bit complicated. :)
Anyway, thanks for your great work.

Adding a Null type may be better than the second approach you mentioned before,
for it may enable us to generate text formats more easily?

BTW: Would you have a plan that when the version 1.1.x will be released?
not hasty, plz on your pace.

Original comment by lzh0...@gmail.com on 7 Aug 2014 at 11:35

[GoogleCodeExporter]

I think your initial patch is better.
That patch only addresses dynamic fields (interface/object).
For all other fields, nulls are not serialized.

"Adding a Null type may be better than the second approach you mentioned before,
for it may enable us to generate text formats more easily?"

With the current IO api, it would not work if the field is an array/list 
because json expects a repeated field to be contiguous.  You cannot write a 
null in the middle of writing the element.

To make it work, the io api and json impl need to be changed.
There would be 2 code paths to writing null.
1. The one that you initially proposed.
2. Adding public boolean Output.writeNull() for json.
If false is returned, the codepath will be 1.

This api change will support null on Array/Collection/Map only.
And the element cannot be a boxed type on static schemas 
(List<Integer>/Boolean[]) because Input.readInt32 and Input.readBool return 
primitive values.
Note that Boolean[]/int[] are static schemas in 1.1.x, they are dynamic (array 
schema) on 1.0.x

Original comment by david.yu...@gmail.com on 8 Aug 2014 at 2:40

[GoogleCodeExporter]

Yes, you are right! 
The latter idea involves too many modifications.

I do not know much about the thole code structure,
maybe I need to dive into the code to learn more.

The problem is only with arrays,
no need to save nulls in Lists or Collections.

It's fine to apply the first patch. thanks.

Original comment by lzh0...@gmail.com on 8 Aug 2014 at 7:11

[GoogleCodeExporter]

Fixed in 1.1.x 
(https://github.com/protostuff/protostuff/commit/e2acee82a9e1b9b874f6c73b72541fd
08667f1f3)

Sorry for the delay.  Busy much :-/

Original comment by david.yu...@gmail.com on 28 Sep 2014 at 6:20

[GoogleCodeExporter]

To activate:
-Dprotostuff.runtime.allow_null_array_element=true

Works only on protostuff/graph/protobuf formats.

Original comment by david.yu...@gmail.com on 28 Sep 2014 at 6:22

[GoogleCodeExporter]

Thanks a lot

Original comment by tully...@gmail.com on 29 Sep 2014 at 8:55