Velocity
  1. Velocity
  2. VELOCITY-449

Velocity Uberspector behaves differently for get(String) and put(String, Object) methods

    Details

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

      Description

      Consider an Object

      public class Test {
      private Object foo;

      public Object get(String dummy)

      { return foo; }

      public void put(String dummy, Object foo)

      { this.foo = foo }

      ;
      }

      Put this object into a Velocity Context as $test, add a HashMap as $map.

      Use the following template:

      $map.put("key", "val1")
      $test.put("key", "val1")

      $map.get("key") --> Returns val1
      $test.get("key") --> Returns val1

      $map.key --> Returns val1
      $test.key --> Returns val1

      #set ($map.key = "val2")
      #set ($test.key = "val2")

      $map.key --> Returns val2
      $test.key --> Returns val1 !!

      The reason for this is, that the UberspectorImpl, in getPropertySet tests in line 247 whether the passed object is assignable to a Map. This test is no in the getPropertyGet
      and seems to have no reason because the following method lookup for "put" will succeed anyway.

      I'd suggest the removal of this test.

        Activity

        Hide
        Nathan Bubna added a comment -

        Agreed. We should remove the test for Map assignability. If public Object put(Object, Object) is there, it should work whether or not it is a Map.

        Show
        Nathan Bubna added a comment - Agreed. We should remove the test for Map assignability. If public Object put(Object, Object) is there, it should work whether or not it is a Map.
        Hide
        Will Glass-Husain added a comment -

        why?

        "put" is part of the Map interface. It makes sense to use it if the object is a map.

        But otherwise, we follow (more or less) javabean semantics. "put" has no meaning there. If a POJO has a "put" object we should ignore it, I think.

        Show
        Will Glass-Husain added a comment - why? "put" is part of the Map interface. It makes sense to use it if the object is a map. But otherwise, we follow (more or less) javabean semantics. "put" has no meaning there. If a POJO has a "put" object we should ignore it, I think.
        Hide
        Nathan Bubna added a comment -

        get(key) is also part of the map interface. yet we give it special syntactic treatment on all objects, map or not. if a put(Object, Object) method exists, it is not meaningless. it is meant to be used. why force a different syntax for an identical method?

        i really think that given our support of get() on any object; the burden of explanation is on the restriction to Map.

        "treat like methods alike" seems like a better, simpler, and less surprising policy than "treat like methods alike unless they fail to declare that they implement a particular interface"

        Show
        Nathan Bubna added a comment - get(key) is also part of the map interface. yet we give it special syntactic treatment on all objects, map or not. if a put(Object, Object) method exists, it is not meaningless. it is meant to be used. why force a different syntax for an identical method? i really think that given our support of get() on any object; the burden of explanation is on the restriction to Map. "treat like methods alike" seems like a better, simpler, and less surprising policy than "treat like methods alike unless they fail to declare that they implement a particular interface"
        Hide
        Will Glass-Husain added a comment -

        ah, forgot that get(string) is not a javabean semantic.

        ok, makes sense. in general, I agree that we should keep things easy for users rather than force rules on them. I withdraw my objection.

        Show
        Will Glass-Husain added a comment - ah, forgot that get(string) is not a javabean semantic. ok, makes sense. in general, I agree that we should keep things easy for users rather than force rules on them. I withdraw my objection.
        Hide
        Henning Schmiedehausen added a comment -

        There is an actual use case for that. I have a number of Turbine tools that offer

        — cut —
        public void get(String key)

        { ... }

        public void getFoo() { }

        public void getBar() { }

        — cut —

        Being able to write $tool.Foo $tool.Bar $tool.Baz with the first two mapped to their explicit getters and the last one to get("Baz") is very useful. Having the same thing for setters makes it even more useful.

        Show
        Henning Schmiedehausen added a comment - There is an actual use case for that. I have a number of Turbine tools that offer — cut — public void get(String key) { ... } public void getFoo() { } public void getBar() { } — cut — Being able to write $tool.Foo $tool.Bar $tool.Baz with the first two mapped to their explicit getters and the last one to get("Baz") is very useful. Having the same thing for setters makes it even more useful.
        Hide
        Alexey Panchenko added a comment -

        There should be two implementation clasess: one for objects implementing Map and another one using reflection for objects implementing just the method put(String,Object). The same for get(String).

        Show
        Alexey Panchenko added a comment - There should be two implementation clasess: one for objects implementing Map and another one using reflection for objects implementing just the method put(String,Object). The same for get(String).
        Hide
        Henning Schmiedehausen added a comment -

        I hope I got you right, that you suggest to have additional executors that are smart about maps. I added code for that. If it was not what you meant, feel free to reopen that issue.

        Show
        Henning Schmiedehausen added a comment - I hope I got you right, that you suggest to have additional executors that are smart about maps. I added code for that. If it was not what you meant, feel free to reopen that issue.
        Hide
        Henning Schmiedehausen added a comment -

        Patch will be in 1.5 release.

        Show
        Henning Schmiedehausen added a comment - Patch will be in 1.5 release.

          People

          • Assignee:
            Unassigned
            Reporter:
            Henning Schmiedehausen
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development