1

I am building a forum application in Django and I want to make sure that users dont enter certain characters in their forum posts. I need an efficient way to scan their whole post to check for the invalid characters. What I have so far is the following although it does not work correctly and I do not think the idea is very efficient.

def clean_topic_message(self):
    topic_message = self.cleaned_data['topic_message']
    words = topic_message.split()
    if (topic_message == ""):
        raise forms.ValidationError(_(u'Please provide a message for your topic'))
    ***for word in words:
        if (re.match(r'[^<>/\{}[]~`]$',topic_message)):
            raise forms.ValidationError(_(u'Topic message cannot contain the following: <>/\{}[]~`'))***
    return topic_message

Thanks for any help.

vt-cwalker
  • 795
  • 2
  • 16
  • 34

9 Answers9

6

For a regex solution, there are two ways to go here:

  1. Find one invalid char anywhere in the string.
  2. Validate every char in the string.

Here is a script that implements both:

import re
topic_message = 'This topic is a-ok'

# Option 1: Invalidate one char in string.
re1 = re.compile(r"[<>/{}[\]~`]");
if re1.search(topic_message):
    print ("RE1: Invalid char detected.")
else:
    print ("RE1: No invalid char detected.")

# Option 2: Validate all chars in string.
re2 =  re.compile(r"^[^<>/{}[\]~`]*$");
if re2.match(topic_message):
    print ("RE2: All chars are valid.")
else:
    print ("RE2: Not all chars are valid.")

Take your pick.

Note: the original regex erroneously has a right square bracket in the character class which needs to be escaped.

Benchmarks: After seeing gnibbler's interesting solution using set(), I was curious to find out which of these methods would actually be fastest, so I decided to measure them. Here are the benchmark data and statements measured and the timeit result values:

Test data:

r"""
TEST topic_message STRINGS:
ok:  'This topic is A-ok.     This topic is     A-ok.'
bad: 'This topic is <not>-ok. This topic is {not}-ok.'

MEASURED PYTHON STATEMENTS:
Method 1: 're1.search(topic_message)'
Method 2: 're2.match(topic_message)'
Method 3: 'set(invalid_chars).intersection(topic_message)'
"""

Results:

r"""
Seconds to perform 1000000 Ok-match/Bad-no-match loops:
Method  Ok-time  Bad-time
1        1.054    1.190
2        1.830    1.636
3        4.364    4.577
"""

The benchmark tests show that Option 1 is slightly faster than option 2 and both are much faster than the set().intersection() method. This is true for strings which both match and don't match.

ridgerunner
  • 33,777
  • 5
  • 57
  • 69
3

You have to be much more careful when using regular expressions - they are full of traps.

in the case of [^<>/\{}[]~] the first ] closes the group which is probably not what you intended. If you want to use ] in a group it has to be the first character after the [ eg []^<>/\{}[~]

simple test confirms this

>>> import re
>>> re.search("[[]]","]")
>>> re.search("[][]","]")
<_sre.SRE_Match object at 0xb7883db0>

regex is overkill for this problem anyway

def clean_topic_message(self):
    topic_message = self.cleaned_data['topic_message']
    invalid_chars = '^<>/\{}[]~`$'
    if (topic_message == ""):
        raise forms.ValidationError(_(u'Please provide a message for your topic'))
    if set(invalid_chars).intersection(topic_message):
        raise forms.ValidationError(_(u'Topic message cannot contain the following: %s'%invalid_chars))
    return topic_message
John La Rooy
  • 295,403
  • 53
  • 369
  • 502
  • Interesting solution (and it does work). However, this method is much slower than using regex. See my answer for some benchmarks. Also, moving the `]` to the "head-of-the-class" displaces the initial `^` which significantly changes its meaning (i.e. its no longer a negated character class). Much simpler to just escape it: `\]` – ridgerunner Apr 18 '11 at 16:14
  • @ridgerunner, my point was that regex like this are easy to get wrong. It's better to have a fast enough correct solution than a wrong one. Especially if the wrong one looks right to a bunch of people. Quite a few answers here were already copying and modifying the regex from the OP without noticing the problem with ']'. – John La Rooy Apr 19 '11 at 09:46
  • Yes I agree. If I had a nickel for every crappy regex I've looked at I would be rich! Regular expressions should be treated like a loaded weapon - its way too easy to shoot yourself in the foot (and many do just that). However, once mastered, regular expressions are a powerful tool indeed. I strongly believe that [Mastering Regular Expressions (3rd Edition)](http://www.amazon.com/Mastering-Regular-Expressions-Jeffrey-Friedl/dp/0596528124 "By Jeffrey Friedl. Best book on Regex - ever!") should be required reading for anyone using regular expressions for production code. – ridgerunner Apr 19 '11 at 14:43
  • Yea I definitely would make sure to know i have them right before putting them into production. But just fyi Im doing this project just to learn programming more in depth to get better at it before I get my computer science degree. Just so you all can be at peace with that. LOL Thanks for your alls help! – vt-cwalker Apr 19 '11 at 19:53
2

If efficiency is a major concern I would re.compile() the re string, since you're going to use the same regex many times.

phunctor
  • 599
  • 3
  • 9
  • @user711569, where should you put the compiled `re` in django? If you leave the compilation anywhere in that method, compiling is no better. – Mike Pennington Apr 18 '11 at 03:54
  • 1
    @Mike, Python caches the compiled version of the most recently regex. So you don't need to bother explictly compiling it if you just have a loop with one regex. There is also a cache for regex that you have compiled explicitly, so it would not be recompiled for each function call unless the size of the cache is exceeded – John La Rooy Apr 18 '11 at 05:30
  • FYI, for Python 2.4 and later, the Regex library will cache the last 100 expressions ( `import sre ; print sre._MAXCACHE` ). – Craig Trader Apr 19 '11 at 16:06
2

re.match and re.search behave differently. Splitting words is not required to search using regular expressions.

import re
symbols_re = re.compile(r"[^<>/\{}[]~`]");

if symbols_re.search(self.cleaned_data('topic_message')):
    //raise Validation error
dheerosaur
  • 14,736
  • 6
  • 30
  • 31
1

I can't say what would be more efficient, but you certainly should get rid of the $ (unless it's an invalid character for the message)... right now you only match the re if the characters are at the end of topic_message because $ anchors the match to the right-hand side of the line.

Mike Pennington
  • 41,899
  • 19
  • 136
  • 174
1

In any case you need to scan the entire message. So wouldn't something simple like this work ?

def checkMessage(topic_message):
  for char in topic_message:
       if char in "<>/\{}[]~`":
           return False
  return True
jack_carver
  • 1,510
  • 2
  • 13
  • 28
1

is_valid = not any(k in text for k in '<>/{}[]~`')

Melug
  • 1,023
  • 9
  • 14
1

I agree with gnibbler, regex is an overkiller for this situation. Probably after removing this unwanted chars you'll want to remove unwanted words also, here's a little basic way to do it:

def remove_bad_words(title):
'''Helper to remove bad words from a sentence based in a dictionary of words.
'''
word_list = title.split(' ')
for word in word_list:
    if word in BAD_WORDS: # BAD_WORDS is a list of unwanted words
        word_list.remove(word)
#let's build the string again
title2 = u''
for word in word_list:
    title2 = ('%s %s') % (title2, word)
    #title2 = title2 + u' '+ word

return title2
draix
  • 11
  • 2
0

Example: just tailor to your needs.

### valid chars: 0-9 , a-z, A-Z only
import re
REGEX_FOR_INVALID_CHARS=re.compile( r'[^0-9a-zA-Z]+' )
list_of_invalid_chars_found=REGEX_FOR_INVALID_CHARS.findall( topic_message )
jldupont
  • 93,734
  • 56
  • 203
  • 318
  • -1 (a) disallows spaces, "normal" punctuation like comma and full stop and apostrophe, etc etc. (b) The `+` in the regex is pointless (c) OP's question would require `search`, not `findall` (d) regex is overkill (e) regex is error-prone (f) it helps if you read all the answers and comments carefully before you decide you can add value to a question that is 9 months old – John Machin Jan 07 '12 at 18:28
  • @John Machin: This was an example of how to achieve what the OP asked. You can easily add whatever character is needed. And why wouldn't "findall" be appropriate, now tell me whywould that be?? And regex overkill... that's a good one: on what basis? – jldupont Jan 07 '12 at 22:24
  • @john machin: I read the OP's question, not the whole trail... and yes I am sorry for posting a *fine working* answer. – jldupont Jan 07 '12 at 22:26
  • "example of how to achieve what the OP asked": what he wanted can be pseudocoded as `any(c in invalid_chars for c in topic_message)`. I.e. he knows exactly what his invalid_chars are. You have given him approximately `list(c in valid_chars for c in topic_message)`. "approx" is due to the `+` in your regex. Your valid_chars is palpably a subset. It is ludicrous to suggest that he should augment it. "findall" produces a list, which he doesn't care about. For the rest, read the other answers and comments. I don't understand why you are sorry fort somethin that you have't done. – John Machin Jan 08 '12 at 00:50