MultiPageMessagesSupport duplicates messages with MyFaces

[GoogleCodeExporter]

Hey all,

the MultiPageMessagesSupport class does not play well with MyFaces. The problem 
is that the phase listener tries to remove the messages from the FacesContext 
after they have been saved. This works well with Mojarra because they return an 
iterator of the underlying list, but MyFaces doesn't. MyFaces implements the 
getMessages() methods in an immutable style. The spec doesn't contain any 
details about immutability. So it is unclear who is right in his way of 
implementation.

If the application is using MyFaces and an action methods returns null all 
messages will be shown twice. That happens because the messages are restored by 
our listener even if they were added (and not deleted) in the same request.

See: 

http://ocpsoft.com/java/persist-and-pass-facesmessages-over-page-redirects/#comm
ent-1288

I think one way to get rid of this problem would be to prevent the message 
restore if there are already messages in the context (see attached patch). I 
think there is no situation where it would make sense to restore messages while 
there are existing ones in the FacesContext queue.

What do you think? I want to make sure this patch doesn't break anything, so 
I'll put it into ReviewRequest state.

Original issue reported on code.google.com by chkalt on 10 Feb 2011 at 4:28

Attachments:

• duplicated-messages-with-myfaces.patch

[GoogleCodeExporter]

What if more messages were added during the second request? Then the original 
messages may be destroyed.

Original comment by lincolnb...@gmail.com on 10 Feb 2011 at 7:07

[GoogleCodeExporter]

Yeah, thats correct. But is this a common situation? I think this is a 
typically scenario:

 * The user submits some form:
   (1) If there are validation errors or the action method creates messages during a custom "in-code" validation, the page will be re-rendered in the same request (action returns null).
   (2) If everything works fine the action will queue a "Success" message and redirect to another PrettyFaces mapping.

I think this is the most common case. At least in my apps! :-)

Unfortunately (1) doesn't work in MyFaces apps so I think we should find a way 
to fix this.

What about checking if there is already "the same" message in the context 
before adding one? It looks like the FacesMessage class doesn't implement 
equals()/hashcode() so we would have to do this manually by comparing all 
relevant properties.

Original comment by chkalt on 11 Feb 2011 at 6:53

[GoogleCodeExporter]

Yeah I think that comparing properties is probably the way to go - I don't 
think *anyone* wants duplicate messages showing up anyway. We'll need to 
compare severity, message, and details I think. Since we don't support 
re-attaching to components, we don't need to worry about that.

Original comment by lincolnb...@gmail.com on 11 Feb 2011 at 3:29

[GoogleCodeExporter]

Sounds good. Let's to it this way. :-)

Original comment by chkalt on 13 Feb 2011 at 2:50

• Changed state: Accepted

[GoogleCodeExporter]

Hey Lincoln,

I just finished to implement correct "duplicated messages" handling in 
FacesMessageUtils. Here is what I did:

1. I created a class called FacesMessageWrapper which wraps a FacesMessage and 
implements hashcode()/equals() correctly.

2. I replaced all "ArrayList<FacesMessage>" with 
"LinkedHashSet<FacesMessageWrapper>" (including the list stored in the session).

3. In restoreMessages() only the messages not already in the FacesContext are 
restored.

4. saveMessages() doesn't remove the FacesMessages from the FacesContext 
anymore. I think we don't need this and it is dangerous to do this, because we 
don't know for sure if the JSF implementation returns an Iterator that allows 
to call remove().

The code is currently on a separate branch:

https://github.com/chkal/prettyfaces/tree/messagefix

What do you think? I think this should finally fix the issues with MyFaces. I 
did some quick tests with MyFaces and Mojarra and everything worked fine. I'll 
try to test the new code on one of our current projects tomorrow. If I find no 
issues I would like to merge it into the master branch so that we can include 
it in 3.2.1.

Original comment by chkalt on 28 Feb 2011 at 4:29

• Changed state: ReviewRequest

[GoogleCodeExporter]

Merged and pushed to the master repository

Original comment by chkalt on 9 Mar 2011 at 7:03

• Changed state: Fixed