Pivot
  1. Pivot
  2. PIVOT-764

BeanAdapter should support Map objects

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.0.1
    • Component/s: core-beans
    • Labels:
      None

      Description

      BeanAdapter currently does not support the ability to wrap Maps, because it will fail to retrieve the correct getterMethod and setterMethod. My current usecase is that I have a TableView with tableData that consists of objects with path "value-of-type-map.subvalue". The following code should return John Doe in my opinion:

      HashMap root = new HashMap();
      HashMap child = new HashMap();
      child.put("name", "John Doe");
      root.put("child", child);
      System.out.println(JSON.get(root, "child.name"));

      I will provide a patch.

        Activity

        Hide
        Edvin Syse added a comment -

        Patch that adds Map support to BeanAdapter. (IntelliJ IDEA optimized the imports a bit, please let me know if I should create a patch without the optimizations).

        Show
        Edvin Syse added a comment - Patch that adds Map support to BeanAdapter. (IntelliJ IDEA optimized the imports a bit, please let me know if I should create a patch without the optimizations).
        Hide
        Greg Brown added a comment -

        This is not a use case for BeanAdapter. Use JSON.get() for this.

        Show
        Greg Brown added a comment - This is not a use case for BeanAdapter. Use JSON.get() for this.
        Hide
        Edvin Syse added a comment -

        How can I use JSON.get() with TableView to overcome this problem?

        Show
        Edvin Syse added a comment - How can I use JSON.get() with TableView to overcome this problem?
        Hide
        Greg Brown added a comment -

        That is not clear. We first need to understand the nature of the problem. As demonstrated in the example I sent to the mailing list, JSON.get() will correctly retrieve nested map values. So something else must be amiss here.

        Show
        Greg Brown added a comment - That is not clear. We first need to understand the nature of the problem. As demonstrated in the example I sent to the mailing list, JSON.get() will correctly retrieve nested map values. So something else must be amiss here.
        Hide
        Edvin Syse added a comment -

        I think I wasn't clear on that my domain object, and the HashMap from my example code, is java.util.Map, not org.apache.pivot.collections.Map. I agree that the code example works if I use Pivot's Map, but that's not the case for any real world domain object My patch adds support for java.util.Map. I see now that it doesn't need to check for pivot.collections.Map, so it can be simplified a bit.

        Show
        Edvin Syse added a comment - I think I wasn't clear on that my domain object, and the HashMap from my example code, is java.util.Map, not org.apache.pivot.collections.Map. I agree that the code example works if I use Pivot's Map, but that's not the case for any real world domain object My patch adds support for java.util.Map. I see now that it doesn't need to check for pivot.collections.Map, so it can be simplified a bit.
        Hide
        Edvin Syse added a comment -

        Patch against JSON to support java.util.Map

        Show
        Edvin Syse added a comment - Patch against JSON to support java.util.Map
        Hide
        Edvin Syse added a comment -

        After discussion on the user list, and suggestion from Sandro, I've reopened this ticket and attached a patch against JSON instead

        Show
        Edvin Syse added a comment - After discussion on the user list, and suggestion from Sandro, I've reopened this ticket and attached a patch against JSON instead
        Hide
        Greg Brown added a comment -

        Other than the misleading variable name ("beanAdapter") this looks good.

        Show
        Greg Brown added a comment - Other than the misleading variable name ("beanAdapter") this looks good.
        Hide
        Edvin Syse added a comment -

        Yes, I mentioned that on the mailinglist. What would you prefer? Simply "wrapper", maybe?

        Show
        Edvin Syse added a comment - Yes, I mentioned that on the mailinglist. What would you prefer? Simply "wrapper", maybe?
        Hide
        Greg Brown added a comment -

        > Yes, I mentioned that on the mailinglist.

        Yes, I know.

        Maybe "adapter"? Or just "map".

        Show
        Greg Brown added a comment - > Yes, I mentioned that on the mailinglist. Yes, I know. Maybe "adapter"? Or just "map".
        Hide
        Edvin Syse added a comment -

        Patch with better local variable name (adapter)

        Show
        Edvin Syse added a comment - Patch with better local variable name (adapter)
        Hide
        Edvin Syse added a comment -

        I think "adapter" sounds better, map is more generic New patch is attached, deleted the old one.

        Show
        Edvin Syse added a comment - I think "adapter" sounds better, map is more generic New patch is attached, deleted the old one.
        Hide
        Edvin Syse added a comment -

        Patch against JSON to support java.util.Map.

        Show
        Edvin Syse added a comment - Patch against JSON to support java.util.Map.
        Hide
        Sandro Martini added a comment -

        Hi Edvin,
        have you commit rights now ?
        If not tell me, I'll commit in your place ...

        Sorry, have you got even a simple test so I can commit even it (I can convert it to JUnit 4.x syntax), or the simple block of code put over here is enough ?

        Bye

        Show
        Sandro Martini added a comment - Hi Edvin, have you commit rights now ? If not tell me, I'll commit in your place ... Sorry, have you got even a simple test so I can commit even it (I can convert it to JUnit 4.x syntax), or the simple block of code put over here is enough ? Bye
        Hide
        Edvin Syse added a comment -

        I don't have commit rights yet, waiting for Greg to get back to me, I've delivered the ICLA though, Can you commit?

        Yes, the code above will actually be enough to test this. Can you create a small unit test from that code, just make sure you import java.util.HashMap, and assert that JSON.get returns "John Doe"?

        Show
        Edvin Syse added a comment - I don't have commit rights yet, waiting for Greg to get back to me, I've delivered the ICLA though, Can you commit? Yes, the code above will actually be enough to test this. Can you create a small unit test from that code, just make sure you import java.util.HashMap, and assert that JSON.get returns "John Doe"?
        Hide
        Sandro Martini added a comment -

        Ok, I'll commit this evening or max tomorrow ...
        Thanks for the info.

        Bye

        Show
        Sandro Martini added a comment - Ok, I'll commit this evening or max tomorrow ... Thanks for the info. Bye
        Hide
        Sandro Martini added a comment -

        Just committed, with a simple unit test.
        Many thanks to Edvin.

        Bye

        Show
        Sandro Martini added a comment - Just committed, with a simple unit test. Many thanks to Edvin. Bye

          People

          • Assignee:
            Sandro Martini
            Reporter:
            Edvin Syse
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development