Details

    • Bug
    • Status: Closed
    • Major
    • Resolution: Fixed
    • 3.2.1
    • 3.2.2
    • process
    • None

    Description

      profile() fails when used in conjunction with withPath():

      gremlin> g.withPath().V().profile()
      When specified, the profile()-Step must be the last step or followed only by the cap()-step.
      Display stack trace? [yN]
      

      Attachments

        Issue Links

          Activity

            Is it interesting that this problem doesn't seem to occur in 3.1.3?

            gremlin> g.withPath().V().profile()
            ==>v[1]
            ==>v[2]
            ==>v[3]
            ==>v[4]
            ==>v[5]
            ==>v[6]
            
            spmallette Stephen Mallette added a comment - Is it interesting that this problem doesn't seem to occur in 3.1.3? gremlin> g.withPath().V().profile() ==>v[1] ==>v[2] ==>v[3] ==>v[4] ==>v[5] ==>v[6]
            twilmes Ted Wilmes added a comment -

            I was wondering if the PathRetractionStrategy was causing issues with this so I took a look and it looks like it's actually having an issue because the withPath adds a RequirementsStep to the tail end of the traversal which breaks some of the StandardVerfificationStrategy logic. Looks like a straightforward fix so I can put a PR in shortly if something isn't already in the works.

            twilmes Ted Wilmes added a comment - I was wondering if the PathRetractionStrategy was causing issues with this so I took a look and it looks like it's actually having an issue because the withPath adds a RequirementsStep to the tail end of the traversal which breaks some of the StandardVerfificationStrategy logic. Looks like a straightforward fix so I can put a PR in shortly if something isn't already in the works.
            githubbot ASF GitHub Bot added a comment -

            GitHub user twilmes opened a pull request:

            https://github.com/apache/tinkerpop/pull/381

            TINKERPOP-1405 - profile() doesn't like withPath()

            Fixed a small bug in `StandardVerificationStrategy` that caused verification to fail when `withPath` was used in conjunction with `ProfileStep`. Updated `StandardVerificationStrategyTest` to test traversals with `ProfileStep` and added a test to confirm verification failed on a reducing barrier nested within a repeat while I was at it.

            This issue did not arise in TP31 so I took a look and the TP31 `StandardVerificationStrategy` does not have the `ProfileStep` and some other verification verification logic in it that is in master. Because of this, I pointed this PR only at master but can re-point to TP31 and add the profile step verification in if you guys think that makes sense.

            The Travis build appears to have stalled out while downloading artifacts. Tests passed local with `mvn clean install`

            ```
            [INFO] ------------------------------------------------------------------------
            [INFO] BUILD SUCCESS
            [INFO] ------------------------------------------------------------------------
            [INFO] Total time: 24:25 min
            [INFO] Finished at: 2016-08-16T08:43:01-05:00
            [INFO] Final Memory: 140M/843M
            [INFO] ------------------------------------------------------------------------
            ```

            VOTE: +1

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

            $ git pull https://github.com/apache/tinkerpop TINKERPOP-1405

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

            https://github.com/apache/tinkerpop/pull/381.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 #381


            commit 226d83e903dc16f5d6da644d7069bb1b54c3878b
            Author: Ted Wilmes <twilmes@gmail.com>
            Date: 2016-08-16T13:04:28Z

            TINKERPOP-1405 Fixed a small bug in StandardVerificationStrategy that caused verification to fail when withPath was used in conjunction with ProfileStep.


            githubbot ASF GitHub Bot added a comment - GitHub user twilmes opened a pull request: https://github.com/apache/tinkerpop/pull/381 TINKERPOP-1405 - profile() doesn't like withPath() Fixed a small bug in `StandardVerificationStrategy` that caused verification to fail when `withPath` was used in conjunction with `ProfileStep`. Updated `StandardVerificationStrategyTest` to test traversals with `ProfileStep` and added a test to confirm verification failed on a reducing barrier nested within a repeat while I was at it. This issue did not arise in TP31 so I took a look and the TP31 `StandardVerificationStrategy` does not have the `ProfileStep` and some other verification verification logic in it that is in master. Because of this, I pointed this PR only at master but can re-point to TP31 and add the profile step verification in if you guys think that makes sense. The Travis build appears to have stalled out while downloading artifacts. Tests passed local with `mvn clean install` ``` [INFO] ------------------------------------------------------------------------ [INFO] BUILD SUCCESS [INFO] ------------------------------------------------------------------------ [INFO] Total time: 24:25 min [INFO] Finished at: 2016-08-16T08:43:01-05:00 [INFO] Final Memory: 140M/843M [INFO] ------------------------------------------------------------------------ ``` VOTE: +1 You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/tinkerpop TINKERPOP-1405 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tinkerpop/pull/381.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 #381 commit 226d83e903dc16f5d6da644d7069bb1b54c3878b Author: Ted Wilmes <twilmes@gmail.com> Date: 2016-08-16T13:04:28Z TINKERPOP-1405 Fixed a small bug in StandardVerificationStrategy that caused verification to fail when withPath was used in conjunction with ProfileStep.
            githubbot ASF GitHub Bot added a comment -

            Github user okram commented on the issue:

            https://github.com/apache/tinkerpop/pull/381

            Nice find. I really do hate `RequirementsStep`.

            VOTE +1.

            githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/381 Nice find. I really do hate `RequirementsStep`. VOTE +1.
            githubbot ASF GitHub Bot added a comment -

            Github user dkuppitz commented on the issue:

            https://github.com/apache/tinkerpop/pull/381

            Lots of calls to ` traversal.asAdmin().getEndStep()`. Maybe it's smarter to call it only once..? More like this:

            ```
            // The ProfileSideEffectStep must be the last step, 2nd last step when accompanied by the cap step,
            // or 3rd to last when the traversal ends with a RequirementsStep.
            Step endStep;
            if (TraversalHelper.hasStepOfClass(ProfileSideEffectStep.class, traversal) &&
                 !((endStep = traversal.asAdmin().getEndStep()) instanceof ProfileSideEffectStep ||
                           (endStep instanceof SideEffectCapStep && endStep.getPreviousStep() instanceof ProfileSideEffectStep) ||
                           (endStep instanceof RequirementsStep && (
                                   endStep.getPreviousStep() instanceof SideEffectCapStep ||
                                   endStep.getPreviousStep() instanceof ProfileSideEffectStep))))

            {    throw new VerificationException("When specified, the profile()-Step must be the last step or followed only by the cap()-step and/or requirements step.", traversal); }

            ```

            ^ I also removed all the negations, which makes the chained conditions a bit easier to read IMO.

            But all in all this PR looks good to me.

            VOTE: +1

            githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the issue: https://github.com/apache/tinkerpop/pull/381 Lots of calls to ` traversal.asAdmin().getEndStep()`. Maybe it's smarter to call it only once..? More like this: ``` // The ProfileSideEffectStep must be the last step, 2nd last step when accompanied by the cap step, // or 3rd to last when the traversal ends with a RequirementsStep. Step endStep; if (TraversalHelper.hasStepOfClass(ProfileSideEffectStep.class, traversal) &&      !((endStep = traversal.asAdmin().getEndStep()) instanceof ProfileSideEffectStep ||                (endStep instanceof SideEffectCapStep && endStep.getPreviousStep() instanceof ProfileSideEffectStep) ||                (endStep instanceof RequirementsStep && (                        endStep.getPreviousStep() instanceof SideEffectCapStep ||                        endStep.getPreviousStep() instanceof ProfileSideEffectStep)))) {    throw new VerificationException("When specified, the profile()-Step must be the last step or followed only by the cap()-step and/or requirements step.", traversal); } ``` ^ I also removed all the negations, which makes the chained conditions a bit easier to read IMO. But all in all this PR looks good to me. VOTE: +1
            githubbot ASF GitHub Bot added a comment -

            Github user twilmes commented on the issue:

            https://github.com/apache/tinkerpop/pull/381

            Good suggestions Daniel. I pushed the cleaned up code. Tests were good on my side but I'll wait for Travis to finish up and merge if all is good.

            githubbot ASF GitHub Bot added a comment - Github user twilmes commented on the issue: https://github.com/apache/tinkerpop/pull/381 Good suggestions Daniel. I pushed the cleaned up code. Tests were good on my side but I'll wait for Travis to finish up and merge if all is good.
            githubbot ASF GitHub Bot added a comment -

            Github user asfgit closed the pull request at:

            https://github.com/apache/tinkerpop/pull/381

            githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/tinkerpop/pull/381

            People

              twilmes Ted Wilmes
              dkuppitz Daniel Kuppitz
              Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: