Velocity
  1. Velocity
  2. VELOCITY-404

Uberspect - info is created wrong / premature blocking of invalid methods

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.4
    • Fix Version/s: 1.5
    • Component/s: Engine
    • Labels:
      None

      Description

      Reported and patch submitted by Llwellyn Falco & Dan Powell. Email below:

      Sorry,

      apparently i am being a bit confusing.

      the patch was rather large (six lines of code) so i can understand why i
      should have explained it more clearly,

      The patch is against 3 issues, 1 of them i submitted when the old
      bugzilla, the other 2 don't exist. but there is a unit test per each issue
      submitted with the patch.
      don't quite understand why i would create an issue merely to close it. i
      mean if i needed to show my boss i was working the busy work would make
      sense, but as we are all doing this
      in our free time.... if you feel you need notes that better explain the
      patch than the unit tests, by all means fell free, but i personally can't
      think what i would write that is more precise than actual code.

      issue 1 (this is Velocity-381)

      [ see Velocity-381 ]

      issue 2 (no jira issue)
      the info is created wrong.
      code change


      • method = rsvc.getUberspect().getMethod(o, methodName,
        params, new Info("",1,1));
        + method = rsvc.getUberspect().getMethod(o, methodName,
        params, new Info(context.getCurrentTemplateName(), getLine(), getColumn()));

        the test checks against an uberspect that throws exceptions with it can't
        find stuff.
        (this is also useful for development, but i am trying to start with small
        changes as to keep things simple, so kept it to testing)

        public Info getInfoFor(String velocity) throws Exception

        Unknown macro: { try { parseString(velocity, this); fail("Uberspect Should have thrown an exception"); throw new Error("Shouldn't be able to reach this point"); } catch (VelocityParsingError t) { return t.getInfo(); } }

      public void testInfoForField() throws Exception

      { Info i = getInfoFor("$main.unknownField"); assertInfoEqual(i, "$main.unknownField", 1, 7); }

      public void testInfoForMethod() throws Exception

      { Info i = getInfoFor("$main.unknownMethod()"); assertInfoEqual(i, "$main.unknownMethod()", 1, 7); }

      private void assertInfoEqual(Info i, String name, int line, int column)

      { assertEquals("Template Name", name, i.getTemplateName()); assertEquals("Template Line", line, i.getLine()); assertEquals("Template Column", column, i.getColumn()); }

      The third test also deals with the uberspect. design indicates that the
      uberspect should be asked to find the method, and therefore if you wanted a
      to write an uberspect to check when you are calling methods on null's you
      could write one. but the parser blocked it.
      the test is

      public void testNullPointer()

      { assertErrorThrown("$main.getNull().callMethod()"); }

      private void assertErrorThrown(String string) {
      try

      { String result = parseString(string, this); Assert.fail("parsing '" + string + "' did not fail but returned '" + result + "'"); }

      catch (Throwable t)

      { return; }

      }


      the code change is the removal of premature exit

      • if (result == null)
      • { - return null; - }
      1. StringResourceLoader.java
        0.8 kB
        Llewellyn Falco
      2. patch1.txt
        3 kB
        Will Glass-Husain
      3. InfoOnExceptionUberspect.java
        1 kB
        Llewellyn Falco
      4. InfoOnExceptionUberspect.java
        1 kB
        Llewellyn Falco
      5. InfoOnExceptionTest.java
        3 kB
        Llewellyn Falco

        Activity

        Hide
        Will Glass-Husain added a comment -

        patch submitted by Llewellyn Falco and Dan Powell on dev list

        Show
        Will Glass-Husain added a comment - patch submitted by Llewellyn Falco and Dan Powell on dev list
        Hide
        Will Glass-Husain added a comment -

        Thanks - now I see what your test is trying to do.

        Probably we'll reformat the test to make it similar to the other tests, removing the parseString method and StringResourceLoader. (its simpler and easier to be consistent). Typically, to test single strings you can use Velocity.evaluate. For more complicated templates, you can stick the template in the test directory and call it similar to EncodingTestCase.

        Show
        Will Glass-Husain added a comment - Thanks - now I see what your test is trying to do. Probably we'll reformat the test to make it similar to the other tests, removing the parseString method and StringResourceLoader. (its simpler and easier to be consistent). Typically, to test single strings you can use Velocity.evaluate. For more complicated templates, you can stick the template in the test directory and call it similar to EncodingTestCase.
        Hide
        Will Glass-Husain added a comment -

        Committed the patch. In a nutshell, this patch passes the template info into the Uberspector properly. Incidentally the UberspectTest class and throwing an Error was a clever way of testing this.

        You might take a look at how I reformatted the tests for future reference. The simplest thing to do would have been to use Velocity.evaluate() but that didn't seem to use the new uberspector. Instead I added the templates as files. This way it all works the same as everything else. I also did some minor reformatting to make the braces follow the Velocity coding conventions.

        I forgot to list the bug number in the svn commit, so note this is revision 291378.

        Thanks Llewellyn and Dan!

        Show
        Will Glass-Husain added a comment - Committed the patch. In a nutshell, this patch passes the template info into the Uberspector properly. Incidentally the UberspectTest class and throwing an Error was a clever way of testing this. You might take a look at how I reformatted the tests for future reference. The simplest thing to do would have been to use Velocity.evaluate() but that didn't seem to use the new uberspector. Instead I added the templates as files. This way it all works the same as everything else. I also did some minor reformatting to make the braces follow the Velocity coding conventions. I forgot to list the bug number in the svn commit, so note this is revision 291378. Thanks Llewellyn and Dan!
        Hide
        Henning Schmiedehausen added a comment -

        Close all resolved issues for Engine 1.5 release.

        Show
        Henning Schmiedehausen added a comment - Close all resolved issues for Engine 1.5 release.

          People

          • Assignee:
            Will Glass-Husain
            Reporter:
            Will Glass-Husain
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development