Uploaded image for project: 'Tajo'
  1. Tajo
  2. TAJO-1681

Fix TajoDump invalid null check for database name

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.11.0
    • Component/s: None
    • Labels:
      None

      Description

      TajoDump invalid null check in isAcceptableDumpingDatabase

        Activity

        Hide
        githubbot ASF GitHub Bot added a comment -

        GitHub user charsyam opened a pull request:

        https://github.com/apache/tajo/pull/628

        TAJO-1681 Fix TajoDump invalid null check for database name

        isAcceptableDumpingDatabase return true even if databaseName is null
        so it cause exception in dumpDatabase

        You can merge this pull request into a Git repository by running:

        $ git pull https://github.com/charsyam/tajo feature/fix-bug-for-tajodump

        Alternatively you can review and apply these changes as the patch at:

        https://github.com/apache/tajo/pull/628.patch

        To close this pull request, make a commit to your master/trunk branch
        with (at least) the following in the commit message:

        This closes #628


        commit 6e7f2b319751ddeb0619648bba3f14c491978178
        Author: clark.kang <clark.kang@kakao.com>
        Date: 2015-07-13T17:21:28Z

        fix invalid null check


        Show
        githubbot ASF GitHub Bot added a comment - GitHub user charsyam opened a pull request: https://github.com/apache/tajo/pull/628 TAJO-1681 Fix TajoDump invalid null check for database name isAcceptableDumpingDatabase return true even if databaseName is null so it cause exception in dumpDatabase You can merge this pull request into a Git repository by running: $ git pull https://github.com/charsyam/tajo feature/fix-bug-for-tajodump Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tajo/pull/628.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #628 commit 6e7f2b319751ddeb0619648bba3f14c491978178 Author: clark.kang <clark.kang@kakao.com> Date: 2015-07-13T17:21:28Z fix invalid null check
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user blrunner commented on the pull request:

        https://github.com/apache/tajo/pull/628#issuecomment-121150343

        Hi @charsyam

        Could you rebase the patch against the master branch?

        Show
        githubbot ASF GitHub Bot added a comment - Github user blrunner commented on the pull request: https://github.com/apache/tajo/pull/628#issuecomment-121150343 Hi @charsyam Could you rebase the patch against the master branch?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user charsyam commented on the pull request:

        https://github.com/apache/tajo/pull/628#issuecomment-121246409

        @blrunner I fixed bug and rebased it Thank you.

        Show
        githubbot ASF GitHub Bot added a comment - Github user charsyam commented on the pull request: https://github.com/apache/tajo/pull/628#issuecomment-121246409 @blrunner I fixed bug and rebased it Thank you.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jihoonson commented on a diff in the pull request:

        https://github.com/apache/tajo/pull/628#discussion_r35478801

        — Diff: tajo-core/src/test/java/org/apache/tajo/cli/tools/TestTajoDump.java —
        @@ -64,4 +64,15 @@ public void testDump2() throws Exception

        { executeString("DROP TABLE \"" + getCurrentDatabase() + "\".\"TableName2\""); }

        }
        +
        + @Test
        + public void testDump3() throws Exception {
        + if (!testingCluster.isHiveCatalogStoreRunning()) {
        + UserRoleInfo userInfo = UserRoleInfo.getCurrentUser();
        + ByteArrayOutputStream bos = new ByteArrayOutputStream();
        + PrintWriter printWriter = new PrintWriter(bos);
        +
        + TajoDump.dump(client, userInfo, null, false, false, false, printWriter);
        — End diff –

        It would be nice if the dump result is verified. Please refer to other tests in this class.

        Show
        githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on a diff in the pull request: https://github.com/apache/tajo/pull/628#discussion_r35478801 — Diff: tajo-core/src/test/java/org/apache/tajo/cli/tools/TestTajoDump.java — @@ -64,4 +64,15 @@ public void testDump2() throws Exception { executeString("DROP TABLE \"" + getCurrentDatabase() + "\".\"TableName2\""); } } + + @Test + public void testDump3() throws Exception { + if (!testingCluster.isHiveCatalogStoreRunning()) { + UserRoleInfo userInfo = UserRoleInfo.getCurrentUser(); + ByteArrayOutputStream bos = new ByteArrayOutputStream(); + PrintWriter printWriter = new PrintWriter(bos); + + TajoDump.dump(client, userInfo, null, false, false, false, printWriter); — End diff – It would be nice if the dump result is verified. Please refer to other tests in this class.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jihoonson commented on the pull request:

        https://github.com/apache/tajo/pull/628#issuecomment-124790841

        @charsyam, sorry for late review.
        This patch looks good, but test seems to be improved.
        Please consider my comment.

        Show
        githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on the pull request: https://github.com/apache/tajo/pull/628#issuecomment-124790841 @charsyam, sorry for late review. This patch looks good, but test seems to be improved. Please consider my comment.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user charsyam commented on the pull request:

        https://github.com/apache/tajo/pull/628#issuecomment-124853307

        @jihoonson Thanks for your review
        But this patch just check null point exception of null parameters.
        and other 2 testcases check dump result is verified.

        so I think this test is more intuitive.
        and It can't get the result because test database name is null

        What do you think?

        Show
        githubbot ASF GitHub Bot added a comment - Github user charsyam commented on the pull request: https://github.com/apache/tajo/pull/628#issuecomment-124853307 @jihoonson Thanks for your review But this patch just check null point exception of null parameters. and other 2 testcases check dump result is verified. so I think this test is more intuitive. and It can't get the result because test database name is null What do you think?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jihoonson commented on the pull request:

        https://github.com/apache/tajo/pull/628#issuecomment-124909940

        I got your point. As you said, the dump result doesn't have to be verified since this ticket is related to fix wrong null checking on database name.
        If so, how about adding an error message when the given database name is wrong? I mean, ```isAcceptableDumpingDatabase()``` returns ```false```, it would be better to print a message to notify users that something is going wrong. In addition, we can verify error handling with that message.

        Show
        githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on the pull request: https://github.com/apache/tajo/pull/628#issuecomment-124909940 I got your point. As you said, the dump result doesn't have to be verified since this ticket is related to fix wrong null checking on database name. If so, how about adding an error message when the given database name is wrong? I mean, ```isAcceptableDumpingDatabase()``` returns ```false```, it would be better to print a message to notify users that something is going wrong. In addition, we can verify error handling with that message.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user charsyam commented on the pull request:

        https://github.com/apache/tajo/pull/628#issuecomment-124997712

        @jihoonson Thanks your review , Yes, I think It is good idea.
        but I still have a question about this behavior

        Because isAcceptableDumpingDatabase's return value doesn't mean error.
        it jsut check database name is not CatalogConstants.INFORMATION_SCHEMA_DB_NAME

        What do you think?
        but I found indent is wrong T.T I should fix it first

        Show
        githubbot ASF GitHub Bot added a comment - Github user charsyam commented on the pull request: https://github.com/apache/tajo/pull/628#issuecomment-124997712 @jihoonson Thanks your review , Yes, I think It is good idea. but I still have a question about this behavior Because isAcceptableDumpingDatabase's return value doesn't mean error. it jsut check database name is not CatalogConstants.INFORMATION_SCHEMA_DB_NAME What do you think? but I found indent is wrong T.T I should fix it first
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jihoonson commented on the pull request:

        https://github.com/apache/tajo/pull/628#issuecomment-125051108

        Oh, right. It was my misunderstanding.
        I still have a doubt whether the unit test verifies correctly.
        How about removing the test? I think it's enough because this change is straightforward.

        Show
        githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on the pull request: https://github.com/apache/tajo/pull/628#issuecomment-125051108 Oh, right. It was my misunderstanding. I still have a doubt whether the unit test verifies correctly. How about removing the test? I think it's enough because this change is straightforward.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user charsyam commented on the pull request:

        https://github.com/apache/tajo/pull/628#issuecomment-125078952

        @jihoonson Ok, I removed testcase and fix indent Thank you for your review

        Show
        githubbot ASF GitHub Bot added a comment - Github user charsyam commented on the pull request: https://github.com/apache/tajo/pull/628#issuecomment-125078952 @jihoonson Ok, I removed testcase and fix indent Thank you for your review
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jihoonson commented on the pull request:

        https://github.com/apache/tajo/pull/628#issuecomment-125463262

        +1. Thanks!

        Show
        githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on the pull request: https://github.com/apache/tajo/pull/628#issuecomment-125463262 +1. Thanks!
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user asfgit closed the pull request at:

        https://github.com/apache/tajo/pull/628

        Show
        githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/tajo/pull/628
        Hide
        jihoonson Jihoon Son added a comment -

        Committed to master.
        Thanks!

        Show
        jihoonson Jihoon Son added a comment - Committed to master. Thanks!
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Tajo-master-CODEGEN-build #412 (See https://builds.apache.org/job/Tajo-master-CODEGEN-build/412/)
        TAJO-1681: Fix TajoDump invalid null check for database name (jihoonson: rev ae9a8c9cb608c5f95d68a06e07e5969b8c9cf2d6)

        • tajo-cli/src/main/java/org/apache/tajo/cli/tools/TajoDump.java
        • CHANGES
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Tajo-master-CODEGEN-build #412 (See https://builds.apache.org/job/Tajo-master-CODEGEN-build/412/ ) TAJO-1681 : Fix TajoDump invalid null check for database name (jihoonson: rev ae9a8c9cb608c5f95d68a06e07e5969b8c9cf2d6) tajo-cli/src/main/java/org/apache/tajo/cli/tools/TajoDump.java CHANGES
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Tajo-master-build #775 (See https://builds.apache.org/job/Tajo-master-build/775/)
        TAJO-1681: Fix TajoDump invalid null check for database name (jihoonson: rev ae9a8c9cb608c5f95d68a06e07e5969b8c9cf2d6)

        • tajo-cli/src/main/java/org/apache/tajo/cli/tools/TajoDump.java
        • CHANGES
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Tajo-master-build #775 (See https://builds.apache.org/job/Tajo-master-build/775/ ) TAJO-1681 : Fix TajoDump invalid null check for database name (jihoonson: rev ae9a8c9cb608c5f95d68a06e07e5969b8c9cf2d6) tajo-cli/src/main/java/org/apache/tajo/cli/tools/TajoDump.java CHANGES

          People

          • Assignee:
            charsyam DaeMyung Kang
            Reporter:
            charsyam DaeMyung Kang
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development