WIP simple configuration #95

SudoBobo · 2018-03-23T13:26:46Z

Related to #59 and #60.

Totktonada

Link #59 and #60 to the PR.

Consider comments below.

Totktonada · 2018-03-26T13:29:03Z

test/testdata/common_testdata.lua

@@ -15,7 +15,7 @@ function common_testdata.get_test_metadata()
            "type": "record",
            "name": "user",
            "fields": [
-                { "name": "user_id", "type": "string" },
+                { "name": " user_id", "type": "string" },


Be sure you read entire git diff before propose changes to review. This is typo, I think.

Sorry. Fixed.

Totktonada · 2018-03-27T10:23:33Z

graphql/config_complement.lua

+
+local config_complement = {}
+
+--- The function determines connection type by fully qualified connection.parts


What do you mean with fully qualified? Usually that means that a name has all its namespaces as parts of the name.

I mean that connection.parts must have a format of:

"parts": [ { "source_field": "user_id", "destination_field": "user_id" }, ... ],

I am not sure about naming here. The logic of config_complement is about converting from:

{ name='order_connection', source_collection = 'user_collection', destination_collection = 'order_collection', index_name = 'user_id_index' }

to (in source_collection)

{ "type": "1:N", "name": "order_connection", "destination_collection": "order_collection", "parts": [ { "source_field": "user_id", "destination_field": "user_id" } ], "index_name": "user_id_index" }

How to name this two formats? Simplified and full connections?

Partially / fully defined?

Fixed to partially / fully defined.

Totktonada · 2018-03-27T10:32:42Z

graphql/accessor_general.lua

@@ -827,7 +827,7 @@ local function process_tuple(state, tuple, opts)

    -- convert tuple -> object
    local obj = opts.unflatten_tuple(collection_name, tuple,
-        opts.default_unflatten_tuple)
+        opts.default_unflatten_tuple, opts.is_collection_formatted)


I think it worth to keep profile of the default_unflatten_tuple function the same as unflatten_tuple (except the last parameter, default).

Discucced verbally.

Totktonada · 2018-03-27T10:33:41Z

graphql/accessor_general.lua

@@ -1285,6 +1288,7 @@ function accessor_general.new(opts, funcs)
        indexes = indexes,
        models = models,
        default_unflatten_tuple = default_unflatten_tuple,
+        formatted_collections = opts.formatted_collections,


Re naming: maybe space_format_layout?

Currently it is a map with {collection_name: is_formatted}. We may extend it to be like: {collection_name: related_space_format}, but currently we does not need this information in accessor.

Discussed. We both ok with have_space_format.

Totktonada · 2018-03-27T10:34:43Z

graphql/accessor_space.lua

-local function unflatten_tuple(collection_name, tuple, default)
+local function unflatten_tuple(collection_name, tuple, default, is_collection_formatted)
+    if is_collection_formatted then
+        return tuple:tomap()


What with number fields? tarantool/tarantool#3281

I understand why it works w/o extra hacks. Because of fields selection in executor. So, okay to leave as is, but add the option for future tarantool versions.

You propose to clarify our intensions with this?

return tuple:tomap{{only_names=true})

Yep, would be good.

Totktonada · 2018-03-27T14:58:42Z

test/local/simple_config.test.lua

+
+
+
+local gql_wrapper = graphql.new({


What is difference from space_common test?

In simple_config we just test new constructor format.
space_common:

local accessor = graphql.accessor_space.new({ schemas = schemas, collections = collections, service_fields = service_fields, indexes = indexes, }) local gql_wrapper = graphql.new({ schemas = schemas, collections = collections, accessor = accessor, })

simple_config:

local gql_wrapper = graphql.new({ schemas = schemas, collections = collections, service_fields = service_fields, indexes = indexes, accessor = 'space' })

Totktonada · 2018-03-27T14:59:17Z

test/local/zero_config.test.lua

+    local shard = shard or box.space
+
+    shard.user_collection:replace(
+    {'user_id_1', 'Ivan', 42})


Totktonada · 2018-03-27T14:59:28Z

test/local/zero_config.test.lua

+        local gql_query_1 = gql_wrapper:compile(query_1)
+        local result = gql_query_1:execute(variables_1)
+        results = results .. print_and_return(
+        ('RESULT\n%s'):format(yaml.encode(result)))


Totktonada · 2018-03-27T14:59:45Z

test/local/zero_config.test.lua

+    box.once('test_space_init_spaces', function()
+        box.schema.create_space('user_collection')
+        box.space.user_collection:create_index('user_id_index',
+        {type = 'tree', unique = true, parts = {


Totktonada · 2018-03-27T15:02:17Z

test/local/zero_config.test.lua

+    box.cfg { background = false }
+    init_spaces()
+    fill_test_data()
+    local gql_wrapper = graphql.new({auto_cfg = true})


Do we want graphql:compile() method (and create zero configuration graphql instance under hood) and graphql:execute() method?

We do. Except for graphql:execute(). Currently we do not use it like graphql:execute(), we use it like compiled_quer.execute(). Do you suggest to support graphql:execute() anyway?

Why not? Consider box.sql.execute for example.

Just call compile + execute at once.

Totktonada

Most of comments are about my feelings and can be ignored. The PR generally LGTM, so you can do what you want on it and merge. Please, squash commits and rebase on current master.

Totktonada · 2018-04-03T20:28:38Z

test/testdata/common_testdata.lua

+                            { "source_field": "user_id", "destination_field": "user_id" }
+                        ],
+                        "index_name": "user_id_index"
+                    }


No reason. Fxd.

Totktonada · 2018-04-03T20:29:04Z

graphql/accessor_general.lua

@@ -827,7 +827,7 @@ local function process_tuple(state, tuple, opts)

    -- convert tuple -> object
    local obj = opts.unflatten_tuple(collection_name, tuple,
-        opts.default_unflatten_tuple)
+    { has_space_format = opts.has_space_format }, opts.default_unflatten_tuple)


Totktonada · 2018-04-03T20:30:07Z

graphql/accessor_general.lua

@@ -1179,6 +1180,7 @@ local function get_pcre_argument_type(self, collection_name)
    return pcre_type
 end

+--todo add comments about opts.formatted_collections (also assertions)


Do you going to do it in this PR?

Totktonada · 2018-04-03T20:35:31Z

graphql/accessor_general.lua

@@ -1285,6 +1287,7 @@ function accessor_general.new(opts, funcs)
        indexes = indexes,
        models = models,
        default_unflatten_tuple = default_unflatten_tuple,
+        has_space_format = opts.has_space_format or {},


Is it a boolean or a table?

Clarified names, so now there is no such uncertain names.

Totktonada · 2018-04-03T20:45:20Z

graphql/accessor_shard.lua

@@ -84,12 +84,19 @@ end
 ---
 --- @tparam string collection_name
 --- @tparam cdata/table tuple
+--- @tparam table opts
+--- * `has_space_format` (boolean, if collection 'collection_name' generated
+--- from space:format() or not).


I suggest to rephrase it to avoid misinterpret as 'boolean if ..., but other type otherwise':

boolean, default: false; whether objects in collection collection_name intended to be unflattened using tuple:tomap({names_only = true}) method instead of compiled_avro_schema.unflatten(tuple).

Here I proposed more concrete wording about how exactly behaviour will be changed by enabling this flag.

Agree. Fxd.

Totktonada · 2018-04-03T22:11:01Z

graphql/utils.lua

+end
+
+--- Check if given table has only one specific key.
+function utils.has_only(t, key)


No copy (and certainly deepcopy) need, try this:

local fst_key = next(t) local snd_key = next(t, fst_key) return fst_key == key and snd_key == nil

Ok, good, fxd.

Totktonada · 2018-04-03T22:18:07Z

graphql/utils.lua

+    return has_key and not_has_another_key
+end
+
+function utils.size(t)


I would prefer less general name like table_size.

Totktonada · 2018-04-03T22:18:58Z

test/local/complemented_config.test.lua

+            ('RESULT\n%s'):format(yaml.encode(result)))
+    end)
+
+    utils.show_trace(function()


I think we don’t need to try-catch such prints.

Agree. Fxd.

Totktonada · 2018-04-03T22:21:35Z

test/local/simple_config.test.lua

@@ -0,0 +1,56 @@
+#!/usr/bin/env tarantool


Consider adding this test as one of configurations of test/local/space_common.test.lua. Use suite.cfg file.

I think this should be part of test refactoring ticket we discussed verbaly

File it to issues, please.

Totktonada · 2018-04-03T22:28:52Z

test/shard_no_redundancy/shard_common_simple_config.test.lua

@@ -0,0 +1,74 @@
+env = require('test_run')


Maybe this can be added as one of configuration of the shard_common.test.lua test?

See comment above

1. Zero config 2. require('graphql').execute()/compile() functionality 3. GraphiQL 4. Config complementation

SudoBobo requested a review from Totktonada March 23, 2018 13:26

Totktonada reviewed Mar 27, 2018

View reviewed changes

Totktonada assigned SudoBobo Mar 27, 2018

Totktonada added enhancement New feature or request usability labels Mar 27, 2018

SudoBobo force-pushed the sb/simple_config branch 4 times, most recently from c2cec15 to 82c0bad Compare April 2, 2018 17:10

Totktonada approved these changes Apr 3, 2018

View reviewed changes

Provide list of usability features:

a2e918f

1. Zero config 2. require('graphql').execute()/compile() functionality 3. GraphiQL 4. Config complementation

SudoBobo force-pushed the sb/simple_config branch from 82c0bad to a2e918f Compare April 6, 2018 10:54

SudoBobo merged commit 30c64ab into master Apr 6, 2018

This was referenced Apr 19, 2018

Single-point graphql constructor #59

Closed

Zero configuration graphql constructor for spaces with format #60

Closed

SudoBobo deleted the sb/simple_config branch May 26, 2018 18:08


		local config_complement = {}

		--- The function determines connection type by fully qualified connection.parts

WIP simple configuration #95

WIP simple configuration #95

Conversation

SudoBobo commented Mar 23, 2018 • edited Loading

Totktonada left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SudoBobo Mar 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SudoBobo Apr 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Totktonada left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SudoBobo Apr 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SudoBobo commented Mar 23, 2018 •

edited

Loading

Totktonada left a comment •

edited

Loading

SudoBobo Mar 28, 2018 •

edited

Loading

SudoBobo Apr 2, 2018 •

edited

Loading

SudoBobo Apr 4, 2018 •

edited

Loading