[FileIO] Reduce design JSON file by saving explicit properties only

[grgustaf]

Two good suggestions from James K:

One is we could move toward using the Closure compiler for our code, which forces you to do JavaDoc documentation of your functions and specify how function arguments can be used, so you could enforce "const" return values, etc, as I understand. (It will just make sure that no caller violates the agreement.) Then we would be able to just use a developer contract like we do now, but Closure would enforce it. We wouldn't have to do extra copies for safety.

The other idea was that we could add a setPropertyByReference, or setPropertyObject function... so it would be illegal to call setProperty on an object property, you'll get an error. Instead you have to call this new method, and its documentation will explicitly state that you are giving up ownership of the object you pass in and should not modify it.

This is a little off-topic for this patch, but again your first hunk kind of prompted reevaluating the property reference situation. If you have comments, let me know. I will try again tomorrow to pick some path and resolve this.

If you want to make life super-easy on me, split your patch into one that only writes explicit properties, one that does stringify to ensure we detect matching distinct objects (which I might reject) and one that protects properties with extra copies (which I might reject). Since the ADMNode data isn't really private anyway, we're already relying on the developers to do the right thing.

I appreciate your desire for defensive programming. Really, worrying about optimization is something better guided by profiling, but I can't quite make myself take the leap here.

[grgustaf]

Hmm, it looks like I never committed my comment, and lost it. John, the example you give with a "false positive" is not the problem at hand in this patch. If there were 'aliasing' of objects like you show, then we would already have a more serious bug because the default value itself has been modified, which it should never be.

The real issue here is the danger of a "false negative", where someone sets something back to an object that's essentially identical to the default value, but because it's not the same instance, the equality check fails. But a false negative is not a serious issue in the first place, because the worst that happens is it gets written out to the data file when it was not really necessary. (Currently, EVERYTHING gets written unnecessarily, so it's still a big step forward.)

[grgustaf]

You are probably right that the property getter APIs should return clones. But to be totally safe, the setter APIs should also clone, to be sure the caller doesn't later modify what was given to the API. I can't stand the idea of doing it on both ends though. :)

I'm pretty sure I'm stupid for getting all hung up on this though. Objects are the rare exceptional case for us in properties, so actually copying is going to be rare and not a big hit.

[grgustaf]

Donna, I ran out of time before committing this, so I checked in the patch I was playing around with on top of this to a new "explicit-props" branch. I haven't tested it yet, I was just trying to make myself happier about JSON.stringify by only doing it when necessary. Probably I'm being silly.

Take a look at it if you have time and poke holes. I'll try to merge tomorrow.

[grgustaf]

Donna, thanks for the comments on my flawed patch. I took another crack just now. It's a much better path with the logic very clear now I think, and self-documenting. If you wouldn't mind testing it, I'd appreciate it. I will do it myself if I find the time today.

[grgustaf]

Another improvement along these lines would be to reduce the need for "zone":"default" all over the JSON file. If the parent only has one zone, for instance... there is no need to record the zone in the JSON file. Also, we could probably assume zone "default" if it's not specified, or something along those lines to remove those as well.

[grgustaf]

Thanks, committed.