[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 |
||
| Attachments: |
|
| 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:
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. |
| 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? |