SelectOneMenuRenderer doesn't work with a Converter

[exabrial]

Hey guys! First I just want to say thanks for Bootsfaces. Open Source work frequently goes unthanked, and I'd just like to extend my gratitude :)

So I think we found an issue with SelectOneMenuRenderer when using a converter. Essentially what I think we found is getAsObject() isn't called.

<o:importConstants
    type="com.xxx.enums.UsState"
    var="UsState" />
<b:selectOneMenu
    id="homeStateSelectOneMenu"
    value="#{contactPage.contact.homeState}"
    label="Home State"
    converter="DisplayableTextConverter">
    <f:selectItem
        id="selectOneUsStateSelectItem"
        itemLabel="Select One"
        noSelectionOption="true"
        itemDisabled="true" />
    <f:selectItems
        id="usStateSelectItems"
        value="#{UsState}" />
</b:selectOneMenu>

@Enumerated(EnumType.STRING)
@Column(name = "home_state")
@NotNull //LYNCHPIN
private UsState homeState;

@ApplicationScoped
@FacesConverter(value = "DisplayableTextConverter", managed = true)
public class DisplayableTextConverter implements Converter<DisplayableText> {
	public static final String STATE_KEY = "DisplayableTextConverter.forClass";
	@Inject
	private Logger log;

	@SuppressWarnings("unchecked")
	@Override
	public DisplayableText getAsObject(final FacesContext context, final UIComponent component, String text) {
		text = trimToNull(text);
		if (text != null) {
			try {
				final String className = (String) component.getAttributes().get(STATE_KEY);
				return DisplayableText.parseEnum(text, (Class<DisplayableText>) Class.forName(className));
			} catch (final Exception e) {
				log.error("getAsObject() STATE_KEY:{} text:{}", component.getAttributes().get(STATE_KEY), text, e);
				throw new RuntimeException(e);
			}
		} else {
			return null;
		}
	}

	@Override
	public String getAsString(final FacesContext context, final UIComponent component, final DisplayableText displayableText) {
		if (displayableText != null) {
			component.getAttributes().put(STATE_KEY, displayableText.getClass().getName());
			return displayableText.getDisplayText();
		} else {
			return null;
		}
	}
}

public enum UsState implements DisplayableText {
    AL("Alabama"), AK("Alaska"), AZ("Arizona"), AR("Arkansas"), ....

    public final String displayText;

    @Override
    public String getDisplayText() {
        return displayText;
    }

    UsState(String specificDisplayValue) {
        displayText = specificDisplayValue;
    }
}

Here's the stack trace, which is a bit misleading:

java.lang.IllegalArgumentException: Arkansas is not an instance of class com.xxx.UsState
    at org.apache.bval.jsr.job.ValidateProperty.<init>(ValidateProperty.java:515)
    at org.apache.bval.jsr.job.ValidationJobFactory.validateValue(ValidationJobFactory.java:76)
    at org.apache.bval.jsr.ValidatorImpl.validateValue(ValidatorImpl.java:65)
    at org.apache.bval.jsr.CascadingPropertyValidator.validateValue(CascadingPropertyValidator.java:99)
    at javax.faces.validator.BeanValidator.validate(BeanValidator.java:218)
    at javax.faces.component._ComponentUtils.callValidators(_ComponentUtils.java:291)
    at javax.faces.component.UIInput.validateValue(UIInput.java:489)
    at net.bootsfaces.component.selectOneMenu.SelectOneMenu.validateValue(SelectOneMenu.java:118)
    at net.bootsfaces.component.selectOneMenu.SelectOneMenuRenderer.decode(SelectOneMenuRenderer.java:96)
    at javax.faces.component.UIComponentBase.decode(UIComponentBase.java:479)
    at javax.faces.component.UIInput.decode(UIInput.java:371)
    at javax.faces.component.UIComponentBase.processDecodes(UIComponentBase.java:1408)
    at javax.faces.component.UIInput.processDecodes(UIInput.java:207)
    at javax.faces.component.UIComponentBase.processDecodes(UIComponentBase.java:1402)
    at javax.faces.component.UIForm.processDecodes(UIForm.java:154)
    at org.apache.myfaces.context.servlet.PartialViewContextImpl$PhaseAwareVisitCallback.visit(PartialViewContextImpl.java:775)

So what's happening in SelectOneMenuRenderer is it looks like it has a default way of converting menu items into objects (sort of like a default converter). This handy feature works by iterating through the SelectItems and trying to match one up with the submittedValue. See here: https://github.com/TheCoder4eu/BootsFaces-OSP/blob/master/src/main/java/net/bootsfaces/component/selectOneMenu/SelectOneMenuRenderer.java#L68

We have a temporary patch:

	public void decode(final FacesContext context, final UIComponent component) {
		final SelectOneMenu menu = (SelectOneMenu) component;
		if (menu.isDisabled() || menu.isReadonly()) {
			return;
		}
		final String outerClientId = menu.getClientId(context);
		final String clientId = outerClientId + "Inner";
		final String submittedOptionValue = context.getExternalContext().getRequestParameterMap().get(clientId);

		Converter<?> converter = menu.getConverter();
		if (null == converter) {
			converter = findImplicitConverter(context, component);
		}

		Object convertedValue;
		if (converter != null) {
			convertedValue = converter.getAsObject(context, component, submittedOptionValue);
		} else {
			convertedValue = submittedOptionValue;
		}

		menu.setValid(true);
		menu.validateValue(context, convertedValue);
		menu.setSubmittedValue(submittedOptionValue);
		new AJAXRenderer().decode(context, component, clientId);
	}

Ideally the code would probably use the converter if possible, and in leui of that, execute the for loop and try to match a value.

We're totally willing and able to write a patch if we can get some direction on what you want and your expectations. Please let us know how to proceed :)

THANK YOU!

[stephanrauh]

Hey, cool, yesterday I've read your question on StackOverflow and wondered how to answer it, and today you've already got a solution. Color me impressed!

[stephanrauh]

First impression: I'm pretty sure your solution works. I'm afraid I need to restore my BootsFaces development environment, so I couldn't run a test today. Please nudge me again if I haven't given you a definite answer next Monday.

Do you happen to know if the for loop is really necessary? At the time, I've implemented it because I wasn't aware converters are such a popular thing in the JSF world. I wonder if the implicit converter is an implementation of my for loop.

Best regards,
Stephan

[stephanrauh]

Oh, and before I forget: thanks for all the kind words! That's what keeps an open source developer going. That and the 3000 downloads each month. It's a pity there's so little activity in this project recently. Would you like to help us? I can't afford to devote as much time as I used to do to BootsFaces, but I'm sure I still can guide and coordinate developers who'd like to fill the gap. Anybody interested?

[exabrial]

Do you happen to know if the for loop is really necessary?

Generally that's what converters are for. It's handy to have a fallback with the loop, but generally a converter is the recommended way in JSF. They also have a lot of uses from displaying version text of objects or for loading stuff on page render.

I still can guide and coordinate developers who'd like to fill the gap. Anybody interested?

:) We're interested. We use quite a bit of BootsFaces, and we'll submit patches for bugs that affect us. The big thing for us is while we're users of JSF frameworks, we don't know much about their implementation. That's why I'm not sure our patch is completely correct... we'd love to know if all of the conditions are satisfied.

[stephanrauh]

We've got a deal! Only it's not a deal because this is open source country... and everything anybody's doing here is voluntary, a gift to the community. So let me try again: welcome to the team!

we're users of JSF frameworks, we don't know much about their implementation

Don't worry. There are only half a dozen sophisticated algorithms in BootsFaces. The vast majority of the code is simple and straight-forward. Mind you: you've already managed to dig into one of the hard parts.

we'd love to know if all of the conditions are satisfied.

If it works, it's good. :) We never managed to write automated tests (lots of attempts, all of them failures), so we compensate this with the showcase. Clone the showcase (https://github.com/TheCoder4eu/BootsFacesWeb), add a demo to https://github.com/TheCoder4eu/BootsFacesWeb/blob/master/src/main/webapp/forms/selectOneMenu.xhtml, and check if both the new feature and the old demos work. If they do, send us a pull request to the showcase and the library and nudge me until I've merged it. That's all!

[exabrial]

Alright, awesome! We'll do some testing and turn a patch around by the weekend.

Can you comment on #1164 and tell us what you think the root issue is and how to go about investigating/fixing? We'll do the leg work from there, just need a bit of direction!

[stephanrauh]

Done! Don't hesitate to tell me if I didn't manage to answer all your question. You've dug really deep into the source code, so I know you'll manage to solve the issue.

BTW, now you've found the most annoying piece of code (the AddResourcesListener) and the second most convoluted class (the DataTableRenderer). :)

[exabrial]

@stephanrauh Patch submitted. Two things I'm unsure of:

1. The original code calls setLocalValue at one point. I have no idea what that means/does. I took it out.
2. on line 74, I call menu.setSubmittedValue(convertedValue); rather than menu.setSubmittedValue(submittedOptionValue);. I believe this is supposed to be the correct course of action, and it does indeed work, but again, not an expert on JSF internals.

[stephanrauh]

Thanks for submitting the PR and for pointing the two changes out. I'll try to remember why I called setLocalValue(). It was one of the first components I ever wrote, so it's possible I made a lot of mistakes. But maybe there was a reason.

[stephanrauh]

I've merged your PR and uploaded it to Maven Central as snapshot 1.6.0. See issue #369 on how to get it.

Thanks a lot!
Stephan