New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ability for the main Umbraco search to include media file names #6579
Add ability for the main Umbraco search to include media file names #6579
Conversation
@Jeavon should we add this as a constant here? and then use it like this similar to
|
@bjarnef yes we should, will add it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the field needs to be prefixed with __ generally that indicates it's a system field that isn't analyzed and can only be exact matched. Have left a couple other comments too.
Cheers!
@Shazwazza trying to come up with a Lucene query that will allow for trailing wildcard but where symbols (hyphens, fullstops, undersores) are allowed, any ideas..... |
@Jeavon don't worry about searching on Key, i've added that in a different PR, see #6530 As for your question regarding escaping chars and allowing wildcards, can you make anything work in Luke? Does the wildcard query work when you aren't escaping chars? Is it because the hyphens gets stripped out by the standard analyzer and for this field it should use whitespace analyzer? |
@Shazwazza I spent quite a while in Luke, the only thing I could make work was to replace the hyphens with spaces |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Jeavon, found a few other potential changes/questions/comments. Regarding searching with hyphens and wildcards, i found this SO thread: https://stackoverflow.com/questions/16858880/java-lucene-search-query-hyphens-with-wildcards
Could be another more effective work around: https://stackoverflow.com/a/36320124/694494)
Else the correct way to do it would be to use a custom analyzer for this field that strips out hyphens
|
||
if (!string.IsNullOrEmpty(umbracoFilePath)) | ||
{ | ||
var uri = new Uri(_runtimeState.ApplicationUrl.GetLeftPart(UriPartial.Authority) + umbracoFilePath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to use the application URL here? i mean, all this is really doing is constructing a Uri just so that it parses, you could just as easily have a static URI with a dummy hostname, etc... so you aren't allocating a new object for this each time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could have a dummy URI but that felt somehow wrong (I could also split the string), but can do that, what would you suggest for a dummy URI (is there a precedent)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a dummy URL would be better and then we don't need to inject the IRuntimeState, at the end of the day it won't make any difference and we don't need to change as much
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Jeavon this will be the last thing we need to cleanup i think and then we can merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Shazwazza updated, thanks!
I like the ? idea, I'll try that also |
@Shazwazza |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple questions left inline
|
||
if (!string.IsNullOrEmpty(umbracoFilePath)) | ||
{ | ||
var uri = new Uri(_runtimeState.ApplicationUrl.GetLeftPart(UriPartial.Authority) + umbracoFilePath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a dummy URL would be better and then we don't need to inject the IRuntimeState, at the end of the day it won't make any difference and we don't need to change as much
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry @Jeavon just a couple small tweaks, sorry for being such a pain ;)
…zation issues and therefore needing to mock the logger for the tests
Awesome stuff @Jeavon all merged in 🎉 |
The PR adds a new field to the MediaValueSet called
"__umbracoFile"
,"__"
prefixed as this is to be used internally only.The new field is populated either by extracting the Src property from the Image Cropper Json or directly from the property in the case of files such as PDFs.
We have also updated the UmbracoTreeSearcher to include the new field and modified the mapper to include the filename in the search results in braces.