New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add get_tree and set_tree methods to irmin-graphql server #590

Merged
merged 7 commits into from Nov 19, 2018

Conversation

Projects
None yet
2 participants
@zshipko
Contributor

zshipko commented Nov 15, 2018

Unfortunately there isn't a good way to model the tree input arguments as recursive objects (as I had hoped):

{
    "a": {"b": {"c": "123"}}, 
    "foo": "bar"
}

Instead, we have to use a flat array of objects:

[
    {"key": "a/b/c", "value": "123"}, 
    {"key": "foo", "value": "bar"}
]

Despite this one minor flaw, I think this is a pretty nice way of supporting read/write transactions in irmin-graphql!

@samoht

This comment has been minimized.

Member

samoht commented Nov 17, 2018

Unfortunately there isn't a good way to model the tree input arguments as recursive objects

Can you detail why it's not possible? I am fine to merge that PR but I would like to understand the design constraints :-)

@zshipko

This comment has been minimized.

Contributor

zshipko commented Nov 17, 2018

There is just no way to construct a recursive (or mutually recursive) input argument. For example, my initial try:

type tree = {
  key: string;
  tree: tree option;
  value: string option;
  metadata: string option;
 }

let rec tree () = Schema.Arg.(
  obj "Tree"
    ~fields:[
       arg "key" ~typ:(non_null string);
       arg "tree" ~typ:(tree ());
       arg "value" ~typ:string;
       arg "metadata" ~typ:string;
    ]
    ~coerce:(fun key tree value metadata -> {key; tree; value; metadata})
)

creates a stack overflow.. Also, making tree lazy would raise Lazy.Undefined in this case.

GraphQL really only works with recursion of a known depth.1 which means the return value from get_tree would pose a problem as well.

This final implementation seems to be more in-line with the way GraphQL is meant to be used, so even if there are workarounds to the infinite loop issue, I think it makes sense to stick with the array-based solution regardless.

1 facebook/graphql#91 (comment)

@samoht

This comment has been minimized.

Member

samoht commented Nov 19, 2018

Hum ok I see. Maybe we can allow to pass "arbitrary" JSON trees and consider them as Json_tree contents? That seems a bit annoying to serialise a nice tree into a collection of key/values pairs. But I guess it's fine as a first step.

@samoht samoht merged commit 4cd15fa into mirage:master Nov 19, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zshipko

This comment has been minimized.

Contributor

zshipko commented Nov 19, 2018

Yeah, it would definitely be possible to pass trees as JSON encoded strings — I will spend some time today to see if that’s a better approach.

@zshipko zshipko deleted the zshipko:graphql-set-tree branch Nov 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment