LPS-57634

[hhuijser]

No description provided.

[liferay-continuous-integration]

Some tests FAILED!

Build Time: 1 hour 7 minutes 13 seconds

Base Branch:

Branch Name: master
Branch GIT ID: 7f8a841915d96a4789f51c600bca83ba573b5551

Job Summary:

• test-portal-acceptance-pullrequest(master)
• test-portal-acceptance-pullrequest-batch(master)/compile-jsp
• test-portal-acceptance-pullrequest-batch(master)/functional-tomcat-hypersonic/0
• test-portal-acceptance-pullrequest-batch(master)/functional-tomcat-mysql/0
• test-portal-acceptance-pullrequest-batch(master)/functional-tomcat-mysql/1
• test-portal-acceptance-pullrequest-batch(master)/functional-tomcat-mysql/2
• test-portal-acceptance-pullrequest-batch(master)/functional-tomcat-mysql/3
• test-portal-acceptance-pullrequest-batch(master)/functional-tomcat-mysql/4
• test-portal-acceptance-pullrequest-batch(master)/functional-tomcat-mysql/5
• test-portal-acceptance-pullrequest-batch(master)/functional-tomcat-mysql/6
• test-portal-acceptance-pullrequest-batch(master)/functional-tomcat-mysql/7
• test-portal-acceptance-pullrequest-batch(master)/integration-hypersonic
• test-portal-acceptance-pullrequest-batch(master)/integration-mysql
• test-portal-acceptance-pullrequest-batch(master)/integration-postgresql
• test-portal-acceptance-pullrequest-batch(master)/modules-integration-hypersonic
• test-portal-acceptance-pullrequest-batch(master)/modules-integration-mysql
• test-portal-acceptance-pullrequest-batch(master)/modules-integration-postgresql
• test-portal-acceptance-pullrequest-batch(master)/modules-unit
• test-portal-acceptance-pullrequest-batch(master)/plugins-compile
• test-portal-acceptance-pullrequest-batch(master)/portal-web
• test-portal-acceptance-pullrequest-batch(master)/ruby-sass-compiler
• test-portal-acceptance-pullrequest-batch(master)/service-builder
• test-portal-acceptance-pullrequest-batch(master)/tck
• test-portal-acceptance-pullrequest-batch(master)/unit
• test-portal-acceptance-pullrequest-source(master)

For more details click here.

---

Failed Jobs:

1. test-portal-acceptance-pullrequest(master)

   Job Results:

   22 Jobs Passed.
   3 Jobs Failed.

   Completed with the status of FAILURE.

2. test-portal-acceptance-pullrequest-batch(master)/functional-tomcat-mysql/6

   Job Results:

   23 Tests Passed.
   1 Test Failed.

   1. PoshiRunner.test[PGWiki#DeleteWikiFrontPage] - Poshi Report - Poshi Summary - Console Output

3. test-portal-acceptance-pullrequest-batch(master)/modules-integration-mysql

   Job Results:

   4195 Tests Passed.
   1 Test Failed.

   1. PortalLogAssertorTest.testScanXMLLog

[shuyangzhou]

@hhuijser @daledotshan @bejondshao Sorry for the late review, but I am having a very hard time on understanding what this is. Could you please clean it up a bit and resend with some explanation?

For example, I see some generated code like this:

        List<Long> inlinePermissionEnabledGroupIds = new ArrayList<Long>();
        List<Long> inlinePermissionNotEnabledGroupIds = new ArrayList<Long>();

        for (long groupId : groupIds) {
            if (InlineSQLHelperUtil.isEnabled(groupId)) {
                inlinePermissionEnabledGroupIds.add(groupId);
            }
            else {
                inlinePermissionNotEnabledGroupIds.add(groupId);
            }
        }

And eventually, inlinePermissionEnabledGroupIds or inlinePermissionNotEnabledGroupIds will be converted back to groupIds, why are we making this many unnecessary conversion?

And the "List filterResults" is even more confusing, it used to be fallback to non-filter finder calls when inline permission is off, but seem right now it gets 2 sources of query results, won't that be duplicated?

Anyway, this pull is very confusing to me, please refine the logic to make it cleaner.

Thanks

Shuyang

[bejondshao]

Hi Shuyangzhou, @hhuijser , @daledotshan ,

Let's take DDMStructurePersistenceImpl.java as example.

1. For duplicated conversion.
   At first, I want to use Long[] inlinePermissionEnabledGroupIds = Long[]; but you can see, I can't make sure the site of new array. Besides, when we do
   for (long groupId : groupIds) { if (InlineSQLHelperUtil.isEnabled(groupId)) { inlinePermissionEnabledGroupIds.add(groupId); } else { inlinePermissionNotEnabledGroupIds.add(groupId); } }
   We have to use ArrayUtil.append(groupId), this method would copy the whole array to a new array. I don't think it's good to use this method in a loop. While list.add(groupId) is fast. After the loop, we just need to covert arrayList to array once for each list.
2. For filterResults.

The issue of LPS-57634 is that when there are more than one groups in the groupIds, portal do "!InlineSQLHelperUtil.isEnabled(groupIds)" checking for the whole groupIds, to decide if use non-filter finder or not. This is not comprehensive. It will only use non-filter finder when permissionCheck is groupAdcmin of all groups. It will use filter finder when permissionCheck is not groupAdmin of all groups. If permissionCheck is groupAdmin of one group, doing filter finder would return nothing. So this causes site admin can't view structure.

So I check groupId one by one and spilt them into two parts.
One part is "!InlineSQLHelperUtil.isEnabled(groupId)", this means permissionCheck has permission to do site admin actions. So we take these groups as non-filter finder, so all structures in these groups would be fetched.
The other part is "InlineSQLHelperUtil.isEnabled(groupId)", this means permissionCheck doesn't have permission to do site admin actions. So we take these groups as filter finder.
So two filter are not duplicated in this situation.

Would you reconsider it? If you think use list to array is not good, would you give me a suggestion for it?

Thanks
Bejond