Accumulo
  1. Accumulo
  2. ACCUMULO-2299

WholeRowIterator.decodeRow should check input stream return val

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.4.4, 1.5.0
    • Fix Version/s: 1.5.1, 1.6.0
    • Component/s: None
    • Labels:

      Description

      The decodeRow method ignores the return code on the DataInputStream.read calls. If that return value is inspected, we can catch errors where the data is malformed.

      1. ACCUMULO-2299.v1.patch.txt
        3 kB
        Vikram Srivastava
      2. ACCUMULO-2299.v2.patch.txt
        5 kB
        Vikram Srivastava
      3. ACCUMULO-2299.v3.patch.txt
        6 kB
        Vikram Srivastava

        Activity

        Josh Elser created issue -
        Vikram Srivastava made changes -
        Field Original Value New Value
        Assignee Vikram Srivastava [ vickyuec ]
        Hide
        Vikram Srivastava added a comment -

        Attached patch. Verified WRIteratorTest still passes.

        Show
        Vikram Srivastava added a comment - Attached patch. Verified WRIteratorTest still passes.
        Vikram Srivastava made changes -
        Attachment ACCUMULO-2299.v1.patch.txt [ 12626601 ]
        Vikram Srivastava made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Fix Version/s 1.5.1 [ 12324399 ]
        Hide
        Mike Drob added a comment -

        Vikram Srivastava, can you add a unit test to assert that an exception is thrown on malformed data?

        Show
        Mike Drob added a comment - Vikram Srivastava , can you add a unit test to assert that an exception is thrown on malformed data?
        Hide
        Vikram Srivastava added a comment -

        Attached patch with unit test.

        Show
        Vikram Srivastava added a comment - Attached patch with unit test.
        Vikram Srivastava made changes -
        Attachment ACCUMULO-2299.v2.patch.txt [ 12626827 ]
        Hide
        Josh Elser added a comment -

        It would be better to use the expected parameter with the IllegalArgumentException on the JUnit Test annotation instead of the

        try {
          do();
          fail(); 
        } catch  {
        //pass 
        }
        

        The test case you wrote is subject to the serialization of the WholeRowIterator. I would think that it would be cleaner to serialize some key-values, truncate the byte array you pass to the decode method and test for a failure there. Then, you're not encoding serialization logic into your test case.

        Also, do we want to throw an IllegalArgumentException, or should it just throw an IOException that's already in the method signature? I'm leaning towards the latter at first thought, but I'm not entirely sure.

        Show
        Josh Elser added a comment - It would be better to use the expected parameter with the IllegalArgumentException on the JUnit Test annotation instead of the try { do (); fail(); } catch { //pass } The test case you wrote is subject to the serialization of the WholeRowIterator. I would think that it would be cleaner to serialize some key-values, truncate the byte array you pass to the decode method and test for a failure there. Then, you're not encoding serialization logic into your test case. Also, do we want to throw an IllegalArgumentException, or should it just throw an IOException that's already in the method signature? I'm leaning towards the latter at first thought, but I'm not entirely sure.
        Hide
        John Vines added a comment -

        I think ioexception is better. It is an issue with the input to it and
        they're more acceptable in the iterator stack than Rte in my opinion

        Sent from my phone, please pardon the typos and brevity.

        Show
        John Vines added a comment - I think ioexception is better. It is an issue with the input to it and they're more acceptable in the iterator stack than Rte in my opinion Sent from my phone, please pardon the typos and brevity.
        Hide
        Vikram Srivastava added a comment -

        Josh Elser Thanks for the feedback.
        1. I didn't use expected parameter because the test class was inheriting TestCase instead of using @Test. But I can change the class to use @Test unless anyone has objections.
        2. To be clear, you are asking about testing the new "readField" method in isolation with the truncated byte array, right?
        3. I don't feel strongly against using either exception. I'll change it to IOException.

        Show
        Vikram Srivastava added a comment - Josh Elser Thanks for the feedback. 1. I didn't use expected parameter because the test class was inheriting TestCase instead of using @Test. But I can change the class to use @Test unless anyone has objections. 2. To be clear, you are asking about testing the new "readField" method in isolation with the truncated byte array, right? 3. I don't feel strongly against using either exception. I'll change it to IOException.
        Hide
        Josh Elser added a comment -

        I didn't use expected parameter because the test class was inheriting TestCase instead of using @Test. But I can change the class to use @Test unless anyone has objections.

        Please do. Anything still extending TestCase is just hold over from old days. There shouldn't be any reason that I'm aware of that we would still want to extend TestCase.

        To be clear, you are asking about testing the new "readField" method in isolation with the truncated byte array, right?

        Yes. My thought was to create some Keys and Values, and serialize them into a single Value using encodeRow. Truncate the byte array from that Value, and pass it back to decodeRow and expect the exception that should be thrown from readField

        Show
        Josh Elser added a comment - I didn't use expected parameter because the test class was inheriting TestCase instead of using @Test. But I can change the class to use @Test unless anyone has objections. Please do. Anything still extending TestCase is just hold over from old days. There shouldn't be any reason that I'm aware of that we would still want to extend TestCase. To be clear, you are asking about testing the new "readField" method in isolation with the truncated byte array, right? Yes. My thought was to create some Keys and Values, and serialize them into a single Value using encodeRow . Truncate the byte array from that Value, and pass it back to decodeRow and expect the exception that should be thrown from readField
        Hide
        Vikram Srivastava added a comment -

        Took care of feedback for v2.

        Show
        Vikram Srivastava added a comment - Took care of feedback for v2.
        Vikram Srivastava made changes -
        Attachment ACCUMULO-2299.v3.patch.txt [ 12627061 ]
        Hide
        Josh Elser added a comment -

        LGTM at a glance. I'll let Mike have a chance to look at it too.

        Show
        Josh Elser added a comment - LGTM at a glance. I'll let Mike have a chance to look at it too.
        Hide
        ASF subversion and git services added a comment -

        Commit b93032190b4cc1bd00c7c0faf121899464c80919 in branch refs/heads/1.5.1-SNAPSHOT from Vikram Srivastava
        [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=b930321 ]

        ACCUMULO-2299 Check that read field value has same length as expected

        Signed-off-by: Mike Drob <mdrob@cloudera.com>

        Show
        ASF subversion and git services added a comment - Commit b93032190b4cc1bd00c7c0faf121899464c80919 in branch refs/heads/1.5.1-SNAPSHOT from Vikram Srivastava [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=b930321 ] ACCUMULO-2299 Check that read field value has same length as expected Signed-off-by: Mike Drob <mdrob@cloudera.com>
        Hide
        ASF subversion and git services added a comment -

        Commit b93032190b4cc1bd00c7c0faf121899464c80919 in branch refs/heads/1.6.0-SNAPSHOT from Vikram Srivastava
        [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=b930321 ]

        ACCUMULO-2299 Check that read field value has same length as expected

        Signed-off-by: Mike Drob <mdrob@cloudera.com>

        Show
        ASF subversion and git services added a comment - Commit b93032190b4cc1bd00c7c0faf121899464c80919 in branch refs/heads/1.6.0-SNAPSHOT from Vikram Srivastava [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=b930321 ] ACCUMULO-2299 Check that read field value has same length as expected Signed-off-by: Mike Drob <mdrob@cloudera.com>
        Hide
        ASF subversion and git services added a comment -

        Commit b93032190b4cc1bd00c7c0faf121899464c80919 in branch refs/heads/master from Vikram Srivastava
        [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=b930321 ]

        ACCUMULO-2299 Check that read field value has same length as expected

        Signed-off-by: Mike Drob <mdrob@cloudera.com>

        Show
        ASF subversion and git services added a comment - Commit b93032190b4cc1bd00c7c0faf121899464c80919 in branch refs/heads/master from Vikram Srivastava [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=b930321 ] ACCUMULO-2299 Check that read field value has same length as expected Signed-off-by: Mike Drob <mdrob@cloudera.com>
        Mike Drob made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Fix Version/s 1.6.0 [ 12322468 ]
        Resolution Fixed [ 1 ]

          People

          • Assignee:
            Vikram Srivastava
            Reporter:
            Josh Elser
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development