Some fixes to work better with the services framework

pkg/serialization/lib/src/format.dart

 part of serialization;
 
 /**
  * An abstract class for serialization formats. Subclasses define how data
  * is read or written to a particular output mechanism.
  */
 abstract class Format {
   /**
    * Return true if this format stores primitives in their own area and uses
    * references to them (e.g. [SimpleFlatFormat]) and false if primitives
@@ skipping 34 matching lines @@
    * is the [json] representation of a nested Map structure. The top level has
    * 3 fields, "rules" which may hold a definition of the rules used,
    * "data" which holds the serialized data, and "roots", which holds
    * [Reference] objects indicating the root objects. Note that roots are
    * necessary because the data is organized in the same way as the object
    * structure, it's a list of lists holding self-contained maps which only
    * refer to other parts via [Reference] objects.
    * This effectively defines a custom JSON serialization format, although
    * the details of the format vary depending which rules were used.
    */
-  String generateOutput(Writer w) {
+  generateOutput(Writer w) {

[justinfagnani, 2013/02/09 21:21:56]

Style nit: Why remove the type here? Since the method does return something, at
least Object would show that. Maybe the doc needs to be update where it says
"return it as a String"?

[Jennifer Messerly, 2013/02/10 00:20:35]

Style nit nit :)

If return type is "dynamic", we don't write "dynamic".

"dynamic" return type is more friendly than "Object". I agree about using
"Object" as a parameter though -- it says you aren't going to use any API beyond
object.

However this particular method could be declared to return Map<String, dynamic>.

[Alan Knight, 2013/02/11 23:38:42]

Good point, changed this to return Map<String, dynamic>. 

     var result = {
       "rules" : w.serializedRules(),
       "data" : w.states,
       "roots" : w._rootReferences()
     };
-    return json.stringify(result);
+    return result;
   }
 
   /**
-   * Read a [json] encoded string representing serialized data in this format
+   * Read a [json] compatible representation of serialized data in this format
    * and return the nested Map representation described in [generateOutput]. If
    * the data also includes rule definitions, then these will replace the rules
    * in the [Serialization] for [reader].
    */
-  Map<String, dynamic> read(String input, Reader reader) {
-    var topLevel = json.parse(input);
+  Map<String, dynamic> read(topLevel, Reader reader) {
     var ruleString = topLevel["rules"];
     reader.readRules(ruleString);
     return topLevel;
   }
 }
 
 /**
  * A format for "normal" JSON representation of objects. It stores
  * the fields of the objects as nested maps, and doesn't allow cycles. This can
  * be useful in talking to existing APIs that expect JSON format data. However,
@@ skipping 14 matching lines @@
   final bool storeRoundTripInfo;
 
   /**
    * If we store the rule numbers, what key should we use to store them.
    */
   String ruleIdentifier = "__rule";
 
   SimpleJsonFormat({this.storeRoundTripInfo : false});
 
   /**
-   * Generate output for this format from [w] and return it as a String which
-   * is the [json] representation of a nested Map structure.
+   * Generate output for this format from [w] and return it as
+   * the [json] representation of a nested Map structure.
    */
-  String generateOutput(Writer w) {
+  generateOutput(Writer w) {

[justinfagnani, 2013/02/09 21:21:56]

Map as the return type?

[Jennifer Messerly, 2013/02/10 00:20:35]

I don't think it necessarily returns a map if not selfDescribing or not
storeRoundTripInfo

[Alan Knight, 2013/02/11 23:38:42]

It doesn't necessarily return a Map, part of the changes in this CL are that the
SimpleJsonFormat maps simple types to themselves, so "abc" -> "abc" and 3 -> 3.
At least if not using the round trip info or selfDescribing. Makes the output
more compact and readable, but not very typeable.

     jsonify(w);
-    return json.stringify(w.stateForReference(w._rootReferences().first));
+    var root = w._rootReferences().first;
+    if (root is Reference) root = w.stateForReference(root);
+    if (w.selfDescribing && storeRoundTripInfo) {
+      root = {
+          "rules" : w.serializedRules(),

[Jennifer Messerly, 2013/02/08 04:04:38]

I think I usually just indent this +2, since it has matching end curly. or could
be one line?

[Alan Knight, 2013/02/11 23:38:42]

Because I made those be identifiers rather than literal strings I can't use a
literal map any more :-( So now it's cascaded assignments, which I think does
want four spaces 

+          "data" : root
+        };
+    }
+    return root;
   }
 
   /**
    * Convert the data generated by the rules to have nested maps instead
    * of Reference objects and to add rule numbers if [storeRoundTripInfo]
    * is true.
    */
   jsonify(Writer w) {
     for (var eachRule in w.rules) {
       var ruleData = w.states[eachRule.number];
@@ skipping 21 matching lines @@
    * For one particular entry, which is either a Map or a List, update it
    * to turn References into a nested List/Map.
    */
   jsonifyEntry(map, Writer w) {
     keysAndValues(map).forEach((key, value) {
       if (value is Reference) map[key] = w.stateForReference(value);
     });
   }
 
   /**
    * Read a [json] encoded string representing serialized data in this format

[justinfagnani, 2013/02/09 21:21:56]

string->?

[Alan Knight, 2013/02/11 23:38:42]

Done.

    * and return the Map representation that the reader expects, with top-level
    * entries for "rules", "data", and "roots". Nested lists/maps will be
    * converted into Reference objects. Note that if the data was not written
    * with [storeRoundTripInfo] true this will fail.
    */
-  Map<String, dynamic> read(String input, Reader r) {
-    var data = json.parse(input);
-    var result = {};
-    result["rules"] = null;
-    var ruleData =
-        new List(r.serialization.rules.length).map((x) => []).toList();
-    var top = recursivelyFixUp(data, r, ruleData);
+  Map<String, dynamic> read(data, Reader reader) {
+    var result = new Map();
+    bool looksLikeItHasOurExtraData = data is Map &&
+        data.containsKey("rules") && data.containsKey("data");
+    var rules = looksLikeItHasOurExtraData ? data["rules"] : null;
+    reader.readRules(rules);

[justinfagnani, 2013/02/09 21:21:56]

what happens here if "rules" is just some user data? Could the
serialization-related properties be made "private" with a '_' prefix?

[Jennifer Messerly, 2013/02/10 00:20:35]

This is a good idea.

We should go even further IMO. User should need to specify explicitly if the
format is "self describing JSON" or normal JSON. We shouldn't have any
possibility of incorrectly recognizing the data. "_" prefix is nice but still
not enough, IMO.

[Alan Knight, 2013/02/11 23:38:42]

OK, I changed this to just look at the "selfDescribing" property on the
serialization. Which I think should probably move to the format, but will do
that in a different CL, so added a TODO.

That makes this a little bit more fragile if the user saves it one way and tries
to read it the other, but simplifies the code. So I added an explicit check that

throws if it looks like it was written without selfDescribing set and is being
read with it.

Also changed these to use _rules, _data, _root for this format, and put them
into constants. I only did it for this format because this is the only one that
has the possibility of mixing user data with private serialization data, as
we're trying to write both plain JSON and JSON with our annotations. And it
minimized the extent of the change.

+    var ruleData = new List(reader.serialization.rules.length).
+        map((x) => []).toList();
+    // If our result was a map with rules and data, get the data part. Otherwise
+    // assume that the whole thing is the data.
+    var actualData = (rules == null) ? data : data["data"];
+    var top = recursivelyFixUp(actualData, reader, ruleData);
     result["data"] = ruleData;
     result["roots"] = [top];
     return result;
   }
 
   /**
    * Convert nested references in [data] into [Reference] objects.
    */
-  recursivelyFixUp(data, Reader r, List result) {
+  recursivelyFixUp(input, Reader r, List result) {
+    var data = input;
     if (isPrimitive(data)) {
       result[r._primitiveRule().number].add(data);
       return data;
     }
     var ruleNumber;
+    // If we've added the rule number on as the last item in a list we have
+    // to get rid of it or it will be interpreted as extra data. For a map
+    // the library will be ok, but we need to get rid of the extra key before
+    // the data is shown to the user, so we destructively modify.
     if (data is List) {
-      ruleNumber = data.removeLast();
+      ruleNumber = data.last;
+      data = data.take(data.length -1);
     } else if (data is Map) {
       ruleNumber = data.remove(ruleIdentifier);
     } else {
       throw new SerializationException("Invalid data format");
     }
-    var newData = mapValues(data, (x) => recursivelyFixUp(x, r, result));
+    // Do not use mappedBy or other lazy operations for this. They do not play
+    // well with a function that destructively modifies its arguments.
+    var newData = mapValues(data, (each) => recursivelyFixUp(each, r, result));
     result[ruleNumber].add(newData);
     return new Reference(r, ruleNumber, result[ruleNumber].length - 1);
   }
 }
 
 /**
  * Writes to a simple mostly-flat format. Details are subject to change.
  * Right now this produces a List containing null, num, and String. This is
  * more space-efficient than the map formats, but much less human-readable.
  * Simple usage is to turn this into JSON for transmission.
@@ skipping 230 matching lines @@
       return new Reference(r, a, b);
     }
   }
 
   /** Return the next element from the input. */
   _next(Iterator input) {
     input.moveNext();
     return input.current;
   }
 }