Thrift
  1. Thrift
  2. THRIFT-1151

Produce more informative runtime error in case of schema and data mismatch during serialization

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.7
    • Component/s: Erlang - Library
    • Labels:
      None
    • Patch Info:
      Patch Available

      Description

      This patch adds generation of informative error during serialization of struct, which is unmatched to it's struct_info.
      Instead of generating

      {error, enival}

      in gen_tcp during sending invalid binary, it produces accurate error with some useful information.

      1. thrift-1151.patch
        0.6 kB
        Anthony Molinaro
      2. diff.txt
        0.6 kB
        Anatoly Kanivetsky

        Activity

        Hide
        Anthony Molinaro added a comment -

        Can you give me an example to test this patch out with? Also, I'm a little confused, it seems like this patch only contains commented out code.

        Show
        Anthony Molinaro added a comment - Can you give me an example to test this patch out with? Also, I'm a little confused, it seems like this patch only contains commented out code.
        Hide
        Anatoly Kanivetsky added a comment -

        Sorry, just uncomment this function in patch

        Example of problem:

        in some.thrift file:

        struct StructA

        { 1: i16 x; }

        struct StructB

        { 1: i32 x; }

        struct StructC

        { 1: StructA x; }

        in some.erl file,

        S1 = #structC{x=#structB{x=1}},
        S2 = #structC{x=#structA{x=1}},

        Sending S1 should fail with error, since it is not valid StructC.
        Can be validated with service, or just by:

        {ok, Transport}

        = thrift_memory_buffer:new(),

        {ok, Protocol}

        = thrift_binary_protocol:new(Transport),
        thrift_protocol:write(Protocol, struct, {'some_types', structC, S1})

        Show
        Anatoly Kanivetsky added a comment - Sorry, just uncomment this function in patch Example of problem: in some.thrift file: struct StructA { 1: i16 x; } struct StructB { 1: i32 x; } struct StructC { 1: StructA x; } in some.erl file, S1 = #structC{x=#structB{x=1}}, S2 = #structC{x=#structA{x=1}}, Sending S1 should fail with error, since it is not valid StructC. Can be validated with service, or just by: {ok, Transport} = thrift_memory_buffer:new(), {ok, Protocol} = thrift_binary_protocol:new(Transport), thrift_protocol:write(Protocol, struct, {'some_types', structC , S1})
        Hide
        Anthony Molinaro added a comment -

        Committed with a slight change, instead of exit/1, I use error/2. I think this is closer to how other failures work, for instance providing the wrong type for x results in a bad argument error exception.

        Thanks for the path Anatoly.

        Show
        Anthony Molinaro added a comment - Committed with a slight change, instead of exit/1, I use error/2. I think this is closer to how other failures work, for instance providing the wrong type for x results in a bad argument error exception. Thanks for the path Anatoly.
        Hide
        Roger Meier added a comment -

        This commit breaks the build running on Ubuntu... see https://builds.apache.org/view/S-Z/view/Thrift/job/Thrift/167/console

        Making all in src
        make[4]: Entering directory `/home/hudson/hudson-slave/workspace/Thrift/thrift/lib/erl/src'
        mkdir ../ebin
           ERLC  thrift_protocol.erl
        ./thrift_protocol.erl:335: function error/2 undefined
        make[4]: *** [../ebin/thrift_protocol.beam] Error 1
        

        As a erlang newbie, I have no idea why... any suggestions?

        Show
        Roger Meier added a comment - This commit breaks the build running on Ubuntu... see https://builds.apache.org/view/S-Z/view/Thrift/job/Thrift/167/console Making all in src make[4]: Entering directory `/home/hudson/hudson-slave/workspace/Thrift/thrift/lib/erl/src' mkdir ../ebin ERLC thrift_protocol.erl ./thrift_protocol.erl:335: function error/2 undefined make[4]: *** [../ebin/thrift_protocol.beam] Error 1 As a erlang newbie, I have no idea why... any suggestions?
        Hide
        Anthony Molinaro added a comment -

        Ahh, I bet Ubuntu uses R13 or earlier. I'm pretty sure the fix is to prefix error with erlang: can you try that and see if it compiles?

        Show
        Anthony Molinaro added a comment - Ahh, I bet Ubuntu uses R13 or earlier. I'm pretty sure the fix is to prefix error with erlang: can you try that and see if it compiles?
        Hide
        Roger Meier added a comment -

        The build machine uses 13b, I have 14a on Debian Squeeze with the same issue.

        A stupid question: How can I prefix error with erlang, the verify or test this?

        Show
        Roger Meier added a comment - The build machine uses 13b, I have 14a on Debian Squeeze with the same issue. A stupid question: How can I prefix error with erlang, the verify or test this?
        Hide
        Anthony Molinaro added a comment -

        Try applying the patch I just uploaded then just run make.

        Essentially the line which starts with 'error(struct_unmatched, ...' needs to be 'erlang:error(struct_unmatched, ...'. In R14B onward they dropped the need for the leading erlang: (which specifies this function is in the erlang module). R14A should be avoided as it's an alpha release. The fact that Debian Squeeze uses it is unfortunate. If at all possible you should be running R13B04 or R14B03.

        Let me know if the patch works for you.

        Show
        Anthony Molinaro added a comment - Try applying the patch I just uploaded then just run make. Essentially the line which starts with 'error(struct_unmatched, ...' needs to be 'erlang:error(struct_unmatched, ...'. In R14B onward they dropped the need for the leading erlang: (which specifies this function is in the erlang module). R14A should be avoided as it's an alpha release. The fact that Debian Squeeze uses it is unfortunate. If at all possible you should be running R13B04 or R14B03. Let me know if the patch works for you.
        Hide
        Roger Meier added a comment -

        Yes, it works! Could you commit?

        Show
        Roger Meier added a comment - Yes, it works! Could you commit?
        Hide
        Anthony Molinaro added a comment -

        Okay, commited.

        Show
        Anthony Molinaro added a comment - Okay, commited.
        Hide
        Hudson added a comment -

        Integrated in Thrift #171 (See https://builds.apache.org/job/Thrift/171/)

        Show
        Hudson added a comment - Integrated in Thrift #171 (See https://builds.apache.org/job/Thrift/171/ )

          People

          • Assignee:
            Anthony Molinaro
            Reporter:
            Anatoly Kanivetsky
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development