Nullable 1:1 connections #67

Totktonada · 2018-03-06T01:58:55Z

Fixes #44.

Totktonada · 2018-03-07T01:13:13Z

@Khatskevich @SudoBobo The PR is ready for review.

Khatskevich

First phase, but not the last...

Khatskevich · 2018-03-07T06:07:28Z

graphql/tarantool_graphql.lua

-            assert(c.type == '1:1' or c.type == '1:N',
-                'connection.type must be 1:1 or 1:N, got ' .. c.type)
+            assert(c.type == '1:1' or c.type == '1:1*' or c.type == '1:N',
+                'connection.type must be 1:1, 1:1* or 1:N, got ' .. c.type)


I do not like * syntax. It looks like magic for the person who see it the first time

Additional field (nullable=true) would also simplify your code. Because now 1:1 and 1:1* are generally the same, but you check both

I want to be consistent with our extended avro-schema approach.

Khatskevich · 2018-03-07T06:08:27Z

graphql/tarantool_graphql.lua

@@ -253,7 +253,7 @@ end
 --- The function converts passed avro-schema to a GraphQL type.
 ---
 --- @tparam table state for read state.accessor and previously filled
--- state.types (state.types are gql types)
+--- state.nullable_collection_types (its are gql types)


Khatskevich · 2018-03-07T06:13:12Z

graphql/tarantool_graphql.lua

@@ -230,7 +230,7 @@ end
 --- Convert each field of an avro-schema to a graphql type.
 ---
 --- @tparam table state for read state.accessor and previously filled
--- state.types
+--- state.nullable_collection_types


Why do you rename it?

I generally against common words to refer specific things. This field contains only collection types (and does not contain, say, nested records) and that types converted to nullable ones (because of lazy types evaluation, consider comments). So more specific name nullable_collection_types is fair term for that. Maybe collection_types_nullable is better, though.

Khatskevich · 2018-03-07T06:31:21Z

graphql/tarantool_graphql.lua

            assert(destination_type ~= nil,
                ('destination_type (named %s) must not be nil'):format(
                c.destination_collection))

            local c_args
            if c.type == '1:1' then
+                destination_type = types.nonNull(
+                    state.nullable_collection_types[c.destination_collection])


Why you do not reuse destination type?

You can make collection nonnull 2 times? I do not understand completely what would happen in this case.

Fixed.

I’m sure that collection types are nullable because it is how I save it.

Khatskevich · 2018-03-07T06:58:38Z

graphql/tarantool_graphql.lua

@@ -405,7 +450,7 @@ gql_type = function(state, avro_schema, collection, collection_name)
                avro_schema.name,
            fields = fields,
        })
-        return avro_t == 'enum' and types.nonNull(res) or res
+        return avro_t == 'record' and types.nonNull(res) or res


May be I am wrong, but I suppose that if the type is nullable or not should decide type user, but not type itself.
I suggest always return nullable types.

Interesting idea, but it would be architectural change. It is smth that we can have in our minds to consider. Thanks!

Khatskevich · 2018-03-07T08:30:38Z

test/testdata/nullable_1_1_conn_testdata.lua

+    end
+
+    results = results .. print_and_return(([[
+        +---------------------+


What is that?) Why do you print it? Moreover it is not printed in shard tests. Better to just leave it as a comment?

Forgot to print in shard tests. Will do. It was for convenient investigation of run of the test.

Consider yaml module in tarantool:

third_party/lua-yaml/lyaml.cc

} else if (strstr(str, "\n\n") != NULL) { /* * Tarantool-specific: use literal style for string with empty lines. * Useful for tutorial(). */ style = YAML_LITERAL_SCALAR_STYLE; }

I added two newlines in the begin of the string and got right output for the shard tests.

Khatskevich · 2018-03-07T08:34:31Z

test/testdata/nullable_1_1_conn_testdata.lua

@@ -0,0 +1,398 @@
+-- The example was inspired by [1]. Consider [2] for the problem description


It is up to you, but I do not like those teste. In my opinion we need to:

Write common tests

Check results in tests

Pros:

To validate test results we do not need to read 3 result files

The code you copy can contain small bugs, so, it should also be read 3 times
It is just a waste of time!

I think I should work on reducing code size of the tests. Maybe I should consider 'common' test suite + tap13, but I want to: run local test as just file, don’t repeat the sime initialization / running steps, working shard module reloading. I’ll consider your pain at reading in the next PRs.

Khatskevich

I request some important changes.
See comments.

Khatskevich · 2018-03-12T08:22:10Z

graphql/tarantool_graphql.lua

+                            value
+
+                        if value ~= nil then -- nil or box.NULL
+                            are_all_parts_null = false


I suppose this code should be moved outside of the closure (as a call to a function)

I’ll think about that before merge, but I’ll skip it now. Thanks for the point.

It seems that similar effort was made by @SudoBobo in the scope of #8. I’m going to leave it as is for now. Feel free to file an issue re code refactoring if you want to track this.

Khatskevich · 2018-03-12T08:24:12Z

graphql/tarantool_graphql.lua

+                    local is_full_match_ok = are_all_parts_null or
+                        are_all_parts_non_null
+                    if not is_full_match_ok then -- avoid extra json.encode()
+                        assert(is_full_match_ok,


Delete if, it is useless because your assert contains the same condition

Please, read the comment for this if.

Khatskevich · 2018-03-12T08:26:24Z

graphql/tarantool_graphql.lua

+                    -- key parts
+                    --
+                    -- zero parts count considered as ok by this check
+                    local is_full_match_ok = are_all_parts_null or


I do not like the extra variable with the strange name is_full_match_ok. It does not increase readability and should be used only once in the following assert, please, use just are_all_parts_null or are_all_parts_non_null instead (in assert).

Ok, I’ll use name ok. Don’t want to carry lines, it looks ugly here.

Khatskevich · 2018-03-12T08:32:21Z

graphql/tarantool_graphql.lua

+                    --   return the empty list.
+                    if are_all_parts_null then
+                        assert(c.type == '1:1*' or c.type == '1:N',
+                            'only 1:1* or 1:N connections can have ' ..


Do not split the user-visible output into lines (grep). We should follow this rule in the future too.
To maintain good code style, I propose to create a file with error messages.

Moreover, in this particular case, I believe you should provide extra information about the failed object. Because even after user sees this error, it is impossible to debug the case.

We discussed the line carrying in the case in context of tarantool code style and decided to force formal line lenght limit. So I’ll prefer to fit the limit, but I’ll try to split a message by meaningful parts.

extra info: TBD

extra info: added

Khatskevich · 2018-03-12T08:36:14Z

graphql/tarantool_graphql.lua

+                    end
+
+                    -- check FULL match constraint before request of
+                    -- destination object(s)


Half of your comments are sentences with a dot at the end and start with a capital teller, and half of them are just text. I like the first option more. Let's follow the style?

My approach is the following. I use small letter in the begin and omit period at the end when the comment is one clause or like so (maybe even an expanded sentence, but only one). Several sentences started from capital letter and have period at the end.

Here we have several sentences, so I reformatted this into one multi-sentence comment (capital letter + period).

Khatskevich · 2018-03-12T08:41:36Z

test/testdata/nullable_1_1_conn_testdata.lua

+
+    local function new_email(body)
+        return {
+            localpart = random_string(16):hex(),


Please, do not use random functions for pest data. This field can not be used in test output because it is not consistent.

I initialized PRNG with a constant seed. You can use it in the output and it will be the same.

Found that math.random() gives different numbers on different machines. Implemented simple PRNG in the test.

Khatskevich · 2018-03-12T08:48:04Z

test/testdata/nullable_1_1_conn_testdata.lua

+    end)
+
+    -- FULL MATCH constraint: connection key parts must be all non-nulls or all
+    -- nulls


Please, create 1 failing test.

I have two failing tests here. Commented explicitly that they are fails.

Fixes #44.

Khatskevich

Everything is negotiated or fixed. ACK

Totktonada added enhancement New feature or request prio1 labels Mar 6, 2018

Totktonada self-assigned this Mar 6, 2018

Totktonada requested review from Kasen, Khatskevich and SudoBobo March 6, 2018 01:58

Totktonada force-pushed the gh-44-nullable-1-1-connections branch from eca0b2b to e9887a5 Compare March 7, 2018 01:12

Totktonada changed the title ~~WIP: nullable 1:1 connections~~ Nullable 1:1 connections Mar 7, 2018

Khatskevich suggested changes Mar 7, 2018

View reviewed changes

Totktonada force-pushed the gh-44-nullable-1-1-connections branch 2 times, most recently from 59e28ff to f79bdaf Compare March 7, 2018 13:36

Khatskevich suggested changes Mar 12, 2018

View reviewed changes

Totktonada force-pushed the gh-44-nullable-1-1-connections branch 2 times, most recently from d7eab2c to 291cf7e Compare March 12, 2018 21:54

Nullable 1:1 connections

13c5c7b

Fixes #44.

Totktonada force-pushed the gh-44-nullable-1-1-connections branch from 291cf7e to 13c5c7b Compare March 12, 2018 23:57

Khatskevich approved these changes Mar 13, 2018

View reviewed changes

Totktonada merged commit 8bc5331 into master Mar 13, 2018

Totktonada deleted the gh-44-nullable-1-1-connections branch March 13, 2018 21:07

		@@ -0,0 +1,398 @@
		-- The example was inspired by [1]. Consider [2] for the problem description

Nullable 1:1 connections #67

Nullable 1:1 connections #67

Conversation

Totktonada commented Mar 6, 2018 • edited Loading

Totktonada commented Mar 7, 2018

Khatskevich 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

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

Khatskevich 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

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

Khatskevich left a comment

Choose a reason for hiding this comment

Totktonada commented Mar 6, 2018 •

edited

Loading