Asset Publisher and checking permissions

• Type: Bug
• Status: Closed
• Resolution: Fixed
• Affects Version/s: 6.1.0 CE RC1, 6.1.0 CE GA1
• Fix Version/s: 6.0.X EE, --Sprint 12/11, 6.1.0 CE RC1, 6.1.1 CE GA2, 6.1.2 CE GA3, 6.1.30 EE GA3, 6.2.0 CE M2
• Component/s: Search, Search > Search Portlet, WCM, WCM > Publication, WCM > Publication > Asset Publisher
• Labels:
  • QA-PORTAL
• Branch Version/s:
  6.1.x, 6.0.x
• Backported to Branch:
  Committed
• Similar Issues: Show 3 results ; ;

  LPS-25175	Manual Selection of Asset Publisher ignores permission even enabled permission is checked
  LPS-26062	Asset publisher portlet with enabled permissions shows or not depending on first asset permissions
  LPS-31529	Asset-Publisher - Add link doesn't check portlet permission

When I have about 10 000 AssetEntry and one asset publisher in my page and I want display all of them - Asset Publisher sends 30 000 queries to database in one request.
I've written about it here: http://liferaycms.wordpress.com/2011/10/11/one-of-bottleneck-in-liferay-6-1/ and here: http://liferaycms.wordpress.com/2011/10/17/asset-publisher-and-queries-count/

[Piotr Filipowicz]

Guys, I thought on this topic a lot. Maybe we should modify PermissionChecker implementation. My inspiration was Lucene index. IMHO we can do that by adding new table AssetEntry_ViewPermission with 4 columns:

• id_
• entryId
• groupId
• roleId
  If we want to add groupRoleId we put groupId and roleId values otherwise if we want to add roleId - the value of groupId will be zero.
  Thus we can check View permission using SQL query by join our new table (lucene indexer do that thing).

Advantages:

• we will have only one query to dabase in one Asset
• we won't get 5000 records just in case
• we won't get specific entry (e.g. JournalArticle, Blog and so on) if we want display Asset's abstract view.

Second way is a full PermissionChecker refactoring and link it with AssetEntries.

Third way is to change AssetPublisher implementation and use LuceneIndexer to pick records. We'll try to test this solution and measure the performance.

[Raymond Auge]

I would recommend the last suggestion of simply re-implementing AP to use the index from the beginning!

[Jorge Ferrer]

I agree that using the index could be a solution medium-term.

But short-term I think there should be easier solutions. Also I'm not sure if the index could replace all the functionalities of the db so fixing that is important.

The very basic solution is to make it so that by default it is not guaranteed that the exact number of items configured are retrieved (if permissions apply to hide some of them). IMHO, for a very large percentage of usage of AP, that will be just fine, specially for those that are used in public pages where most content is public. Those are also the cases that will care about performance first.

We could then add a checkbox next to the selection of the number of items to enable "Exact count of items" which has a help icon that explains that this may have performance implications. In any case I would also look for opportunities for improvement in the current algorithm. It may not be optimal in most cases to grab 5000 records ahead of time. A 3-10 times the number selected (or something in that order of magnitude) is probably more reasonable.

Thoughts?

[Szymon Golebiewski]

Jorge,

Such resolution even as a workaround is a bad idea. If we want to show 20 articles I want to see 20 articles unless I know that there are no so many in the CMS system. We can not guarantee users that "not exact count of items" won't return ZERO articles. Everty time they will see other number of articles will be counted as a bug so no will use it. Also I don't know any CMS system that has functionality that will return non-deterministic number of articles. Such poor workaround can only lead to more "Liferay hate" and any explanation in help icon will be treated as an excuse.

[Jorge Ferrer]

Hi Szymon,

I understand what you mean. I still think that we need to find the best possible compromise solution.

In 6.0 and previous asset publisher didn't do any permission check at all, so Ray added that capability for 6.1 which is what has caused the potential scalability problem. We have already made several improvements including making sure that the portlet makes the minimum number of db requests (previously, as pointed out in Piotr's blog it was making several unnecessary requests).

I also think that the default filter limit should be lowered from 5000 to something around a few hundreths (or maybe a multiplier of the number of items configured by that portlet). What do you guys think about this?

Finally, I think that we should provide an option to disable permission checks until we find a long term solution that allows for scalable permission check queries. I thought that it was better to still do a permission check on the frontend even if that implied that the count was inaccurate (or even the list ends up being empty), but an alternative is to avoid it altogether.

Jorge

[Szymon Golebiewski]

Hi Jorge,

Solution based on limiting number of default filter will be always slower than lucene. Also the lower limit we make the higher goes possibility of wrong results. Also disabling permissions checks is not the way our customers will accept.

So having all that in mind we prepared solution based on Lucene. You can read about our results on our blog our blog. We think that's the best option for both short and long term solution.

[Szymon Golebiewski]

Any news about that thing?

[Piotr Filipowicz]

Michael, is your solution rely on enable/disable checkin' permissions? I saw your commit. Did you add enablePermissions flag?

[Michael Han]

For short term:
(1) Provide ability to enable/disable permission checks
(2) Reduce amount of permission checks executed and added more caching

[Piotr Filipowicz]

(2) Reduce amount of permission checks you mean remove 'assetRenderer.hasViewPermission(permissionChecker)' on asset's views (jsps)?

[Michael Han]

@Piotr, no there were some situations where the same query was happening multiple times so we activated more caching to avoid this.

[Juan G]

Seems this issue caused this one: LPS-26062

[Matthew Lee]

Committed on:
Portal 6.1.x CE GIT ID: 7e448632ac9f81d5a4bd1b088016fa1ceb95c990.
Portal 6.2.x GIT ID: 7e448632ac9f81d5a4bd1b088016fa1ceb95c990.