Avoid duplicate info and warning messages

[Ryan J Ollos <ryan.j.ollos@…>]

To reproduce the issue I've seen:

• Create an instance of Trac without the SubversionConnector component enabled.
• Observe that the Warning: Can't synchronize with repository … message is seen.
• Edit a wiki page and save the change.
• Observe that the Warning: Can't synchronize with repository … message is seen twice.

Here are screen captures before and after an edit:

[Ryan J Ollos <ryan.j.ollos@…>]

Patch against r11483 of the trunk.

[Ryan J Ollos <ryan.j.ollos@…>]

t10987-r11483-1.patch​ attempts to fix the issue by preventing duplicate entries in the req.chrome['warnings'] and req.chrome['notices'] lists.

[Ryan J Ollos <ryan.j.ollos@…>]

I went about trying to fix this under the assumption that we could end up with duplicate info or warning messages displayed under circumstances other than the repository not correctly configured. However, if that is not the case, then it is probably not too important to fix the issue.

[Remy Blank]

Thanks for the patch.

[Remy Blank]

Patch applied in [11494].

[Ryan J Ollos <ryan.j.ollos@…>]

Patch against r11508 of the trunk.

[Ryan J Ollos <ryan.j.ollos@…>]

I thought I had tested this patch pretty thoroughly, but I'm still seeing the issue described in comment:description. This seems to be due to the saving/restoring of messages saved in session data to handle the case of redirect. I've created t10987-r11508-1.patch​ to deal with the issue. Sorry for the error.

[Christian Boos]

Wouldn't there be a way to make sure no duplicate messages get stored in the session in the first place?

[Ryan J Ollos <ryan.j.ollos@…>]

I'm seeing only one message stored in the session. There is a message popped from req.session, and another already existing in req.chrome at line 966 of trac.web.chrome:

                    for i in itertools.count():
                        message = req.session.pop('chrome.%s.%d' % (type_, i))
                        req.chrome[type_].append(Markup(message))

It looks like we need to make sure there aren't duplicates added when the messages in req.session are appended to req.chrome, which is what the patch tries to accomplish.

That said, I'm not claiming to have a big picture view of what's going on in this function, so I wouldn't be surprised if there is a better way to fix this.

[Christian Boos]

Ah, I see, thanks for the clarification.

[Remy Blank]

Dedupe messages when loading from the session after a redirect.

[Remy Blank]

Using unescape() in t10987-r11508-1.patch​ isn't quite correct, as req.chrome[type_] can contain strings or Markup instances, and the comparison will fail for the latter. We need to compare the messages read from the session to the escaped messages. 10987-dedupe-messages-r11531.patch​ does this.

But even this isn't completely correct. If a message is read from the session after a redirect, it's stored in req.chrome[type_] as a Markup instance. After that, if the same message is added again as a string, it will be duplicated.

How about escaping the messages in add_warning() and add_notice()? That way, req.chrome[type_] will only contain Markup instances, and the comparison should always work.

Opinions?

[Remy Blank]

Store messages as Markup.

[Remy Blank]

10987-dedupe-markup-r11531.patch​ implements the suggestion from comment:12.

Opinions?

[Christian Boos]

Replying to rblank:

10987-dedupe-markup-r11531.patch​ implements the suggestion from comment:12.

We should add a comment in add_warning saying that we normalize on Markup instances we have to be wary about the fact that '<a>' == Markup('<a>') (which is not completely obvious).

Then about the session, doing the escape(sessiondata) on save and Markup(sessiondata) on read seems to be a good thing to do. However, shouldn't we rather do that in general, for any session_attribute, and therefore in Session.get_session/set? And if we store the session attributes escaped, we should probably use full escaping (keep the default quote=True).

But if you want to do it the incremental way, we could leave the above suggestion for 1.1.x and fix the present issue like you did in the patch.

[Remy Blank]

10987-dedupe-markup-r11531.patch​ applied in [11532], with improved docstrings.

Replying to cboos:

Then about the session, doing the escape(sessiondata) on save and Markup(sessiondata) on read seems to be a good thing to do. However, shouldn't we rather do that in general, for any session_attribute, and therefore in Session.get_session/set? And if we store the session attributes escaped, we should probably use full escaping (keep the default quote=True).

I wouldn't do this for all the data stored in the session. Session attributes can be arbitrary strings (and the DB supports this), so there's no reason to encode or escape them. Why escape e.g. the last ticket query? It wouldn't even work, as escape() actually modifies the string value, so you would have to call unescape() when reading the value, not Markup().

Also, these messages are the only occurrence I know of of text stored in the session which is used afterward for rendering.

[Christian Boos]

Replying to rblank:

… Why escape e.g. the last ticket query? It wouldn't even work, …

Oh, you're right.