Re: [tarantool-patches] Re: [PATCH v2 02/10] yaml: introduce yaml.encode_tagged

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hello. Thanks for the review.

On 10/05/2018 21:22, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [18/04/20 16:25]:
>> index 430f4be2c..8329f84e9 100644
>> --- a/third_party/lua-yaml/lyaml.cc
>> +++ b/third_party/lua-yaml/lyaml.cc
>> @@ -78,6 +78,8 @@ struct lua_yaml_dumper {
>>      unsigned int anchor_number;
>>      yaml_emitter_t emitter;
>>      char error;
>> +   yaml_tag_directive_t begin_tag;
>> +   yaml_tag_directive_t *end_tag;
> 
> Please add comments for the new members.

@@ -78,7 +78,16 @@ struct lua_yaml_dumper {
     unsigned int anchor_number;
     yaml_emitter_t emitter;
     char error;
+   /** Global tag to label the result document by. */
     yaml_tag_directive_t begin_tag;
+   /**
+    * - end_tag == &begin_tag - a document is not labeld with a
+    * global tag.
+    * - end_tag == &begin_tag + 1 - a document is labeled with a
+    * global tag specified in begin_tag attribute. End_tag pointer
+    * is used instead of tag count because of libyaml API - it
+    * takes begin and end pointers of tags array.
+    */
     yaml_tag_directive_t *end_tag;

> 
>>   
>> +/**
>> + * Encode a Lua object or objects into YAML documents onto Lua
>> + * stack.
> 
> Encode an object or objects on Lua stack into YAML stream.

@@ -622,9 +631,9 @@ static void find_references(struct lua_yaml_dumper *dumper) {
  }
  
  /**
- * Encode a Lua object or objects into YAML documents onto Lua
- * stack.
+ * Encode an object or objects on Lua stack into YAML stream.

> 
>> + * @param L Lua stack to get arguments and push result.
>> + * @param serializer Lua YAML serializer.
>> + * @param tag_handle NULL, or a global tag handle. Handle becomes
>> + *        a synonym for prefix.
> 
> The handle becomes a synonym for prefix.
> 
> 
> I don't understand what this means.

Handle and prefix are YAML standard terms. Consider this tag:
%TAG !push! tag:tarantool.io/push,2018

Here !push! is a handle, tag:tarantool.io/push,2018 - prefix.
When in a document !push! is written, it is exposed to
!tag:tarantool.io/push,2018.

Example:
-- !push!str string_value

Same as:
-- !tag:tarantool.io/push,2018!str string_value

So here handle '!push!' is exposed to 'tag:tarantool.io/push,2018'.

Look:
tarantool> a = yaml.decode('!tag:tarantool.io/push,2018!str string_value')
---
...

tarantool> a
---
- string_value
...

So handle is synonym for prefix.

> 
>> + * @param tag_prefix NULL, or a global tag prefix, to which @a
>> + *        handle is expanded.
> 
> Perhaps you could say a few words here about handles, prefixes and
> expansions, or, better yet, quote the relevant parts of YAML
> standard.

- * @param tag_handle NULL, or a global tag handle. Handle becomes
- *        a synonym for prefix.
+ * @param tag_handle NULL, or a global tag handle. For global tags
+ *        details see the standard:
+ *        http://yaml.org/spec/1.2/spec.html#tag/shorthand/.
   * @param tag_prefix NULL, or a global tag prefix, to which @a
- *        handle is expanded.
+ *        handle is expanded. Example of a tagged document:
+ *              handle          prefix
+ *               ____   ________________________
+ *              /    \ /                        \
+ *        '%TAG !push! tag:tarantool.io/push,2018
+ *         --- value
+ *         ...
+ *

> 
>> + * @retval nil, error Error.
>> + * @retval not nil Lua string with dumped object.
> 
> The return value is integer. What did you mean to say?
> For Lua functions it's better to write something like -2, +1
> (pops two, pushes 1).

- * @retval nil, error Error.
- * @retval not nil Lua string with dumped object.
+ * @retval 2 Pushes two values on error: nil, error description.
+ * @retval 1 Pushes one value on success: string with dumped
+ *         object.
   */

> 
>> + */
>> +static int
>> +lua_yaml_encode_impl(lua_State *L, struct luaL_serializer *serializer,
>> +                     const char *tag_handle, const char *tag_prefix)
>> +{
>>      struct lua_yaml_dumper dumper;
>>      int i, argcount = lua_gettop(L);
>>      yaml_event_t ev;
>>   
>>      dumper.L = L;
>> -   dumper.cfg = luaL_checkserializer(L);
>> +   dumper.begin_tag.handle = (yaml_char_t *) tag_handle;
>> +   dumper.begin_tag.prefix = (yaml_char_t *) tag_prefix;
>> +   /*
>> +    * If a tag is specified, then tag list is not empty and
>> +    * consists of a single tag.
>> +    */
>> +   if (tag_handle != NULL && tag_prefix != NULL)
> 
> Why do you need to check both?

@@ -661,11 +661,12 @@ lua_yaml_encode_impl(lua_State *L, int argcount,
     dumper.L = L;
     dumper.begin_tag.handle = (yaml_char_t *) tag_handle;
     dumper.begin_tag.prefix = (yaml_char_t *) tag_prefix;
+   assert((tag_handle == NULL) == (tag_prefix == NULL));
     /*
      * If a tag is specified, then tag list is not empty and
      * consists of a single tag.
      */
-   if (tag_handle != NULL && tag_prefix != NULL)
+   if (tag_prefix != NULL)
        dumper.end_tag = &dumper.begin_tag + 1;
     else
        dumper.end_tag = &dumper.begin_tag;

> 
> dumper.end_tag = &dumper.begin_tag + (tag_handle != NULL && tag_prefix != NULL);

I very do not like arithmetic operations on boolean and integers, so no.

>> @@ -684,11 +712,46 @@ error:
>>      }
>>   }
>>   
>> +static int l_dump(lua_State *L) {
> 
> Missing newline.

Not missing. I did it advisedly. The lyaml.cc file has its own code
style, that slightly differs from Tarantool's. We must either convert
the entire file to Tarantool, or obey lyaml style.

> 
> The function needs a formal comment, even though it's trivial.

+/**
+ * Dump Lua objects into YAML string. It takes any argument count,
+ * and dumps each in a separate document.
+ * @retval 1 Pushes one value: string with dumped documents.
+ */

> 
>> +   return lua_yaml_encode_impl(L, luaL_checkserializer(L), NULL, NULL);
>> +}
>> +
>> +/**
>> + * Dump a Lua object into YAML string with a global TAG specified.
> 
> Serialize a Lua object as YAML string, taking into account a
> global tag, if it's supplied in the arguments.

- * Dump a Lua object into YAML string with a global TAG specified.
+ * Serialize a Lua object as YAML string, taking into account a
+ * global tag.

I did not add "if it's supplied in the arguments", because a tag is
required.

> 
>> + * @param options First argument on a stack, consists of two
>> + *        options: tag prefix and tag handle.
>> + * @param object Lua object to dump under the global tag.
>> + * @retval Lua string with dumped object.
> 
> Why do you take options first, object second? AFAICS we usually
> take object first, options second. Let's be consistent?

+ * @param object Lua object to dump under the global tag.
+ * @param options Table with two options: tag prefix and tag
+ *        handle.

> 
> Which begs the question, why do you need a new function rather
> than extend an existing one with options?

Original yaml.encode has this signature: encode(...). It takes
any argument count, and dumps each. So I can not add an option to
this. Example:
yaml.encode(object, {prefix = ..., handle = ...})

What to do, dump both arguments or use the second as options? I think
that we lost a chance to change encode() signature, it is already public
and documented, and will always take (...) arguments.

But here it is documented
https://tarantool.io/en/doc/1.9/reference/reference_lua/yaml.html?highlight=yaml#lua-function.yaml.encode
that yaml.encode has one argument. Maybe no one noticed that it takes multiple
values. If you want, I could change its signature from (...) to (value, [{tag = <tag>}]).

> 
>> + */
>> +static int l_dump_tagged(lua_State *L)
>> +{
>> +   struct luaL_serializer *serializer = luaL_checkserializer(L);
>> +   int argcount = lua_gettop(L);
>> +   if (argcount != 2 || !lua_istable(L, 1)) {
>> +usage_error:
>> +      return luaL_error(L, "Usage: encode_tagged({prefix = <string>, "\
>> +                           "handle = <string>}, object)");
>> +   }
>> +   lua_getfield(L, 1, "prefix");
>> +   if (! lua_isstring(L, -1))
>> +      goto usage_error;
>> +   const char *prefix = lua_tostring(L, -1);
>> +   lua_getfield(L, 1, "handle");
>> +   if (! lua_isstring(L, -1))
>> +      goto usage_error;
>> +   const char *handle = lua_tostring(L, -1);
>> +   lua_pop(L, 2);
>> +   lua_replace(L, 1);
>> +   lua_settop(L, 1);
> 
> AFAIR you invalidate handle and prefix pointers as soon as you pop
> and replace things on the stack.

Fixed on the branch.

 From the next review I fixed usage tests:

@@ -71,14 +71,13 @@ end
  
  local function test_tagged(test, s)
      test:plan(7)
-    local must_be_err = 'Usage: encode_tagged(object, {prefix = <string>, handle = <string>})'
      local prefix = 'tag:tarantool.io/push,2018'
      local ok, err = pcall(s.encode_tagged)
-    test:is(err, must_be_err, "encode_tagged usage")
+    test:isnt(err:find('Usage'), nil, "encode_tagged usage")
      ok, err = pcall(s.encode_tagged, 100, {})
-    test:is(err, must_be_err, "encode_tagged usage")
+    test:isnt(err:find('Usage'), nil, "encode_tagged usage")
      ok, err = pcall(s.encode_tagged, 200, {handle = true, prefix = 100})
-    test:is(err, must_be_err, "encode_tagged usage")
+    test:isnt(err:find('Usage'), nil, "encode_tagged usage")
      local ret
      ret, err = s.encode_tagged(300, {handle = '!push', prefix = prefix})
      test:is(ret, nil, 'non-usage and non-oom errors do not raise')

See below the entire patch.

=======================================================================

commit 0117eac46095ac7fbf7d36c7ebfeb7fe16f96cc2
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Mon Apr 2 23:40:30 2018 +0300

     yaml: introduce yaml.encode_tagged
     
     Encode_tagged allows to define one global YAML tag for a
     document. Tagged YAML documents are going to be used for
     console text pushes to distinguish actual box.session.push() from
     console.print(). The first will have tag !push, and the
     second - !print.

diff --git a/test/app-tap/yaml.test.lua b/test/app-tap/yaml.test.lua
index def278570..bb75ce4ea 100755
--- a/test/app-tap/yaml.test.lua
+++ b/test/app-tap/yaml.test.lua
@@ -69,9 +69,28 @@ local function test_output(test, s)
          "--- |-\n  Tutorial -- Header\n  ====\n\n  Text\n...\n", "tutorial string");
  end
  
+local function test_tagged(test, s)
+    test:plan(7)
+    local prefix = 'tag:tarantool.io/push,2018'
+    local ok, err = pcall(s.encode_tagged)
+    test:isnt(err:find('Usage'), nil, "encode_tagged usage")
+    ok, err = pcall(s.encode_tagged, 100, {})
+    test:isnt(err:find('Usage'), nil, "encode_tagged usage")
+    ok, err = pcall(s.encode_tagged, 200, {handle = true, prefix = 100})
+    test:isnt(err:find('Usage'), nil, "encode_tagged usage")
+    local ret
+    ret, err = s.encode_tagged(300, {handle = '!push', prefix = prefix})
+    test:is(ret, nil, 'non-usage and non-oom errors do not raise')
+    test:is(err, "tag handle must end with '!'", "encode_tagged usage")
+    ret = s.encode_tagged(300, {handle = '!push!', prefix = prefix})
+    test:is(ret, "%TAG !push! "..prefix.."\n--- 300\n...\n", "encode_tagged usage")
+    ret = s.encode_tagged({a = 100, b = 200}, {handle = '!print!', prefix = prefix})
+    test:is(ret, "%TAG !print! tag:tarantool.io/push,2018\n---\na: 100\nb: 200\n...\n", 'encode_tagged usage')
+end
+
  tap.test("yaml", function(test)
      local serializer = require('yaml')
-    test:plan(10)
+    test:plan(11)
      test:test("unsigned", common.test_unsigned, serializer)
      test:test("signed", common.test_signed, serializer)
      test:test("double", common.test_double, serializer)
@@ -82,4 +101,5 @@ tap.test("yaml", function(test)
      test:test("ucdata", common.test_ucdata, serializer)
      test:test("compact", test_compact, serializer)
      test:test("output", test_output, serializer)
+    test:test("tagged", test_tagged, serializer)
  end)
diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc
index 430f4be2c..ca7f4d224 100644
--- a/third_party/lua-yaml/lyaml.cc
+++ b/third_party/lua-yaml/lyaml.cc
@@ -78,6 +78,17 @@ struct lua_yaml_dumper {
     unsigned int anchor_number;
     yaml_emitter_t emitter;
     char error;
+   /** Global tag to label the result document by. */
+   yaml_tag_directive_t begin_tag;
+   /**
+    * - end_tag == &begin_tag - a document is not labeld with a
+    * global tag.
+    * - end_tag == &begin_tag + 1 - a document is labeled with a
+    * global tag specified in begin_tag attribute. End_tag pointer
+    * is used instead of tag count because of libyaml API - it
+    * takes begin and end pointers of tags array.
+    */
+   yaml_tag_directive_t *end_tag;
  
     lua_State *outputL;
     luaL_Buffer yamlbuf;
@@ -571,7 +582,8 @@ static int dump_node(struct lua_yaml_dumper *dumper)
  static void dump_document(struct lua_yaml_dumper *dumper) {
     yaml_event_t ev;
  
-   if (!yaml_document_start_event_initialize(&ev, NULL, NULL, NULL, 0) ||
+   if (!yaml_document_start_event_initialize(&ev, NULL, &dumper->begin_tag,
+                                             dumper->end_tag, 0) ||
         !yaml_emitter_emit(&dumper->emitter, &ev))
        return;
  
@@ -618,13 +630,52 @@ static void find_references(struct lua_yaml_dumper *dumper) {
     }
  }
  
-static int l_dump(lua_State *L) {
+/**
+ * Encode an object or objects on Lua stack into YAML stream.
+ * @param L Lua stack to get arguments and push result.
+ * @param serializer Lua YAML serializer.
+ * @param tag_handle NULL, or a global tag handle. For global tags
+ *        details see the standard:
+ *        http://yaml.org/spec/1.2/spec.html#tag/shorthand/.
+ * @param tag_prefix NULL, or a global tag prefix, to which @a
+ *        handle is expanded. Example of a tagged document:
+ *              handle          prefix
+ *               ____   ________________________
+ *              /    \ /                        \
+ *        '%TAG !push! tag:tarantool.io/push,2018
+ *         --- value
+ *         ...
+ *
+ * @retval 2 Pushes two values on error: nil, error description.
+ * @retval 1 Pushes one value on success: string with dumped
+ *         object.
+ */
+static int
+lua_yaml_encode_impl(lua_State *L, struct luaL_serializer *serializer,
+                     const char *tag_handle, const char *tag_prefix)
+{
     struct lua_yaml_dumper dumper;
-   int i, argcount = lua_gettop(L);
     yaml_event_t ev;
+   int argcount;
  
     dumper.L = L;
-   dumper.cfg = luaL_checkserializer(L);
+   dumper.begin_tag.handle = (yaml_char_t *) tag_handle;
+   dumper.begin_tag.prefix = (yaml_char_t *) tag_prefix;
+   assert((tag_handle == NULL) == (tag_prefix == NULL));
+   /*
+    * If a tag is specified, then tag list is not empty and
+    * consists of a single tag.
+    */
+   if (tag_prefix != NULL) {
+      dumper.end_tag = &dumper.begin_tag + 1;
+      /* Only one object can be encoded with a tag. */
+      argcount = 1;
+   } else {
+      dumper.end_tag = &dumper.begin_tag;
+      /* When no tags - encode all the stack. */
+      argcount = lua_gettop(L);
+   }
+   dumper.cfg = serializer;
     dumper.error = 0;
     /* create thread to use for YAML buffer */
     dumper.outputL = lua_newthread(L);
@@ -643,7 +694,7 @@ static int l_dump(lua_State *L) {
         !yaml_emitter_emit(&dumper.emitter, &ev))
        goto error;
  
-   for (i = 0; i < argcount; i++) {
+   for (int i = 0; i < argcount; i++) {
        lua_newtable(L);
        dumper.anchortable_index = lua_gettop(L);
        dumper.anchor_number = 0;
@@ -684,11 +735,49 @@ error:
     }
  }
  
+/**
+ * Dump Lua objects into YAML string. It takes any argument count,
+ * and dumps each in a separate document.
+ * @retval 1 Pushes one value: string with dumped documents.
+ */
+static int l_dump(lua_State *L) {
+   return lua_yaml_encode_impl(L, luaL_checkserializer(L), NULL, NULL);
+}
+
+/**
+ * Serialize a Lua object as YAML string, taking into account a
+ * global tag.
+ * @param object Lua object to dump under the global tag.
+ * @param options Table with two options: tag prefix and tag
+ *        handle.
+ * @retval 1 Pushes Lua string with dumped object.
+ * @retval 2 Pushes nil and error message.
+ */
+static int l_dump_tagged(lua_State *L)
+{
+   struct luaL_serializer *serializer = luaL_checkserializer(L);
+   if (lua_gettop(L) != 2 || !lua_istable(L, 2)) {
+usage_error:
+      return luaL_error(L, "Usage: encode_tagged(object, {prefix = <string>, "\
+                        "handle = <string>})");
+   }
+   lua_getfield(L, 2, "prefix");
+   if (! lua_isstring(L, -1))
+      goto usage_error;
+   const char *prefix = lua_tostring(L, -1);
+   lua_getfield(L, 2, "handle");
+   if (! lua_isstring(L, -1))
+      goto usage_error;
+   const char *handle = lua_tostring(L, -1);
+   return lua_yaml_encode_impl(L, serializer, handle, prefix);
+}
+
  static int
  l_new(lua_State *L);
  
  static const luaL_Reg yamllib[] = {
     { "encode", l_dump },
+   { "encode_tagged", l_dump_tagged },
     { "decode", l_load },
     { "new",    l_new },
     { NULL, NULL}