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

allow regex from columns, to support yadcf multi_select #41

Merged
merged 7 commits into from
May 5, 2016

Conversation

louking
Copy link
Collaborator

@louking louking commented Mar 8, 2016

* support multi_select column filters from yadcf
* supports mysql (tested) postgresql (untested), otherwise feature is ignored
* code in place to support regex for search bar, but exception is raised when datatables sends incomplete regex so code was made dead
   * see http://datatables.net/forums/discussion/33699/search-regex-sub-expression-error
* fixes #40, at least partially
* based on top of 890907a

* support multi_select column filters from yadcf
* supports mysql (tested) postgresql (untested), otherwise feature is ignored
* code in place to support regex for search bar, but exception is raised when datatables sends incomplete regex so code was made dead
   * see http://datatables.net/forums/discussion/33699/search-regex-sub-expression-error
* fixes Pegase745#40, at least partially
* based on top of 890907a
@louking louking changed the title alow regex from columns, to support yadcf multi_select allow regex from columns, to support yadcf multi_select Mar 8, 2016
@louking
Copy link
Collaborator Author

louking commented Mar 9, 2016

To support global search regex, Allan (dataTables) had answer to return empty table for intermediate result, i.e., when regex is invalid. See http://datatables.net/forums/discussion/33699/search-regex-sub-expression-error

See http://stackoverflow.com/questions/19630994/how-to-check-is-a-string-is-a-valid-regex-python - but note this is not sufficient as mysql regex is different than python regex.

Also note answer http://stackoverflow.com/questions/28608702/django-operationalerror-error-repetition-operator-operand-invalid-from-the-r which suggests care when accepting regex because malicious user can cause DOS attack by constructing a bad regex.

I will add function to verify regex is within certain parameters. If this condition is not met, regex will be treated as "contains" string for global search and either as "contains" string or full string match (depending on column configuration) for column search.

I think this should have effect of returning empty table as suggested by Allan.

For now, function will accept non-null text with | in between only, but can be enhanced in future for other use cases.

@Pegase745
Copy link
Owner

Good job wanting to handle regex search. Is it possible for you to provide (and fix) unit tests please, and maybe even an example of the feature please ?

@louking
Copy link
Collaborator Author

louking commented Mar 9, 2016

I looked at the unit tests, but these look like they are built on sqlite. This feature only works with mysql and postgresql, unfortunately. Is it possible to set up mysql and postgresql databases?

You also said "(and fix) unit tests". Did I break anything? This feature should be backwards compatible.

@Pegase745
Copy link
Owner

yep, see this . the compatibility broke withe use of format

I'll think into dockerizing some examples with mysql and postgresql then. I think it's a good timing. whatdo you think?

@louking
Copy link
Collaborator Author

louking commented Mar 9, 2016

Hmm. Not sure what is syntax error for python 3.4 and 3.5, but I'll look into that and fix.

Looks like I also left a debug print statement in, sorry, will fix that

I've not used docker so I'm not the right one to ask what I think. :) But I'm happy to learn new things.

#39 was my first PR ever, so this is learning experience for me.

@Pegase745
Copy link
Owner

Happy that my project was that interesting for you to start contributing :) I appreciate that 👍

Docker is easy, I'll try something asap

@louking
Copy link
Collaborator Author

louking commented Mar 9, 2016

I think all unit test failures have been fixed now

@louking
Copy link
Collaborator Author

louking commented Mar 9, 2016

Note I also plan to improve this based on Allan's input, but as it stands it works for columns. If you decide to merge I will make my additional changes on top of new master.

@Pegase745 Pegase745 added this to the 0.3.0 milestone Mar 11, 2016
louking added 2 commits March 11, 2016 12:09
* ignores any special regex characters [^$.?*+(){} except for alternation
* escape characters are removed
* all special regex characters are escaped
* if alternation | at end of string it is removed to avoid error
* note special case of || is made into | to avoid error
@louking
Copy link
Collaborator Author

louking commented Mar 11, 2016

This completes my changes for regex.

I'm blocked on the filterarg tests for now (ref #39) I assume due to local environment problems, as sent to you via email. Unfortunately I am going out of town tomorrow and will not necessarily have time for next week, but if you have ideas on what is going wrong, please let me know and I might be able to find time to address.

@Pegase745
Copy link
Owner

No worries @louking . Thanks I'll look into that when I have time too.

@Pegase745
Copy link
Owner

@louking Hey, got anything new for that ?

@louking
Copy link
Collaborator Author

louking commented Mar 25, 2016

I think the way we left this pr was that it is done with exception of test
code. I think you were going to look into containers to support mysql and
posgres.

Just so you know, I was out of town for a week or so, and then had back
surgery yesterday. I'm out of commission for at least several days.

I couldn't get local tests working for the pr you merged, I think #39. I
was hoping you might have ideas on what is wrong with my environment.
On Mar 25, 2016 4:16 PM, "Michel Nemnom" [email protected] wrote:

@louking https://github.com/louking Hey, got anything new for that ?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#41 (comment)

@Pegase745
Copy link
Owner

Nothing's urgent of course ! I'll look into it if I have time. Get well soon :)

@louking
Copy link
Collaborator Author

louking commented Mar 25, 2016

Thanks
On Mar 25, 2016 6:15 PM, "Michel Nemnom" [email protected] wrote:

Nothing's urgent of course ! I'll look into it if I have time. Get well
soon :)


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#41 (comment)

@louking
Copy link
Collaborator Author

louking commented Apr 20, 2016

Was there a reason this never got merged? I know we were talking about unit tests for the other pull which did get merged. Do you need me to bring this up to date?

@Pegase745
Copy link
Owner

I don't see any tests on the regex part, unless I missed something :-/

@louking
Copy link
Collaborator Author

louking commented Apr 20, 2016

The change only affects mysql and postgresql. I don't think current testing environment can support testing for these databases. You had said you might do something with containers to resolve.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.9%) to 87.156% when pulling 2739809 on louking:regex-support-issue-#40 into 14d058a on Pegase745:master.

@louking
Copy link
Collaborator Author

louking commented Apr 20, 2016

Based on local unit testing (which I should have done before the push, sorry) I may have broken the global regex in the latest merge. Column regex seems to work ok. I should be able to look into this tomorrow.

@louking
Copy link
Collaborator Author

louking commented Apr 21, 2016

Update - my merged code works fine, there was a setup issue in my test environment. Please advise if I need to make any more changes before this can be merged.

@Pegase745 Pegase745 merged commit 2357727 into Pegase745:master May 5, 2016
@louking
Copy link
Collaborator Author

louking commented May 5, 2016

Thanks!

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

Successfully merging this pull request may close these issues.

3 participants