Changing custom tiles base class leads to content loss

[tomgross]

I had the following scenario:

• I implemented a custom tile (myhtml) based on the plone.tile.Tile (similar to the HTML tile in p.a.standardtiles) and used it in production.
• Content was created with this tile.
• After a while I changed the base class to plone.tile.PersistentTIle Now: If editors change another tile in a mosaic page which contains an old tile the content of the old myhtml tile is lost without any warning or log entry.

The temporary solution for me was to open all the content objects in edit mode and click into the myhtml tile to update its value with the new base class and save the object.

Ideal would be catch this scenario on POST and transfer the tile data or at least issue a warning.

plone.app.mosaic version is 2.0.0rc4

[datakurre]

@tomgross Thanks for the issue.

What I suspect is happening:

1. When annotation is still missing, "Persistent tiles" / "tiles with annotation storage" read their defaults from querystring configuration. That's why the refactored tiles seem to work at first.

2. MosaicEditor always reproduces the layout completely, and for each tile, it asks tile URL from the tile code. If tile is inherited from Tile, it includes its configuration in its absolute_url as querystring. When tile is inherited from PersistentTile, it does not include configuration in its absolute_url. That's why the tile reference is saved without data and it seems there is content loss.

Possible solution would be that whenever MosaicEditor detects change in tile's URL, it implicitly saves that tile with the known configuration.

A slightly related is a story about configuration storages in Mosaic. TL; DR; If you had Mosaic 2.0, the "lost" data is probably still in "content" attribute of the object. Also, your HTML might not need persistent tile, but custom Tile base class inherited from Tile, but without possibly problematic querystring serialization (which is currently done in IAbsoluteURL adapter). I know, this is confusing, because we already used to think in "transient tiles" and "persistent tiles". Possibly, we get this terminology fixed somewhere around Mosaic 2.2 or Mosaic 3.0.

Now the long story:

1. At first we inherited the original Deco design of transient tiles, persistent tiles, site layouts and content layouts. Each content object had (in addition to normal fields) simple editable "content" attribute, which could contain any HTML. Of course, that HTML could also contain "transient tiles" with their configuration in querystring. If it did not make sense to serialize tile configuration as query string, a "persistent tile" could be used: it would store configuration in annotation and only have data-tile-reference in "content" attribute. So, everything content specific was mostly in "content" attribute.

2. Then Nathan implemented managed content layouts, which replaced "content" attribute with reference to centrally managed HTML layouts. Now, by default, the "content" attribute was not editable, so all tile customizations were stored into annotations similarly to "persistent tiles". Mosaic could still be used like 1), but by default all tile configuration and content was in object annotations. (Agreed that this was unexpected development just after we had consensus that only transient tiles should be used.)

3. Nathan's use case was a real one, so we needed a compromise. In Mosaic 2.0 there's separate attribute for layout ("customContentLayout") and separate attribute for "transient tile" configuration ("content"). Once again, annotation storage is only used for tiles inherited from PersistentTile. Also all existing peristent tile types are disabled by default (because our community cannot maintain e.g. image scale machinery for both content images and tile annotation images). Yet, effectively all tiles are "persistent", because of separation "customContentLayout" and "content" attribute. But querystring configuration is still used for default values and non-customizable site layout tiles.

[tomgross]

I experience a lot of ConflictErrors with PersistentTiles. I have a tile which implements an accordeon. It is a tile storing HTML and changing the markup via XML transformation on render. If these get big and there are multiple of this kind attached to a content object sometimes these ConflictErrors occur. I suspect the conflict occurs in accessing the PersistentDict which is used as storage.

I'm no longer conviced that falling back to "another" storage is the right thing to do. Probably issue a big warning that PersistentTiles are used at own risk.

Modifying the tile datamanager would legalize them even more.

[datakurre]

A lot of conflicts is unexpected, because e.g. @vangheem has mostly used only annotation storage for tiles and has not reported it as a major issue (even transient tiles use annotation storage with "shared layouts").

Are those read or write conflicts? Is there one or multiple editors editing when those happen? Does the content type have drafting behavior enabled?

I doubt HTML tiles can never be "non-persistent", because the data might be too much to be encoded into querystring and according to @vangheem it would also be XSS attack vector (current default HTML tile ignores querystring for the main data).

What was the problem again in keeping the tile based on Tile instead of PersistentTile?

As I've started to explain recently, Mosaic 2.0 already saves also Tile data into (tile data) content field (in addition it still being serialised into query because of legacy). But that might also conflict. Yet, by building a direct support for that new field based tile data storage into editor, those could be fixed in the long run (because a write would happen only on Save similarly to nowadays optional custom layout).

On 29. tammikuuta 2017 klo 21.39 +0200, Tom Gross notifications@github.com, wrote:

I experience a lot of ConflictErrors with PersistentTiles. I have a tile which implements an accordeon. It is a tile storing HTML and changing the markup via XML transformation on render. If these get big and there are multiple of this kind attached to a content object sometimes these ConflictErrors occur. I suspect the conflict occurs in accessing the PersistentDict which is used as storage.

I'm no longer conviced that falling back to "another" storage is the right thing to do. Probably issue a big warning that PersistentTiles are used at own risk.

Modifying the tile datamanager would legalize them even more.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub (https://github.com/plone/plone.app.mosaic/issues/332#issuecomment-275940080), or mute the thread (https://github.com/notifications/unsubscribe-auth/AAJyv8XjHwU1qKXDHZNtw73660aJmX00ks5rXOsCgaJpZM4Lb3Sz).

[datakurre]

@tomgross Any more details of those ConflictErrors?

I'm afraid that with the Mosaic 2.0 field based tile data storage also normal tiles may cause conflict errors during editing (because drafts for multiple tiles may be saved at the same time and they get saved into same field).

This is not a trivial issue, because server side rendering requires that data is saved somewhere even during editing.

[tomgross]

@datakurre My investigation shows the following

If you edit a html tile (a couple of times) and you don't leave the WYSWIG editor and click the save button the following happens:

• Mosaic tries to save the full page (including the html tile) -> The modification time of the context is updated
• The html tile tries to save itself. -> This triggers notifyModified in plone.app.tiles, which also updates the modification time of the context.

This leads to write conflict and an resulting empty html tile. I wonder why this happens only sometimes?

A possible but surlely not ideal solution would be to provide a custom __presolveConflict method and picking the newer tile. Can this be checked earlier?

[datakurre]

Thanks. I believe that the correct fix right now would be to ensure that the tile has save itself and only that save the form. That'd be the correct order and would avoid the conflict.

I should be able to fix that soon, because I do have the same issue.

On 6. helmikuuta 2017 klo 17.49 +0200, Tom Gross notifications@github.com, wrote:

@datakurre (https://github.com/datakurre) My investigation shows the following

If you edit a html tile (a couple of times) and you don't leave the WYSWIG editor and click the save button the following happens:

Mosaic tries to save the full page (including the html tile) -> The modification time of the context is updated The html tile tries to save itself. -> This triggers notifyModified in plone.app.tiles, which also updates the modification time of the context.

This leads to write conflict and an resulting empty html tile. I wonder why this happens only sometimes?

A possible but surlely not ideal solution would be to provide a custom _p_resolveConflict method and picking the newer tile. Can this be checked earlier?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub (https://github.com/plone/plone.app.mosaic/issues/332#issuecomment-277722632), or mute the thread (https://github.com/notifications/unsubscribe-auth/AAJyv0iJxDTWNawMC8uUd5WGja1yeJ0uks5rZ0DhgaJpZM4Lb3Sz).

[vangheem]

fwiw, for now, I'm sticking with a branch that doesn't store everything in one field. I haven't seen any of the problems you're describing.

[datakurre]

Yes. It makes sense that only the new storage is seeing these issues. Yet, fixing the race condition in saving sounds important anyway.

On 6. helmikuuta 2017 klo 18.25 +0200, Nathan Van Gheem notifications@github.com, wrote:

fwiw, for now, I'm sticking with a branch that doesn't store everything in one field. I haven't seen any of the problems you're describing.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub (https://github.com/plone/plone.app.mosaic/issues/332#issuecomment-277733574), or mute the thread (https://github.com/notifications/unsubscribe-auth/AAJyvzWqGa8DaZhdkbAeUvbjrwgCn2xjks5rZ0lYgaJpZM4Lb3Sz).

[tomgross]

@datakurre Do you have any news on this? We need a fix for this and #346 soon. Anything I can help? I will try to resolve the conflict in the conflicthandler as a workaround, if there is no official solution soon.

[datakurre]

I'll look into these tomorrow. #346 is even more critical, because it breaks rendering. I try to make plone.tiles 2.0.0b1 so that you can try it. About conflicts I know better tomorrow evening. Once those get fixed, I don't miss annotations. (My plan in short is to simply queue all saving posts using jQuery queues to ensure that no conflicting parallel posts can happen.)

[tomgross]

The conflict errors are fixed with https://github.com/plone/plone.app.mosaic/pull/348. Changing base classes is probably not common enough to justify a generic solution. An example on how this could be done is here: https://github.com/plone/plone.tiles/pull/20