Skip to content
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

DedupeFind: Allow search datatable by regex on server side #8463

Conversation

niels-heinemann
Copy link
Contributor

@niels-heinemann niels-heinemann commented May 27, 2016

DataTable's column search() method allows searching by regex but this has to be reflected serverside.
In template use dtCol.search("myregex", true, false) and mysql's extended regular expressions are being utilized: where colName REGEX 'myregex'

See CRM-18694

@colemanw
Copy link
Member

colemanw commented May 27, 2016

@nielosz can you please open a ticket on https://issues.civicrm.org for this? (or link to it if one is already open). Thanks! (ok to have one issue for these 2 related PRs)

@niels-heinemann
Copy link
Contributor Author

I opened two tickets because I want the core team to have all choices :)

@josephlacey
Copy link
Contributor

I'm reviewing this now.

@JoeMurray
Copy link
Contributor

Heh @nielosz as Release Manager this month, I'm trying to recruit people to help pare down the backlog of almost 100 PRs, some going back to last summer. I'm wondering if you would be able to help QA another PR if I got someone to QA this PR and the related #8464 ?

@niels-heinemann
Copy link
Contributor Author

niels-heinemann commented Jun 14, 2016

hey @JoeMurray, yeah. I'll have a look. Do you have a list of PRs to be proofed?

@eileenmcnaughton
Copy link
Contributor

Prs are listed on here https://docs.google.com/spreadsheets/d/1fmHFOZ83ectSPCMWvXwCeJgrw4KpYi8JEV2ZDkd6XDU/edit#gid=529353056

More detail at
https://github.com/civicrm/release-management

You can add your name to the contributors list...

@josephlacey
Copy link
Contributor

@JoeMurray I'll note that this PR and the other related one on CRM-18694 are likely being subsumed under a larger rewrite of the function in question. @eileenmcnaughton should probably comment on whether that's still the case.

@JoeMurray
Copy link
Contributor

Thanks, @josephlacey - feel free to pick PRs if none have been assigned. Cheers!

@eileenmcnaughton
Copy link
Contributor

Yeah - I'll look at this more this week - definitely don't want to be messing with this function without adding tests. My initial concern was that this might be insecure. But I don't think it is. It will also perform poorly on large data sets - but that seems to be no worse than the existing LIKE code - in both cases a large prev_next cache will be slow here.

@JoeMurray
Copy link
Contributor

@nielosz could you please create a JIRA issue for this explaining the intent behind this PR? Thanks.

@JoeMurray
Copy link
Contributor

@eileenmcnaughton would you be interested in reviewing this PR?

@eileenmcnaughton
Copy link
Contributor

yes

@eileenmcnaughton
Copy link
Contributor

@nielosz I'm wondering about this PR.

Although you talk about changing to regex I see comments elsewhere suggesting this is about assuming people enter the postcode from the first char & not showing them every result with that char.

If that is the case then a more performant option is to swap from LIKE '%xyx%'; to LIKE 'xyz%'

This would work to do that

https://github.com/eileenmcnaughton/civicrm-core/compare/merge_perms...eileenmcnaughton:merge_like?expand=1

(Note I've done that against my PR not core due to the degree of change)

@niels-heinemann
Copy link
Contributor Author

@eileenmcnaughton, you're totally right. Just "hardcoding" the handling of zip codes would fully satisfy CRM-18694. I just wanted to be more generic. But reading you're hint on performance I must admit I didn't know that like queries make use of indices if the fist char isn't a wildcard. That means that your approach increases performance compared to the default '%keyword%' clauses.
Sorry for that.

@eileenmcnaughton
Copy link
Contributor

Yeah I did quite a bit of digging on that & did this write-up
https://civicrm.org/blog/eileen/would-you-like-coffee-while-you-wait-sir

OK - so I'll add this to my (big) PR & close your 2. If you have time to checkout the stuff in the big merge-stuff pr (#8456) that would be amazing. I think John Kingsnorth will take a look but the more eyes the better

@niels-heinemann
Copy link
Contributor Author

Okay, thanks!

@eileenmcnaughton
Copy link
Contributor

per discussion above - closing these in favour of the already merged chagnes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants