Skip to content

Add per column regex filtering#108

Open
erssebaggala wants to merge 3 commits intoPegase745:masterfrom
erssebaggala:per_column_regex_filter
Open

Add per column regex filtering#108
erssebaggala wants to merge 3 commits intoPegase745:masterfrom
erssebaggala:per_column_regex_filter

Conversation

@erssebaggala
Copy link

This PR adds regex filtering for each column. The current version of sqlalchemy-datatables only support regex filtering for the global search.

The regex is used by setting the search_method to regex in the column definition i.e. when creating the ColumnDT instance.

@Pegase745
Copy link
Owner

Could you add a test please?

@codecov
Copy link

codecov bot commented Aug 12, 2018

Codecov Report

Merging #108 into master will increase coverage by 0.06%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #108      +/-   ##
==========================================
+ Coverage   93.68%   93.75%   +0.06%     
==========================================
  Files           1        1              
  Lines         190      192       +2     
==========================================
+ Hits          178      180       +2     
  Misses         12       12
Impacted Files Coverage Δ
datatables/__init__.py 93.75% <75%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d1e686a...65bd440. Read the comment docs.

@erssebaggala
Copy link
Author

@Pegase745 I have added a test for this. The global search regex is failing. The culprit is :

val = clean_regex(global_search)

I could be wrong but from my tests, it is unnecessarily escaping the regex characters.

Why is there a need to clean the regex? Shouldn't it be up to the user to escape any special characters in the search value?

Copy link
Collaborator

@tdamsma tdamsma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to have this addition to the tool! I think we should deal with case sensitivity more consistently, would like to see that improved. Nice that you have implemented the testing with sqlite, perhaps we should expand the tests a bit more.

self.query.session.bind.dialect,
postgresql.dialect):
return '~'
return '~*'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please document and motivate the change from case sensitive to case insensitive searching. And perhaps even better to leave as it is, as I understand the MySQL REGEXP operator is case sensitive as well. Perhaps regexpi functions should also be added?

if value:
search_func = search_methods[self.columns[i].search_method]
filter_expr = search_func(self.columns[i].sqla_expr, value)
if self.columns[i].search_method == 'regex':
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this extra if/else statement here, but see that it is done because in this case the query method is is actually a function of the dialect. Perhaps the code needs to be reorganized a bit to allow for this, but the current implementation is a bit hacky

@tdamsma
Copy link
Collaborator

tdamsma commented Aug 20, 2018

I believe the regexes are cleaned for security reasons, as exposing full regex functionality might be a security risk. Not sure how much of a real vulnerability this is with an up to date postgres or mysql backend though.

https://www.owasp.org/index.php/Regular_expression_Denial_of_Service_-_ReDoS
https://stackoverflow.com/questions/25269811/is-it-safe-to-let-users-enter-custom-regex-patterns#25269866

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants