search

wayf-dk / janus-ssp

Automatically exported from code.google.com/p/janus-ssp
Other
0 stars 0 forks source link

Entity is missing if prettyname metadatafield is not present #289

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
What steps will reproduce the problem?
1. Create an entity and fill inn with metadata except the metadatafield that is 
used as prettyname
2. Search for the entity by entityID

What is the expected output? What do you see instead? If possible, please
give an URL the page with the defect.

I would expect that the entity would apear in the list, bur it doesn't.

What version of the product are you using?

JANUS version: 1.10.0
SimpleSAMLphp version: 1.7.0
Browser: Chrome
PHP version: 5.2.3
Database version: MySQL 5.1.41
OS: Ubuntu 10.04

Please provide any additional information below:

The problem lies in:

sspmod_janus_UserController->_loadEntities($state = null, $state_exclude = null)

// If the fieldName is set, also add the fieldName SQL, Join SQL and orderBy SQL
if ($fieldName) {
    $fieldNameSQL = ' AND jm.key = \''. $fieldName .'\'';
    $orderBySQL = ' ORDER BY jm.value ASC;';
}

$fieldNameSQL will never match any if the prettyname field is not attached to 
the entity, even if you search for the entityID.

$fieldname is set if entity.prettyname is set in the config file.

Original issue reported on code.google.com by j...@wayf.dk on 27 Oct 2011 at 12:29

GoogleCodeExporter commented 9 years ago 
This is a bug introduced by r821 (issue 240)

Original comment by j...@wayf.dk on 27 Oct 2011 at 12:35

GoogleCodeExporter commented 9 years ago

Original comment by j...@wayf.dk on 27 Oct 2011 at 12:36

GoogleCodeExporter commented 9 years ago 
[deleted comment]
GoogleCodeExporter commented 9 years ago 
This query should do "most" the trick

SELECT E1.`eid`, M1.`value`
FROM 
    (SELECT DISTINCT `eid`
    FROM `janus__entity`) E1
NATURAL LEFT OUTER JOIN
    (SELECT M1.`eid`, M1.`value`
    FROM `janus__metadata` M1,
        (SELECT `eid`, MAX(`revisionid`) AS `revisionid`
        FROM `janus__entity`
        GROUP By `eid`) E2
    WHERE M1.`eid` = E2.`eid` AND M1.`key` = 'name:da' AND M1.`revisionid` = E2.`revisionid`) M1
ORDER BY M1.`value`;

It joins a temporary table of all entities that have a prettyname field 
defined, with a complele list of all entities. The result is a list sorted on 
prettyname and the entities that do not have the prettyname field are put at 
the fron of the list.

Problem: How to get en entityid out, so it can be used to sort on i conjunction 
with the prettyname fields.

Original comment by j...@wayf.dk on 28 Oct 2011 at 2:24

GoogleCodeExporter commented 9 years ago 
This will secondary sort on entityid

SELECT E1.`eid`, E1.`revisionid`, M1.`value`, E1.`entityid`
FROM 
    (SELECT `eid`, MAX(`revisionid`) AS `revisionid`, `entityid`
    FROM `janus__entity`
    GROUP By `eid`) E1
NATURAL LEFT OUTER JOIN
    (SELECT M1.`eid`, M1.`value`
    FROM `janus__metadata` M1,
        (SELECT `eid`, MAX(`revisionid`) AS `revisionid`
        FROM `janus__entity`
        GROUP By `eid`) E2
    WHERE M1.`eid` = E2.`eid` AND M1.`key` = 'name:da' AND M1.`revisionid` = E2.`revisionid`) M1
ORDER BY M1.`value`, E1.`entityid`;

Not quite finished though

Original comment by j...@wayf.dk on 28 Oct 2011 at 2:34

GoogleCodeExporter commented 9 years ago 
Finalised query to solve the problem:

SELECT E1.`eid`, IFNULL(M1.`value`, E1.`entityid`) AS `orderfield` # combine 
value and entityId to field that the sort is done on
FROM 
    (SELECT `eid`, MAX(`revisionid`) AS `revisionid`, `entityid` # Used to get latest entityId of all entities
    FROM `janus__entity`
    GROUP By `eid`) E1 
NATURAL LEFT OUTER JOIN # Combine the two. Set NULL if prettyname field is not 
set
    (SELECT M1.`eid`, M1.`value`
    FROM `janus__metadata` M1,
        (SELECT `eid`, MAX(`revisionid`) AS `revisionid` # Get latest revision number for all entities
        FROM `janus__entity`
        GROUP By `eid`) E2
    WHERE M1.`eid` = E2.`eid` AND M1.`key` = 'name:da' AND M1.`revisionid` = E2.`revisionid`) M1 # Ensure latest revision and value is set
ORDER BY `orderfield`;

Original comment by j...@wayf.dk on 31 Oct 2011 at 1:09

GoogleCodeExporter commented 9 years ago 
Hi, Since we seem to have caused the issue I was working on a fix also. As 
usual 2 programmers come up with a different approach.

I noticed the _loadEntities() method had a lot of duplicate code and subqueries 
which made it hard to read therefore I did I rewrite of the method in which 
functionality is grouped instead of duplicated. This new approach (using left 
join) allows for sorting on pretty name if available.

Please review attached patch to see our solution.

Btw I am the successor of Boy Baukema at Surfnet so I would like to ask if I 
can get commit rights on janus?

This is my profile:
http://code.google.com/u/110492220067764126629/

Original comment by vanliero...@gmail.com on 31 Oct 2011 at 2:21

Attachments:

GoogleCodeExporter commented 9 years ago 
I'll have a look at the patch tomorrow.

Original comment by j...@wayf.dk on 31 Oct 2011 at 3:51

GoogleCodeExporter commented 9 years ago 
The patch still do not properly sort, the entities that do not have a 
prettyname set and do not take into account the entities that have the default 
value set on the prettyname field. This patch should do the trick.

Original comment by j...@wayf.dk on 1 Nov 2011 at 12:40

Attachments:

GoogleCodeExporter commented 9 years ago 
Hi Jacob, thanks for looking and tweaking our patch, will you apply it to the 
trunk?

Original comment by vanliero...@gmail.com on 1 Nov 2011 at 12:56

GoogleCodeExporter commented 9 years ago 
I have adde you as a commiter, so please apply the patch. Please also apply it 
to the v.1.10 branch as well, since we need to do a new release, containing 
this patch.

Original comment by j...@wayf.dk on 1 Nov 2011 at 6:16

GoogleCodeExporter commented 9 years ago 
Fixed by r898 (trunk) and r899 (branch v1.10)

I had to make a small tweak your code since the config contains entries
like ''name:#'' instead of 'name:en' (which we were using) which caused
notices when trying to find default values in config so I've added:
$sortFieldConfigName = preg_replace('/:[^:]+/i', ':#', $sortFieldName);
to fix that, is the result ok for you like this?

Btw thanks very much for the commit rights

Original comment by vanliero...@gmail.com on 2 Nov 2011 at 10:06

GoogleCodeExporter commented 9 years ago 
The tweak involving name:# gives problems in out end, since the configuration 
in our end is name:da. I'll have a look at it.

Original comment by j...@wayf.dk on 2 Nov 2011 at 11:39

GoogleCodeExporter commented 9 years ago 
I do not think that your tweak work. 

Fx. if people use name:en:displayName as the prettyname field and the fieldname 
is fefined as name:#:displayName. This will be trasformed to name:#:#, which is 
not defined.

I think that the only way to do this is to force users do define the field used 
as prettyname.

Original comment by j...@wayf.dk on 2 Nov 2011 at 11:57

GoogleCodeExporter commented 9 years ago 
I propose this solution:

Index: lib/UserController.php
===================================================================
--- lib/UserController.php  (revision 899)
+++ lib/UserController.php  (working copy)
@@ -146,13 +146,16 @@

         // Find default value for sort field so it can be excluded
         $sortFieldName = $this->_config->getString('entity.prettyname', NULL);
-        $sortFieldConfigName = preg_replace('/:[^:]+/i', ':#', $sortFieldName);
+        $queryData['default_value'] = '';
+        
         if ($sortFieldDefaultValue = $this->_config->getArray('metadatafields.saml20-idp', FALSE)) {
-            $queryData['default_value'] = 
$sortFieldDefaultValue[$sortFieldConfigName]['default'];
+            if (isset($sortFieldDefaultValue[$sortFieldName])) {
+                $queryData['default_value'] = 
$sortFieldDefaultValue[$sortFieldName]['default'];
+            }
         } else if ($sortFieldDefaultValue = $this->_config->getArray('metadatafields.saml20-sp', FALSE)) {
-            $queryData['default_value'] = 
$sortFieldDefaultValue[$sortFieldConfigName]['default'];
-        } else {
-            $queryData['default_value'] = '';
+            if (isset($sortFieldDefaultValue[$sortFieldName])) {
+                $queryData['default_value'] = 
$sortFieldDefaultValue[$sortFieldName]['default'];
+            }
         }

         // Try to sort results by pretty name from metadata

Original comment by j...@wayf.dk on 2 Nov 2011 at 12:02

GoogleCodeExporter commented 9 years ago 
Just to be sure I do understand what goes wrong, you mean you have configured 
something like?:

'name:da' => array(
            'type' => 'text',
            'order' => 3100,
            'default' => 'CHANGE THIS',
            'default_allow' => false,
            'supported' => array('en', 'da')
        ),

instead of:

'name:#' => array(
            'type' => 'text',
            'order' => 3100,
            'default' => 'CHANGE THIS',
            'default_allow' => false,
            'supported' => array('en', 'da')
        ),

Original comment by vanliero...@gmail.com on 2 Nov 2011 at 12:14

GoogleCodeExporter commented 9 years ago 
[deleted comment]
GoogleCodeExporter commented 9 years ago 
Ah you replied already while I was typing..

I see the issue with the double #, I will check your solution now

Original comment by vanliero...@gmail.com on 2 Nov 2011 at 12:17

GoogleCodeExporter commented 9 years ago 
Your solution seems to work fine for us, I would say this is ok for now.

Original comment by vanliero...@gmail.com on 2 Nov 2011 at 12:30

GoogleCodeExporter commented 9 years ago 
Our configuration is like this:

'name:da' => array(
            'type' => 'text',
            'order' => 3100,
            'default' => 'CHANGE THIS',
            'default_allow' => false,
),

this means that search for 'name:#' is not going to work

Original comment by j...@wayf.dk on 2 Nov 2011 at 1:37

GoogleCodeExporter commented 9 years ago 
Ah that explains it all, I wasn't aware of the fact that it was it used like 
this also but I understand why it breaks at your side now. I think you should 
commit your proposed change.
After that we could optionally improve it sometime to use pretty name (like 
'name:da') first with a fallback to name:#'. In that case we should fix the 
'name:#:#' issue you mentioned.

Original comment by vanliero...@gmail.com on 3 Nov 2011 at 7:50

GoogleCodeExporter commented 9 years ago 
This issue was closed by revision r905.

Original comment by j...@wayf.dk on 3 Nov 2011 at 10:15