Flow: TemplateRenderer.withProperty overwrites previous property definition

[stefanuebe]

I have a grid with several columns, each column is generated by a TemplateRenderer. The grid instance itself is initialized from a polymer template.

Two of these columns are referring to properties of the same type (Person). To keep the details (like the person's role) out of the template, the properties are both put with the name person into the template. The result of this is, that the second declaration overwrites the first one.

For me this behavior is intransparent, since I define the property at the renderer instance and not the grid and would therefore expect some local scope.

So request is, that properties defined for a TemplateRenderer should be available only in this instance and do not overwrite other renderer instances.
If this is not possible due to technical reasons, then the property definition should be moved away from the TemplateRenderer to the Grid (or some other instance within the grid wide scope).

Grid column definition

@Id("grid")
private Grid<Project> projectGrid;

@PostConstrcut
private void init() {
    // ... 
    projectGrid.addColumn(TemplateRenderer.<Project>of("<div>[[item.person.name]]</div>").withProperty("person", Project::getDeliveryLead));
    projectGrid.addColumn(TemplateRenderer.<Project>of("<div>[[item.person.name]]</div>").withProperty("person", Project::getAccountManager)); // this overrides the previous set "deliveryLead" 
// ...
}

Beans

public class Project {
    private Person deliveryLead;
    private Person accountManager;
    // ...
}

public class Person {
     private String name;
     // ...
}

[gilberto-torrezan]

The name of the property is just an alias, and it is global for the Grid instance. Each row in the Grid represent an object, and the properties of the object are defined with the withProperty method. When you define the same property twice, only the latest definition is used.

One thing that you can do is to use a different property name for each column:

projectGrid.addColumn(TemplateRenderer.<Project>of("<div>[[item.deliveryLead.name]]</div>").withProperty("deliveryLead", Project::getDeliveryLead));
projectGrid.addColumn(TemplateRenderer.<Project>of("<div>[[item.accountManager.name]]</div>").withProperty("accountManager", Project::getAccountManager));

Another option is to send just the data that you need as a property (this reduces the amount of data that the server sends to the client):

projectGrid.addColumn(TemplateRenderer.<Project>of("<div>[[item.deliveryLeadName]]</div>").withProperty("deliveryLeadName", person -> person.getDeliveryLead().getName()));
projectGrid.addColumn(TemplateRenderer.<Project>of("<div>[[item.accountManagerName]]</div>").withProperty("accountManagerName", person -> person.getAccountManager().getName());

[stefanuebe]

That's the problem. It is not transparent or predictable behavior that a property defined on one renderer instance influence another. I guess best solution would be to assign these properties to the Grid directly if having a local scope for properties is not possible. Then it would be more clear, that those properties are defined for the Grid's scope.

[pleku]

Unless this would be a really easy fix, I'm a bit against fixing it by isolating the scopes since there is not that much value that this adds. I however suspect that it is not that simple to make, or is it @gilberto-torrezan ?

Couple things we could do is highlight the shared scope in the javadocs for withProperty, and maybe log a warning, but don't think the latter is really necessary and you might want to do this in some cases.

[gilberto-torrezan]

Leaving the implementation of the properties to the component instead of to the Renderer would impact all components that support Renderers (not just Grid). Also it would become cumbersome to declare the column template in one place, and the properties in another place.

One thing that could be done instead is to remove the need to come up with unique names for the properties. For example, we could support this type of syntax, similar to what we have for Page::executeJavaScript:

projectGrid.addColumn(TemplateRenderer.<Project>of("<div>$0</div>").withProperty(person -> person.getDeliveryLead().getName()));

Then the renderer would be responsible to create the property names under the hood, guaranteeing uniqueness. The same thing goes for events.

[stefanuebe]

That sounds like a good solution to me. Would that also allow property references like this one?

projectGrid.addColumn(TemplateRenderer.<Project>of("<a href$="$0.email">$0.name</a>").withProperty(person -> person.getDeliveryLead()));

[eriklumme]

Just ran into the same issue, and only figured out the issue when reading this.

For me it was nondeterministic, sometimes both columns had the value of the first one, sometimes of the second one. As it was a class in this case, it was a bit harder to see the correlation.

[jcgueriaud1]

Got the same problem.
If you have 2 TemplateRenderer with the same property (like value), sometimes it will show the first one, sometimes the second one.

It's pretty hard to debug it in a project. (especially in Java it fetches the correct values)

To avoid this, I used a uuid string, so my property is unique.

Should this be documented somewhere?

[mvysny]

I also ran into this, the result is highly confusing. This is a bug, not an enhancement, since the javadoc at Renderer.java:78 says that

Sets a property to be used inside the template.

which suggests that the property is scoped to the Renderer.

Couple things we could do is highlight the shared scope in the javadocs for withProperty, and maybe log a warning, but don't think the latter is really necessary and you might want to do this in some cases.

@pleku if the scope is decided to be kept global, then the current behaviour is surprising, and in such case a warning is a bare minimum I'd expect.

In fact I'd expect Grid to throw an exception with a clear message that the property name needs to be unique within the scope of the Grid. Even though this is backward-incompatible, this is okay since 1) only a few users will run into this, 2) the price of not fixing this is that we will keep the confusing behaviour which is perceived 100% as buggy, at least by everybody in this thread.

[masbaehr]

Also came across this today.. causing a major issue. This needs to be fixed or at least it needs a decent warning in the javadoc

[mvysny]

There. Fixed the tags for you.

[mvysny]

A workaround, prefixing every property with an unique prefix:

public final class TemplateRenderer2<SOURCE> extends Renderer<SOURCE> {

    private static final Pattern BINDING_MISSING_DOLLAR = Pattern
            .compile("\\s(class|style)\\s*=\\s*['\"]?[{\\[]{2}");

    private static final Pattern PROPERTY_NAME =
            Pattern.compile("\\[\\[item\\.(.+)\\]\\]");

    private static final AtomicInteger ID = new AtomicInteger();
    private final Map<String, String> uniquePropertyNames;

    /**
     * Creates a new TemplateRenderer based on the provided template. The
     * template accepts anything that is allowed inside a {@code <template>}
     * element, and works with Polymer data binding syntax.
     * <p>
     * Examples:
     *
     * <pre>
     * {@code
     * // Prints the index of the item inside a repeating list
     * TemplateRenderer2.of("[[index]]");
     *
     * // Prints the property of an item
     * TemplateRenderer2.of("<div>Property: [[item.property]]</div>");
     * }
     * </pre>
     *
     * @param <SOURCE>
     *            the type of the input object used inside the template
     * @param template
     *            the template used to render items, not <code>null</code>
     * @return an initialized TemplateRenderer
     * @see TemplateRenderer2#withProperty(String, ValueProvider)
     */
    public static <SOURCE> TemplateRenderer2<SOURCE> of(String template) {
        Objects.requireNonNull(template);
        warnIfClassOrStyleWithoutDollar(template);

        Map<String, String> uniquePropertyNames = new HashMap<>();
        Matcher m = PROPERTY_NAME.matcher(template);
        final String uniquePropertyPrefix = "p" + ID.incrementAndGet();
        while (m.find()) {
            final String propertyName = m.group(1);
            final String uniquePropertyName = uniquePropertyPrefix + propertyName;
            uniquePropertyNames.put(propertyName, uniquePropertyName);
            template = template.replace("[[item." + propertyName + "]]", "[[item." + uniquePropertyName + "]]");
        }

        TemplateRenderer2<SOURCE> renderer = new TemplateRenderer2<>(template, uniquePropertyNames);
        return renderer;
    }

    private static void warnIfClassOrStyleWithoutDollar(String template) {
        if (hasClassOrStyleWithoutDollar(template)) {
            LoggerFactory.getLogger(TemplateRenderer2.class)
                    .warn("Bindings for 'class' and 'style' need a $ prefix (e.g. 'class$=\"[[item.className]]\"') to use an attribute binding instead of a property binding."
                            + " Apparent omission detected in the template '{}'",
                            template);
        }
    }

    // Not private for testing purposes
    static boolean hasClassOrStyleWithoutDollar(String template) {
        return BINDING_MISSING_DOLLAR.matcher(template).find();
    }

    private TemplateRenderer2(String template, Map<String, String> uniquePropertyNames) {
        super(template);
        this.uniquePropertyNames = uniquePropertyNames;
    }

    /**
     * Sets a property to be used inside the template. Each property is
     * referenced inside the template by using the {@code [[item.property]]}
     * syntax.
     * <p>
     * Examples:
     *
     * <pre>
     * {@code
     * // Regular property
     * TemplateRenderer.<Person> of("<div>Name: [[item.name]]</div>")
     *          .withProperty("name", Person::getName);
     *
     * // Property that uses a bean. Note that in this case the entire "Address" object will be sent to the template.
     * // Note that even properties of the bean which are not used in the template are sent to the client, so use
     * // this feature with caution.
     * TemplateRenderer.<Person> of("<span>Street: [[item.address.street]]</span>")
     *          .withProperty("address", Person::getAddress);
     *
     * // In this case only the street field inside the Address object is sent
     * TemplateRenderer.<Person> of("<span>Street: [[item.street]]</span>")
     *          .withProperty("street", person -> person.getAddress().getStreet());
     * }
     * </pre>
     *
     * Any types supported by the {@link JsonSerializer} are valid types for the
     * TemplateRenderer.
     *
     * @param property
     *            the name of the property used inside the template, not
     *            <code>null</code>
     * @param provider
     *            a {@link ValueProvider} that provides the actual value for the
     *            property, not <code>null</code>
     * @return this instance for method chaining
     */
    public TemplateRenderer2<SOURCE> withProperty(String property,
                                                  ValueProvider<SOURCE, ?> provider) {
        // this will however cause getValueProviders() to return the unique property names instead of "property"...
        setProperty(uniquePropertyNames.get(property), provider);
        return this;
    }

    /**
     * Sets an event handler for events from elements inside the template. Each
     * event is referenced inside the template by using the {@code on-event}
     * syntax.
     * <p>
     * Examples:
     *
     * <pre>
     * {@code
     * // Standard event
     * TemplateRenderer.of("<button on-click='handleClick'>Click me</button>")
     *          .withEventHandler("handleClick", object -> doSomething());
     *
     * // You can handle custom events from webcomponents as well, using the same syntax
     * TemplateRenderer.of("<my-webcomponent on-customevent=
    'onCustomEvent'></my-webcomponent>")
     *          .withEventHandler("onCustomEvent", object -> doSomething());
     * }
     * </pre>
     *
     * The name of the function used on the {@code on-event} attribute should be
     * the name used at the handlerName parameter. This name must be a valid
     * Javascript function name.
     *
     * @param handlerName
     *            the name of the handler used inside the
     *            {@code on-event="handlerName"}, not <code>null</code>
     * @param handler
     *            the handler executed when the event is triggered, not
     *            <code>null</code>
     * @return this instance for method chaining
     * @see <a href=
     *      "https://www.polymer-project.org/2.0/docs/devguide/events">https://www.polymer-project.org/2.0/docs/devguide/events</a>
     */
    public TemplateRenderer2<SOURCE> withEventHandler(String handlerName,
                                                      SerializableConsumer<SOURCE> handler) {
        // todo mavi possibly make handlerName unique too?
        setEventHandler(handlerName, handler);
        return this;
    }
}

The disadvantage is that the getValueProviders() map will contain the mangled property names. Also the properties and template is really present in the Renderer class itself, and so the fix should go there.

I like Gilberto's solution of using $0. Alternatively, we could keep [[item.foo]] for backward compatiblity, but the Grid will scope item to individual columns, or would replace item by item_col0 or item.col0 or similar. Renderer:260 will then only receive a scoped JSON.

Alternatively we could fail with an very informative exception at Renderer:260 if the property is already there. The exception must clearly explain that all renderers populate properties of a row item shared for all columns and all renderers, and the best way to avoid misunderstanding/conflicts is for the programmer to make sure the property names are unique with respect to entire row item. This is the root of the misunderstanding.

[pleku]

I'm not sure what should be done with this - minimum is probably that javadocs should be further improved or errors be thrown instead of the current weird behavior.

In the future the TemplateRenderer is going to be replaced by something that is not based on Polymer and thus I feel we should spend time on other areas than the current TemplateRenderer. Moving this to flow repository as the source of the problem is there.

[KasparScherrer]

Came across this issue as well. The most confusing thing to me is the lack of information about the TemplateRenderer in general. Please document this! even if its going to be replaced in the future. I use it now.

a workaround is to add a unique part to the property name as jcgueriaud1 suggested here and Leif suggested to me right now in this forum post

PS: for .withEventHandler this uuid workaround is not needed. The grid can handle multiple eventhandlers with the same name. In fact it gave me errors when I applied this unique naming workaround there.

[heruan]

@pleku I understand that "in the future the TemplateRenderer is going to be replaced" but I hope this future is very close since AFAIK TemplateRenderer is the only alternative to ComponentRenderer which does not slow down the grid (see https://vaadin.com/blog/using-the-right-r).