Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FlowFacesContext does not return the same FacesMessages instances [SWF-1017] #1703

Closed
spring-operator opened this issue May 28, 2008 · 16 comments

Comments

@spring-operator
Copy link
Contributor

spring-operator commented May 28, 2008

!!Use pwebb rather than philw opened SWF-1017 and commented

I am currently using a phase listener to fix up faces messages before they are displayed (a method described here http://www.oracle.com/technology/pub/articles/masterj2ee/j2ee_wk7.html). Unfortunately FlowFacesContext creates a new FacesMessage adapted from spring messages on each call to FacesContext.getMessages(). This means that any set method called on the message is lost.


Affects: 2.0.5

Attachments:

Issue Links:

1 votes, 2 watchers

@spring-operator
Copy link
Contributor Author

!!Use pwebb rather than philw commented

Attached is a patch that resolves the issue along with a test case.

@spring-operator
Copy link
Contributor Author

!!Use pwebb rather than philw commented

I think that the submitted patch might need a minor tweak, the ImmutableFacesMessage should probably be public and include a serial UID.

@spring-operator
Copy link
Contributor Author

spring-operator commented Jul 2, 2008

Jeremy Grelle commented

Phil,

Some fairly significant revisions to the way we translate and store FacesMessages have been committed for the upcoming 2.0.3 release. (See #1525 and #1526)

First test with these changes and see if they alone solve your issue. If they do not, an updated patch against these changes would be appreciated and considered for inclusion in 2.0.4.

Thanks,
Jeremy

@spring-operator
Copy link
Contributor Author

!!Use pwebb rather than philw commented

Looking at the updated code I think that the problem will remain as getMessages() creates a new Iterator and adapted messages on each call. I will update the patch as soon as I can.

@spring-operator
Copy link
Contributor Author

!!Use pwebb rather than philw commented

Jeremy,

Attached is an updated patch that can be applied to the current trunk. I have moved all FlowFacesContext code relating to messaging into a new delegate class.

The main change that I have made is to created a new FlowFacesMessageAdapter class. This class converts a FacesMessage to a spring Message and replaces the builder pattern that was used previously. Because the Message is now an adapter the original FacesMessage that was added can be retained and returned from the toFacesMessage helper method.

There is also an update to the test case to ensure the fix works.

Could this change be considered for the 2.0.3 release?

Cheers,
Phil.

@spring-operator
Copy link
Contributor Author

Jeremy Grelle commented

Phil,

Thanks! The patch looks fairly clean, but can you perhaps explain in more detail exactly what you need to do to the messages that this patch allows you to do? The Oracle link is not working for me. If you could perhaps even share some of the code from your PhaseListener, that would be helpful. I just want to be sure that there's not already a clean alternative to your current approach.

Thanks,
Jeremy

@spring-operator
Copy link
Contributor Author

!!Use pwebb rather than philw commented

Sure,

I can't share all of the code at the moment but I can describe the intent. The idea is to present nicer error messages to the user should validation fail. For example suppose I have a component and I attach a validateLongRange validator, the error I get is "Validation error: Specified attribute is not between the expected values of X and Y". This is nice, but I want to tell the user what field it is that they need to edit. The technique described in the now defunct link involves attaching a f:attribute to the component with the name of the field. The phase listener looks for all error messages, finds the component and if there is an attribute changes the error message to include the attribute value:

public void beforePhase(PhaseEvent e) {
FacesContext fc = e.getFacesContext();
UIViewRoot root = fc.getViewRoot();
Iterator i = fc.getClientIdsWithMessages();
while (i.hasNext()) {
String clientId = (String) i.next();
UIComponent c = root.findComponent(clientId);
String fieldRef =
(String) c.getAttributes().get("fieldRef");
if (fieldRef != null) {
Iterator j = fc.getMessages(clientId);
while (j.hasNext()) {
FacesMessage fm = (FacesMessage) j.next();
fm.setDetail(fieldRef + ": " + fm.getDetail());
}
}
}
}

The problem with the existing implementation is that the the getMessages call will return a new set of FacesMessage items each time. The phase listener will run, update all the messages and think that everything is fine. Then the facelets render will run but the call that it makes to the iterator returns a new, freshly adapted set of messages. These will not includes the updates from the phase listener.

You can try this google cache link for the original article if it works,

http://216.239.59.104/search?q=cache:mYHZd8hDfE8J:www.oracle.com/technology/pub/articles/masterj2ee/j2ee_wk7.html+j2ee_wk7.html+oracle

I also use the same technique to deal with some internationalization issue I had where the user could switch language after error messages have been created and end up with a mix of different languages for the screen and errors.

Happy to have another attempt at the patch if you can think of another approach? I am not entirely happy with the overriding of the spring org.springframework.binding.message.Message class getters. If org.springframework.binding.message.Message was an interface the code would be a little bit cleaner but I thought that that would be too much code to change.

Regards,
Phil.

@spring-operator
Copy link
Contributor Author

!!Use pwebb rather than philw commented

Updated patch to fix a bug where getSeverity returned null.

@spring-operator
Copy link
Contributor Author

Jeremy Grelle commented

Phil,

Are you using JSF 1.2? I just ask because in JSF 1.2 you can improve the messages in this way without the PhaseListener. For example, the JSF 1.2 standard message for the long range validation is:

{2}: Validation Error: Specified attribute is not between the expected values of {0} and {1}.

You can specify a label attribute on the component and that label will get inserted for {2}.

For example:
<h:inputText label="A Long Input" id="foo" value="#{bar}"/>

  • Jeremy

@spring-operator
Copy link
Contributor Author

!!Use pwebb rather than philw commented

Cool, I did not know that. Unfortunately I am currently on myfaces 1.1 but I will be looking to upgrade at some point. Unfortunately my PhaseListener does a few more bits and pieces as well, such as support a custom message when a value is required.

Do you think that the patch in its current form is unsuitable? I am happy to have another go at it if you can suggest an alternative way?

@spring-operator
Copy link
Contributor Author

Jeremy Grelle commented

Your patch looks good and I am definitely leaning towards applying it for 2.0.3. I'm just making sure we've covered all possible angles.

@spring-operator
Copy link
Contributor Author

!!Use pwebb rather than philw commented

OK Cool, 2.0.3 would be good for me :-)

@spring-operator
Copy link
Contributor Author

spring-operator commented Oct 27, 2008

!!Use pwebb rather than philw commented

I just hit an issue with this patch that occurs when logging is enabled, it sort of relates to #1507

The FlowFacesMessageAdapter.toString() method calls the getText() method and this in turn calls facesMessage.getDetail(). MyFaces uses FacesContext at this point;

@Override
public String getDetail() {
	FacesContext facesContext = FacesContext.getCurrentInstance();
	ValueExpression value = facesContext.getApplication().getExpressionFactory().
		createValueExpression(facesContext.getELContext(), super.getDetail(), String.class);
	return (String) value.getValue(facesContext.getELContext());
}

In webflow FacesContext.getInstance() does not always exist, if it returns null:

java.lang.NullPointerException
at javax.faces.component._LabeledFacesMessage.getDetail(_LabeledFacesMessage.java:58)
at org.springframework.faces.webflow.FlowFacesContextMessageDelegate$FlowFacesMessageAdapter.getText(FlowFacesContextMessageDelegate.java:288)
at org.springframework.faces.webflow.FlowFacesContextMessageDelegate$FlowFacesMessageAdapter.toString(FlowFacesContextMessageDelegate.java:310)
at java.lang.String.valueOf(Ljava.lang.Object;)Ljava.lang.String;(Unknown Source)
at org.springframework.core.style.DefaultValueStyler.style(DefaultValueStyler.java:78)
at org.springframework.core.style.DefaultValueStyler.style(DefaultValueStyler.java:107)
at org.springframework.core.style.DefaultValueStyler.style(DefaultValueStyler.java:72)
at org.springframework.core.style.DefaultValueStyler.style(DefaultValueStyler.java:100)
at org.springframework.core.style.DefaultValueStyler.style(DefaultValueStyler.java:87)
at org.springframework.core.style.DefaultValueStyler.style(DefaultValueStyler.java:66)
at org.springframework.core.style.DefaultValueStyler.style(DefaultValueStyler.java:100)
at org.springframework.core.style.DefaultValueStyler.style(DefaultValueStyler.java:87)
at org.springframework.core.style.DefaultValueStyler.style(DefaultValueStyler.java:66)
at org.springframework.core.style.StylerUtils.style(StylerUtils.java:47)
at org.springframework.webflow.core.collection.LocalAttributeMap.toString(LocalAttributeMap.java:336)
at java.lang.String.valueOf(Ljava.lang.Object;)Ljava.lang.String;(Unknown Source)
at org.springframework.core.style.DefaultValueStyler.style(DefaultValueStyler.java:78)
at org.springframework.core.style.DefaultValueStyler.style(DefaultValueStyler.java:100)
at org.springframework.core.style.DefaultValueStyler.style(DefaultValueStyler.java:87)
at org.springframework.core.style.DefaultValueStyler.style(DefaultValueStyler.java:66)
at org.springframework.core.style.StylerUtils.style(StylerUtils.java:47)
at org.springframework.webflow.core.collection.LocalAttributeMap.toString(LocalAttributeMap.java:336)
at java.lang.String.valueOf(Ljava.lang.Object;)Ljava.lang.String;(Unknown Source)
at java.lang.StringBuilder.append(StringBuilder.java:116)
at org.springframework.webflow.conversation.impl.PublicContainedConversation.putAttribute(PublicContainedConversation.java:97)
at org.springframework.webflow.execution.repository.snapshot.AbstractSnapshottingFlowExecutionRepository.putConversationScope(AbstractSnapshottingFlowExecutionRepository.java:98)
at org.springframework.webflow.execution.repository.impl.DefaultFlowExecutionRepository.putFlowExecution(DefaultFlowExecutionRepository.java:120)
at org.springframework.webflow.executor.FlowExecutorImpl.resumeExecution(FlowExecutorImpl.java:155)
at org.springframework.webflow.mvc.servlet.FlowHandlerAdapter.handle(FlowHandlerAdapter.java:173)
at org.springframework.webflow.mvc.servlet.FlowController.handleRequest(FlowController.java:172)
at org.springframework.web.servlet.mvc.SimpleControllerHandlerAdapter.handle(SimpleControllerHandlerAdapter.java:48)
at org.springframework.web.servlet.DispatcherServlet.doDispatch(DispatcherServlet.java:875)

No too sure on what the fix could be. The get methods could check that FacesContext.getInstance() does not return null before returning a result or toString() could be changed to not call the getText() method.

I have a feeling that the better solution would be to create and release FacesContext much earlier. MyFaces creates it in the FacesServlet.service method.

@spring-operator
Copy link
Contributor Author

spring-operator commented Dec 8, 2008

!!Use pwebb rather than philw commented

Updated patch to deal with toString() NPE issue and #1684

@spring-operator
Copy link
Contributor Author

Mark Diskin commented

Hi Phil,

Do you have an example for a facelet use? I patched the code into my app and still not getting the field message out, I was not too sure for this to work if I had to follow a set convention on Model name to form name to help with the mapping. A Simple snipet of the page, flow and validator would be very helpful.

Thanks
Mark

@spring-operator
Copy link
Contributor Author

Jeremy Grelle commented

Patch has been applied, with just a couple of tweaks. Thanks Phil!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant