When serialising objects via OutStream.writeObj() I really like the idea of using the skipDefaults option:

• "skipDefaults": Bool specifies if we should skip fields at their default 
                  values. Field values are compared according to the equals 
                  method. Default is false.

It has the potential to save huge amounts of text and readObj processing. I do similar in Morphia - albeit only null's are skipped (though I have contemplated going the full defVal hog!).

Anyway, say I want to serialise this object:

@Serializable
class MyObj {
    Str name
    new make(Str name) { this.name = name }
}

...

list := [MyObj("bongo")]
fog  := Buf().writeObj(list).flip.readAllStr

echo(fog) // --> acme::MyObj[ acme::MyObj { name="bongo" } ]

That's cool, but when I add the skipDefaults option I get an Err:

list := [MyObj("bongo")]
fog  := Buf().writeObj(list, ["skipDefaults":true]).flip.readAllStr

// --> sys::Err: Type missing "make" or "defVal" slots: acme::MyObj

I would argue that any object that can be serialised without skipDefaults should also be serialised with skipDefaults.

If MyObj doesn't have a make() or defVal slot, then it clearly doesn't have a default and can't be skipped. I don't expect an error to be thrown, or be forced to add defVals to all my serialisable objects.

Furthermore I would say that, for serialisation at least, the default value for nullable object types is null and make() / defVal should only be inspected for non-nullable object types.

Note that all these problems are bourne out of the one line from fanx.serial.ObjEncoder#writeComplex which reads:

if (skipDefaults) defObj = FanObj.typeof(obj).make();

which could be something like:

if (skipDefaults) {
    Type typeof := FanObj.typeof(obj);
    if (!typeof.isNullable()) {
        try {
            defObj = typeof.make();
        } catch (Exception e) { /* meh /* }
    }
}

Note that for large lists / graphs, skipDefaults seems to make excessive use of typeof.make() creating many objects along the way. It would be cool if each ObjEncoder had it's own cache map of Types to defVals.

I'm not following this logic, because the specification of Serializable objects requires them to have a no-arg make constructor.

The specification of Serializable objects says:

A serializable object must support a make constructor which either takes no parameters or takes an it-block

Which I'm happy with - because obviously de-serialisation needs a way to re-construct the object!

So lets re-state the example with an it-block ctor, and note that serialisation works perfectly:

@Serializable
class MyObj {
    Str name
    new make(Str name) { this.name = name }
}

list := [MyObj { it.name = "bongo"}]
fog  := Buf().writeObj(list).flip.readAllStr
	
echo(fog) // --> acme::MyObj[ acme::MyObj { name="bongo" } ]

But if I add the skipDefaults option, then I get an error:

list := [MyObj { it.name = "bongo"}]
fog  := Buf().writeObj(list, ["skipDefaults":true]).flip.readAllStr

sys::Err: Type missing 'make' or 'defVal' slots: acme::MyObj
  fan.sys.Type.make (Type.java:256)
  fan.sys.ClassType.make (ClassType.java:110)
  fan.sys.Type.make (Type.java:236)
  fanx.serial.ObjEncoder.writeComplex (ObjEncoder.java:107)
  fanx.serial.ObjEncoder.writeObj (ObjEncoder.java:77)
  fanx.serial.ObjEncoder.writeList (ObjEncoder.java:219)
  fan.sys.List.encode (List.java:1120)
  fanx.serial.ObjEncoder.writeObj (ObjEncoder.java:66)
  fan.sys.OutStream.writeObj (OutStream.java:314)

It is this part I'm not happy with, because any object that can be serialised without skipDefaults should also be serialised with skipDefaults.

Your code doesn't compile - did you mean that MyObj has an it-block constructor a constructor that takes a required Str parameter?

I mean an it-block ctor.

My bad, I keep cut'n'pasting the wrong code. 3rd time lucky, myObj looks like:

@Serializable
class MyObj {
    Str name
    new make(|This| f) { f(this) }
}

Serialisation works normally, but not with skipDefaults:

list := [MyObj { it.name = "bongo"}]

fog1 := Buf().writeObj(list)                        // --> okay
fog2 := Buf().writeObj(list, ["skipDefaults":true]) // --> Err

Ok, that makes sense now and I understand the issue. I pushed a fix

Thanks Brian, that works nicely. Although the implementation creates a new "default" object every time it writes a complex object - which strikes me as rather inefficient.

For example, given I have a simple class with a no-args ctor:

@Serializable
class MyObj {
    Str x := "foo"
	
    new make() { }
}

When I serialise a List of 100 MyObjs, then serialisation creates an extra 100 (unwanted) default instances of MyObj!

A better way would be to cache the default objects so they're only created once per serialisation run:

diff -r 4e594b4f9989 src/sys/java/fanx/serial/ObjEncoder.java
--- a/src/sys/java/fanx/serial/ObjEncoder.java	Fri Oct 13 13:20:22 2017 -0400
+++ b/src/sys/java/fanx/serial/ObjEncoder.java	Sun Oct 15 15:28:12 2017 +0100
@@ -8,6 +8,7 @@
 package fanx.serial;
 
 import java.math.BigDecimal;
+import java.util.HashMap;
 import java.util.Iterator;
 import java.util.Map.Entry;
 import fan.sys.*;
@@ -106,9 +107,17 @@
     Object defObj = null;
     if (skipDefaults)
     {
-      // attempt to instantiate default object for type,
-      // this will fail if complex has it-block ctor
-      try { defObj = FanObj.typeof(obj).make(); } catch (Exception e) {}
+      Type defType = FanObj.typeof(obj).toNonNullable();
+      if (defaultObjs.containsKey(defType))
+        // use cached defVal if it exists
+        defObj = defaultObjs.get(defType);
+      else
+      {
+        // else attempt to instantiate default object for type,
+        // this will fail if complex has it-block ctor
+        try { defObj = defType.make(); } catch (Exception e) {}
+        defaultObjs.put(defType, defObj);
+      }
     }
 
     List fields = type.fields();
@@ -342,6 +351,8 @@
     indent = option(options, "indent", indent);
     skipDefaults = option(options, "skipDefaults", skipDefaults);
     skipErrors = option(options, "skipErrors", skipErrors);
+    if (skipDefaults)
+      defaultObjs = new HashMap();
   }
 
   private static int option(Map options, String name, int def)
@@ -368,5 +379,6 @@
   boolean skipDefaults = false;
   boolean skipErrors = false;
   Type curFieldType;
+  HashMap defaultObjs;
 
 }
\ No newline at end of file

A quick test that counts the number of instances created shows that now, only the 1 default instance is created:

using concurrent::AtomicInt

class TestSkipDefaults : Test {

    Void testSkipDefaults() {
        // create a list of 100 objects
        list := MyObj[,]
        100.times { list.add(MyObj { }) }

        // serialise list
        MyObj.instanceCount.val = 0
        Buf().writeObj(list, ["skipDefaults":true])

        // assert that only 1 defVal instance is created when serialising object 
        verifyEq(MyObj.instanceCount.val, 1)
    }
    
    static Void main(Str[] args) {
        TestSkipDefaults().testSkipDefaults
    }
}

@Serializable
class MyObj {
    static const AtomicInt instanceCount := AtomicInt()
    
    new make() {
        instanceCount.increment
    }
}

Whereas before the patch, the test would fail with Test failed: 100 [sys::Int] != 1 [sys::Int]

lthough the implementation creates a new "default" object every time it writes a complex object - which strikes me as rather inefficient.

I considered it, but I am not sure in most cases it actually makes sense to create a whole hash table. Creating temp throw away objects in the nursery pool is actually very cheap, so I don't typically use a cache for cases like that

not sure in most cases it actually makes sense to create a whole hash table

Not sure if you saw in the patch, but the HashTable is only created if skipDefaults is true.

Creating temp throw away objects in the nursery pool is actually very cheap

That may be, but I'm pretty sure that not creating the objects in the first place is even cheaper!