Add policy hooks to std.json to support custom extensions #6059

marler8997 · 2018-01-23T23:27:45Z

This is an exploration on enhancing std.json. The idea is to add support to preserve order of object members. An example of where this is useful is if you'd like to parse some JSON and then later write it somewhere, but you'd like to maintain the original order of the object members.

Note that the techniques used in this PR could also be reused for adding other optional features. It allows the application to pass in a Policy structure that can enable/disable features. If you wanted to add another feature, say, "enableComments", this could be added as a Policy setting as well. This technique could also be ported to the upcoming std.data.json.

Preserving field order is beneficial for any kind of tool that modifies JSON files. You could imagine tools that modify dub.json files, DMD JSON output files, sublime/vscode config files. If a tool like this couldn't guarantee preserving the order of object fields, then this would make the tools much less usable.

NOTE: This PR is not fully documented. The work to fully document is pending approval that this is a desirable enhancement.

dlang-bot · 2018-01-23T23:27:45Z

Thanks for your pull request, @marler8997! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

smaller, focused PRs are easier to review than big ones
try not to mix up refactoring or style changes with bug fixes or feature enhancements
provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

timotheecour · 2018-01-23T23:33:54Z

std/json.d

+                foreach (ref member; members)
+                if (name == member.name)
+                    return member.value;
+                assert(0, "JSON object does not contain member named: " ~ name);


enforce for external / environmental errors

timotheecour · 2018-01-23T23:34:04Z

std/json.d

-struct JSONValue
+alias JSONValue = JSONValueTemplate!DefaultPolicy;
+
+private struct JSONValueTemplate(alias Policy)


not private

timotheecour · 2018-01-23T23:35:45Z

std/json.d

+            ref auto opIndex(in char[] name) const
+            {
+                foreach (ref member; members)
+                if (name == member.name)


missing indent

timotheecour · 2018-01-23T23:37:08Z

std/json.d

+                    return member.value;
+                assert(0, "JSON object does not contain member named: " ~ name);
+            }
+            ref auto opIndexAssign(Value value, string name)


ref auto => u probably wanna return *this ?
or how about using same signature as in std.json:
pure void opIndexAssign(ref Value value, string key);

timotheecour · 2018-01-23T23:38:42Z

std/json.d

+    alias Value = typeof(this);
+    static if (Policy.preserveMemberOrder)
+    {
+        struct JSONObject


optional: this is basically an AA with ordered fields; it's re-usable; why not expose it as a generic:

struct AAOrdered(K,V){ ... }

timotheecour

.

timotheecour · 2018-01-24T00:22:43Z

std/json.d

+    enum preserveMemberOrder = true;
+}
+
+struct OrderedAA(K,V)


move to std.array? (eg has assocArray etc)

timotheecour

.

timotheecour

.

timotheecour · 2018-01-24T00:25:12Z

std/json.d

+    }
+    private Member[] members;
+    size_t length() const { return members.length; }
+    inout(V)* opIn(in char[] name) inout


char[] instead of K ? (do we need specialization for case K==string to allow char[] in that case?)
=> a unitest with orderedAA!(int, double) would've caught this bug

timotheecour · 2018-01-24T00:32:34Z

std/json.d

+            return &member.value;
+        return null;
+    }
+    ref auto opIndex(in char[] name) const


timotheecour · 2018-01-24T00:33:37Z

std/json.d

+            return member.value;
+        assert(0, "OrderedAA does not contain member named: " ~ name);
+    }
+    void opIndexAssign(V value, K name)


s/name/key/ everywhere in this struct

timotheecour · 2018-01-24T00:54:17Z

std/json.d

+        foreach (ref member; members)
+        if (name == member.name)
+            return member.value;
+        assert(0, "OrderedAA does not contain member named: " ~ name);


enforce!JSONException ?

timotheecour · 2018-01-24T01:01:40Z

std/json.d

                    {
-                        foreach (name; names)
+                        foreach (member; obj.members)


foreach (key, value; obj) {...}

Actually, in the DefaultPolicy case, names is just an array of strings, not an associative array.

timotheecour · 2018-01-24T01:15:12Z

std/json.d

                () @trusted { store.object = aa; }();
            }
        }
+        else static if (Policy.preserveMemberOrder && is(T == JSONObject))


that seems restrictive compared to other policy which didn't only accept JSONObject but also things that converted to it via Key : string and Value(value)

Maybe. It seems a bit odd to take something that is unordered and then copy it to something that is ordered. Not sure.

An application could always do the conversion themselves to. This is something that doesn't have to be implemented in the library (AFAICT).

no, it could be AAordered!(Key, Value) with Key and Value convertible to destination types

timotheecour · 2018-01-24T01:23:12Z

std/array.d

+        foreach (ref member; array)
+        if (key == member.key)
+            return member.value;
+        assert(0, "OrderedAA does not contain value with key: " ~ key);


~ key will fail with non-string types

unittest to catch that eg OrderedAA!(int, double)

not sure if assert is appropriate; it probably should be same type of error as corresponding error in an associative array

timotheecour · 2018-01-24T01:31:36Z

std/array.d

+        K key;
+        V value;
+    }
+    private KeyValue[] array;


is that more efficient with Appender!KeyValue?

timotheecour · 2018-01-24T01:45:22Z

std/array.d

+
+struct OrderedAA(K,V)
+{
+    struct KeyValue


static struct? to avoid hidden __this pointer

JackStouffer · 2018-01-24T15:44:09Z

This seems like a misapplication of JSON. JSON objects are designed to be unordered; from the spec:

An object is an unordered set of name/value pairs.

My inclination is that if what you're doing with JSON requires you to keep the same order of keys, then there's something wrong with your design.

quickfur · 2018-01-25T19:47:25Z

My main hesitation with this feature is that it doesn't conform to the JSON spec. It's one thing to make it possible for user code to do whatever it wants, which is why I gravitate towards a SAX-style / stream-based parser, because that enables user code to implement order-dependence without actually explicitly doing that in Phobos. But it's another thing for Phobos to explicitly support something that, strictly speaking, doesn't follow the JSON spec. I'm all for enabling the user, but I'm not a fan of vendor-specific extensions (which is basically what this PR is) that eventually leads to fragmentation.

And just to be clear, I'm not complaining merely because I like being pedantic about spec conformance; the underlying issue is that we're trying to impose order upon a structure that has been explicitly defined to be unordered. This, to me, is a code smell. In my experience, this style of coding often has or leads to fragility, unhandled corner cases, exceptional behaviours, and lots of little crufty things like that, that are symptomatic of an inherent impedance mismatch between what the code does and what the data is supposed to represent. I don't feel strongly enough about std.json to actively blockade this PR, the others can merge if they think it's a good idea, but I'll just say that I won't actively support this direction.

marler8997 · 2018-01-25T19:57:45Z

@quickfur thanks for explaining yourself, I see where you are coming from now. I'm thinking a better solution might be to provide a mechanism to allow the user to override the type used for JSONObject with the default being JSONValue[string]. If you're ok with this I'll go ahead and modify this PR to do that.

marler8997 · 2018-01-26T23:56:42Z

Pinging @quickfur and @JackStouffer

I've refactored the change. Now it allows you to override the default type for JSON objects, but does not provide any of these types. This allows an application to customize the parser to their needs without the parser providing any non-spec functionality out of the box. Note that I've included the OrderedAA as a unittest, but not as a publicly accessible structure. Please have a look and let us know if you are OK with this design.

timotheecour · 2018-01-28T08:48:25Z

std/json.d

    {
        auto o = this.objectNoRef;
-        return *enforce!JSONException(k in o,
-                                        "Key not found: " ~ k);
+        static if (hasMember!(Policy, "ObjectType"))


DRY:

static if (hasMember!(Policy, "ObjectType")) auto ok=o.opIn(k); else auto ok=k in o; return *enforce!JSONException(ok, "Key not found: " ~ k);

Just discovered that opIn_r was a thing...now that I know about it, this code only has the one case now :)

timotheecour · 2018-01-28T08:52:51Z

std/json.d

+private struct DefaultPolicy
+{
+    enum sortObjectMembers = true;
+}


you can simplify a lot the rest of the code by always requiring a template ObjectType to Policy:

private struct DefaultPolicy { enum sortObjectMembers = true; alias ObjectType(Value)=Value[string]; }

Which code can be simplified?

timotheecour · 2018-01-28T08:58:27Z

std/json.d

@@ -1110,6 +1165,91 @@ if (isInputRange!T && !isInfinite!T && isSomeChar!(ElementEncodingType!T))
    assert(json["key1"]["key2"].integer == 1);
 }

+version(unittest)
+{
+    struct OrderedAA(Key, Value)


I think others (not me...) were concerned about exposing json with ordered_fields, but I didn't see objections to exposing OrderedAA in std.array; as you currently wrote it by hiding OrderedAA inside a unittest block, this'll require someone who wants to use json with ordered_fields to reimplement OrderedAA below, which is not DRY and much worse than exposing json with ordered_fields.

In short: using json with ordered_fields should be as simple as:

import std.array:OrderedAA; static struct OrderedAAPolicy { alias ObjectType = OrderedAA; enum sortObjectMembers = false; } auto json1 = parseJSONTemplate!OrderedAAPolicy(...);

Agreed. However, now that std.json does not depend on having OrderedAA defined, these can be 2 separate changes. When I make a separate PR to add OrderedAA to std.array, we can remove this instance I've included in the unittest and swap it out for that.

Note that by separating these changes into distinct PRs, it makes this one easier to accept on it's own. Once this one gets accepted, we can start using this feature in dmd by simply including our own definition of OrderedAA until we can add it to std.array. I'm guessing that adding OrderedAA is going to be a fair amount of work with lots of revision and documentation so I would rather not have this change depend on that one.

…der.

marler8997 · 2018-01-28T10:13:39Z

Per @quickfur's comments, I've created a PR to remove the object field sorting that is currently present in std.json: #6084

wilzbach · 2018-01-29T12:06:39Z

BTW here's what the ECMA15 spec says - it requires insertion order to be preserved (it didn't before).

order of enumerating the properties is not specified but must conform to the rules specified below.

Let keys be a new empty List.
For each own property key P of O that is an integer index, in ascending numeric index order
Add P as the last element of keys.
For each own property key P of O that is a String but is not an integer index, in property creation order
Add P as the last element of keys.
For each own property key P of O that is a Symbol, in property creation order
Add P as the last element of keys.
Return keys.

Granted, JS and JSON are different things, but people mix them up often enough, s.t. preserving insertion order is an interesting use case.

marler8997 · 2018-01-29T17:15:03Z

BTW here's what the ECMA15 spec says - it requires insertion order to be preserved (it didn't before).

Can't say I'm surprised by this change to ECMA15 :) Based on @quickfur 's comments I refactored this change to remove the option to preserve order, maybe I should bring it back in? I've currently been working on modifying this PR to appease @quickfur but after I refactored the change based on his comments he has been silent. This seems to be a pattern among some of dlang's developers and definitely not a good one.

quickfur · 2018-01-29T18:18:37Z

@marler8997 Please understand that (1) I'm not paid for doing this; (2) I'm doing this entirely out of my free time; (3) my free time is very scant (my weekends are usually completely booked); (4) Phobos has tons of other PRs and things that I can look at; and (5) I'm not under contract to respond to stuff by some deadline. Basically, the only reason I even respond at all is only out of goodwill. So I don't understand why a pause of 4 days is unacceptable. If you paid me to work on this full-time, then I would respond within a couple of hours, but IMO the accusatory tone in your comment is uncalled for. If you feel the response rate of Phobos devs is not up to your standard, then perhaps you should consider donating to the D foundation to hire full-time programmers to work on this.

marler8997 · 2018-01-29T18:35:21Z

@quickfur

Sorry for offending you. I don't mean to say that you NEED to be more responsive in general. My point was that if you decide to engage in a PR with criticism that you should follow through when the person takes your criticism and spends their time refactoring their work based on your comments. Of course you're not under any obligation to respond quickly, but I do think it's inconsiderate to criticize someone else's work and then fail to respond in a timely manner after they've gone out of their way to redo all their work based on that criticism.

quickfur · 2018-01-29T19:23:38Z

Well, your last change was 3 days ago, and as I said, my weekend are booked, so if you subtract that, that's really only 1 day. I don't think that's unreasonable at all, given that there's a whole bunch of other stuff I was reviewing, and my 1 day is really a lot less since I have a full-time job and can't be spending all day working on Phobos -- my employer would fire me. If I didn't respond for a week, then you may have a point, but if your expectations are that I have to respond instantly, then perhaps I should refrain from reviewing your PRs in the future since I can't keep up with your expectations.

marler8997 · 2018-01-29T19:28:38Z

@quickfur I'll know in the future to expect it to take up to a week for you to respond. Glad we've got that cleared up. Let's continue the more useful discussion at hand, namely, what to do with this PR.

quickfur

OK, this is a really big change. I'm going to have to defer to @andralex and the other reviewers whether or not to go with this.

My main concern is that this is a lot of change adding a lot of complexity just to support one use case (support the ability to swap in a different implementation of JSONObject). Surely there's a simpler way to implement this that doesn't require a changeset of this size? E.g., if JSONObject were made an interface instead, so that different concrete types can be hooked in without a renaming that affects everything else in JSONValue. Then it would just be a matter of having the user supply a factory method of some sort to construct the desired object type. There are probably other ways to implement this that doesn't entail such a massive change.

With a change this big for such a small feature, I'm afraid that even if I were to approve this PR, @andralex is likely to veto it.

quickfur · 2018-01-29T19:32:46Z

std/json.d

+    alias MyObjectType(K,V) = OrderedAA!(K,V);
+}
+alias MyJSONValue = JSONValueTemplate!MyPolicy;
+------------


The example here really should be in a ddoc'd unittest. And it should show how to actually use JSONValueTemplate, not just in how to instantiate it, but how to use it with the rest of the module, e.g., to configure it with a different JSONObject type, etc..

Agreed. Was going to wait to fully document until a general "approved" consensus was reached.

marler8997 · 2018-01-29T22:41:50Z

I'm going to have to disagree on the size of this change. Most of the lines the actual change are unit tests/documentation and renaming JSONValue to Value (internally). The only functional change is redifining the implementation of JSONValue, toJSON and parseJSON as templates with hooks to override functionality.

Also, saying that this only adds support for "one use case' is just not true. The change is providing a couple hooks to override to parts of the implementation, along with a framework for providing more hooks in the future.

As for @andralex, he may or may not approve but I'd be more interested in every one giving their own opinion rather than people trying to guess what others might think.

quickfur · 2018-01-29T22:44:44Z

OK, then let's let @andralex decide whether or not to go ahead with this.

andralex · 2018-01-30T00:08:13Z

On a cursory look I see two issues with this:

Preserving order of fields is mentioned repeatedly as an important application, but relying on order of fields is specifically listed as an antipattern in JSON canon - see e.g. https://stackoverflow.com/questions/3948206/json-order-mixed-up
std.json is generally disliked. This feature addition does not improve on any of the dislikes.

It don't see how this can be brought to a solid addition.

marler8997 requested a review from CyberShadow as a code owner January 23, 2018 23:27

timotheecour reviewed Jan 23, 2018

View reviewed changes

marler8997 force-pushed the jsonOrder branch 2 times, most recently from 2fb8b15 to e6f1e64 Compare January 23, 2018 23:55