[MAGNOLIA-2293] startLevel of simpleSearch is ignored Created: 02/Aug/08  Updated: 23/Jan/13  Resolved: 25/Nov/09

Status: Closed
Project: Magnolia
Component/s: taglibs
Affects Version/s: 3.6, 3.6.1, 3.6.3, 4.0.1, 4.2.1
Fix Version/s: 4.1.2, 4.2.2

Type: Bug Priority: Minor
Reporter: Oliver Knorr Assignee: Jan Haderka
Resolution: Fixed Votes: 1
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified
Environment:

Windows Vista Business SP1
jdk1.6.0_07
apache-tomcat-5.5.26
magnolia-bundled-webapp-3.6.1


Attachments: File SimpleSearchTagPatch.diff    
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Date of First Response:

 Description   

The startLevel attribute of cmsu:simplesearch should limit the search only to the current website tree, but is ignored at least in Magnolia 3.6 and 3.6.1.

Steps to reproduce:

  1. Open the demo search page http://localhost:8080/magnoliaAuthor/help/search.html and search for "mail". 5 results from inside and outside the help tree should appear.
  2. In webapps\magnoliaAuthor\templates\samples\templates\sample-search.jsp, replace the line
    <cmsu:simpleSearch query="${param.query}" var="results" />
    with the line
    <cmsu:simpleSearch query="${param.query}" var="results" startLevel="1" />
  3. Try the search again. Expected result: one hit (/magnoliaAuthor/help/user-mailing-list.html),
    actual result: all the 5 results from the first test are shown.

The reason seems to be info\magnolia\cms\taglibs\util\SimpleSearchTag.java: the startLevel is respected only in the obsolete generateXPathQuery() method, but not in the new generateSimpleQuery().

I could fix the problem for me, by replacing

    protected String generateSimpleQuery(String input) {
        // jcr and xpath escaping :
        final String escapedQuery = input.replace("'", "\\''");
        return "//*[@jcr:primaryType='mgnl:content']//*[jcr:contains(., '"+ escapedQuery +"')]";
    }

with the following (taking the code from generateXPathQuery()):

    protected String generateSimpleQuery(String input) {
        // search only in a specific subtree
        startPath = "";
        if (this.startLevel != 0) {
            try {
                Content activePage = Resource.getActivePage();
                if (activePage != null) {
                    startPath = StringUtils.strip(activePage.getAncestor(this.startLevel).getHandle(), "/"); //$NON-NLS-1$
                }
            }
            catch (RepositoryException e) {
                log.error(e.getMessage(), e);
            }
        }

        // jcr and xpath escaping :
        final String escapedQuery = input.replace("'", "\\''");
        return startPath+"//*[@jcr:primaryType='mgnl:content']//*[jcr:contains(., '"+ escapedQuery +"')]";
    }


 Comments   
Comment by Will Scheidegger [ 08/Apr/09 ]

The proposed fix would overwrite any startPath attribute. Therefore I would suggest to do it like this:

 
    protected String generateSimpleQuery(String input) { 
        // search only in a specific subtree 
        if (StringUtils.isBlank(this.startPath)) { 
            if (this.startLevel > 0) { 
                try { 
                    Content activePage = Resource.getActivePage(); 
                    if (activePage != null) { 
                        this.startPath = StringUtils.strip(activePage.getAncestor(this.startLevel).getHandle(), "/"); //$NON-NLS-1$ 
                    } 
                } catch (RepositoryException e) { 
                    log.error(e.getMessage(), e); 
                } 
            } else { 
                this.startPath = ""; 
            } 
        } else { 
            this.startPath = StringUtils.strip(this.startPath, "/"); //$NON-NLS-1$ 
        } 

        // jcr and xpath escaping : 
        final String escapedQuery = input.replaceAll("'", "\\\\''"); 
        String queryString = this.startPath + "//*[@jcr:primaryType='mgnl:content']//*[jcr:contains(., '" + escapedQuery + "')]"; 
        log.debug("query string: " + queryString); 
        return queryString; 
    } 

I tried to put create a patch file with this fix (see attached SimpleSearchTagPatch.diff), but since I let my IDE format the code, I got tons of diffs. I removed the unnecessary stuff manually and hope the patch file still works.

Comment by Magnolia International [ 24/Apr/09 ]

Could you please provide testcases (i.e augment the existing one) - i'd then be happy to apply the patch, knowing it does what it's meant to do

edit - actually I just meant testing the generateSimpleQuery() method, so the current test is already too complex.

Comment by Magnolia International [ 22/Jun/09 ]

Refactored and applied patch - needs testing.
Testcases, even if only in pseudo code, would be much welcome

Comment by Will Scheidegger [ 17/Jul/09 ]

Well, I just found out that the code above only works once properly if you use the tag with a startLevel attribute. If the tag has no "startPath" set, but does have a "startLevel" the path is calculated from the active page and then stored in the "startPath". Since from then on the tag does have a value in "startPath" the path is never calculated again. So if you use the tag in different contexts (e.g. in two differen language branches) after its first use it will always search in that branch. Using a temporary path for the query solves the problem.

    protected String generateSimpleQuery(String input) {
    	String queryStartPath = "";  // temporary start path used only for this query
        // search only in a specific subtree
        if (StringUtils.isBlank(this.startPath) && this.startLevel > 0) {
            try {
                Content activePage = Resource.getActivePage();
                if (activePage != null) {
                    queryStartPath = StringUtils.strip(activePage.getAncestor(this.startLevel).getHandle(), "/"); //$NON-NLS-1$
                }
            }
            catch (RepositoryException e) {
                log.error(e.getMessage(), e);
            }
        } else if (StringUtils.isNotBlank(this.startPath)) {
            queryStartPath = this.startPath;
        }
        // jcr and xpath escaping :
        final String escapedQuery = input.replaceAll("'", "\\\\''");
        return queryStartPath + "//*[@jcr:primaryType='mgnl:content']//*[jcr:contains(., '"+ escapedQuery +"')]";
    }
Comment by Magnolia International [ 17/Jul/09 ]

Could you please provide a patch again the current trunk? It might help highlighting the issue. (I haven't applied the previous ones 1:1)

Thanks !

Comment by Jan Haderka [ 25/Nov/09 ]

startPath is a member variable. Surely the proper course of action is to release the value of all member variables in the release() method of the tag no?

Generated at Mon Feb 12 03:35:18 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.