Breaking change in release 0.7.9 caused by new renderer helper

pmenhart commented 5 years ago

I am scratching my head about solving the problem #132 by copying the whole map to JSON and back to map, as implemented in #136 (details on line 347-348 ).

Performance impact aside (double transformation is expensive, especially if only a small subset of values is needed in the template), this change has serious compatibility consequences.

In the old version, scalar values are retrieved using Map.get() method (in liqp.nodes.LookupNode.Indexable#get).

Since PR #136, this mechanism is replaced with calling parseSettings.mapper.convertValue(), dumping all variables to Jackson representation. Under the hood, Map.entrySet() is called. Any key not returned by entrySet is assigned an empty value.

What are the consequences? Any instance of a class implementing Map interface can be added to template variables. For example, we are using this approach with wrappers around a large and deep hierarchy of domain classes. Each map-wrapper has a get() method to provide proxied or dynamically calculated values, possibly returning other map instances at a lower level of hierarchy. Some get() calculations are very costly, but only the values that are really needed in rendering will be called. Returning all values via entrySet() is prohibitively expensive, making release 0.7.9 unusable.

Instead of double dumping the whole map hierarchy, wouldn't it be much simpler to enhance liqp.nodes.LookupNode.Indexable to deal with POJO objects? If value.class is neither Map nor TemplateContext, then try a getter method or a public field to obtain the "index".

This would elegantly solve the MapWithPojos problem given in #132. Unfortunately there wasn't much discussion in that ticket - please let me know whether I am missing something.

msangel commented 5 years ago

Have only one opinion: It's better to keep everything as Lists and Maps in data models, so all the stuff around (filters for arrays, and so on) will have limited types of entities to work with.

Regarding that fact that this is breaking change - agree, but anycase before it haven't worked as expected.

msangel commented 5 years ago

For example: open implementation of map filter. And now assume that we must handle raw properties of POJOs... When all is map or list/array - its much simple here.

pmenhart commented 5 years ago

If I understand your opinion correctly, POJOs should not be allowed in data models. Instead of library being responsible for that, the caller has to explicitly convert the POJO object to a map (or wrap the object with a Map wrapper, as I mentioned above).

I rewrote example from #132 with an explicit user conversion. For convenience, I am using com.fasterxml.jackson.databind.ObjectMapper already available as liqp dependency:

    static class Foo {
        public String a = "A";
    }

    private Map pojo2Map(Object o) {
        ObjectMapper objectMapper = new ObjectMapper();
        ObjectNode oNode = objectMapper.convertValue(o, ObjectNode.class);
        return objectMapper.convertValue(oNode, Map.class);
    }

    @Test
    public void renderMapWithPojosExistedThrowError() {

        Map<String, Object> data = new HashMap<String, Object>();
        data.put("foo", pojo2Map(new Foo()));
        String fooA = Template.parse("{{foo.a}}").render(data);

        assertThat(fooA, is("A"));
    }

Is that what you meant by "keep everything as Lists and Maps in data models"?

msangel commented 5 years ago

In my opinion, the view objects should not contain anything that is not serializable(i.e. cannot be converted by mapper to data tree). And this looks essential for me. Noone tries to put, for example, the Thread object to Spring MVC Model, or mentioned above map with lazy computing.

That MR do not rejecting POJOs, it's making opposite: allow POJOs in data model - reading its properties, iterate it's iterable fields, etc.

The POJOs must be in model, but they must be just data containers, not more, they must be logic-less.