readable validation with python regex and simple logic

Question

I have come with such a code for checking IP addresses. But would like to know how clean and good it is from 1 to 10. What I care about are readability and simplicity. Please give me any feedback.

Should I go here with unittests?

import re
import logging

logging.basicConfig(level=logging.ERROR)

pattern = re.compile(r'^(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$')

def is_in_bounds(s): 
    return int(s) >= 0 and int(s) <= 255

def is_leading_zeros(s):
    if s is not '0':
        return s[0] is not '0'
    return True

def validate_ipv4(ipv4: str, expectFail: bool = False) -> bool:
    logging.info(f"Validating IP '{ipv4}' START.")

    try:
        match = pattern.match(ipv4)
        assert(match)

        groups = match.groups()
        assert(len(groups)==4)

        for s in match.groups():
            assert(is_leading_zeros(s))
            assert(is_in_bounds(s))
    
        logging.info(f"Validating IP '{ipv4}' SUCCESS.")

    except AssertionError:
        logging.info(f"Validating IP '{ipv4}' FAILED.", exc_info=True)
        if not expectFail:
            raise


if __name__ == '__main__':
    octets = []
    octets.extend(range(0,3))
    octets.extend(range(9,12))
    octets.extend(range(98,102))
    octets.extend(range(198,202))
    octets.extend(range(250,256))

    for i in octets:
        for j in octets:
            for k in octets:
                for l in octets:
                    validate_ipv4(f'{i}.{j}.{k}.{l}')

    octets = []
    octets.extend(range(-3,0))
    octets.extend(range(256, 260))  

    exceptions = []

    for i in octets:
        for j in octets:
            for k in octets:
                for l in octets:
                    validate_ipv4(f'{i}.{j}.{k}.{l}', True)

Answer 1

When to use is not

Your usage of is not is incorrect. You want to use == when you're testing equality, like when you check if s doesn't equal a '0'. You want to use is not when checking for identity, like when you're ensuring two variables are of the same object.

def is_leading_zeros(s: str) -> bool:
    if s != '0':
        return s[0] != '0'
    return True

Clearer comparisons

Python has a syntax that allows you to do this:

def is_in_bounds(s: str) -> int:
    return 0 <= int(s) <= 255

Type Hints

You should add types hints to your functions, which display what types are accepted and returned from your functions. See above for an example of this.

Answer 2

As Linny noted, is_leading_zeros has an error because you want != intead of is not. My Python 3.8.2 actually says SyntaxWarning: "is not" with a literal. Did you mean "!="? on those two lines. But my bigger problem is with the name of the function: is_leading_zeros(s) is True when s doesn't have leading zeros (a docstring could have cleared that up). You use it correctly in your code, but its name is the opposite of what you claim. You could call it has_no_leading_zeros, but that could lead to not has_no_leading_zeros(s) in other code, and I prefer to avoid double negatives. I would call it has_leading_zeros, and negate its use. Also, I would prefer s.startswith('0') to s[0]=='0'.

For the same reason of avoiding double negatives, I'm going to change expectFail to expectPass.

Linny also noted that you can simplify the is_in_bounds comparison and that you are missing some type hints. I've kept those changes as well.

You log that the validation failed when you expected it to fail. This would confuse me if I were looking through my logs and just saw "FAILED". I'll tack on "as expected".

Presumably, you want another python script to be able to use your function to test if something is a valid ipv4 address. But in that case, I would not expect a validator to raise an AssertionError. Let's separate out the logging and assertion portions. I'll also change for s in match.groups() into a generator expression and use all in order to shorten things up. That code feels more readable to me. Unfortunately, this loses the ability to have assertions on each line of the validation.

Are you familiar with itertools? You can change out your nested for loop with
for i,j,k,l in itertools.product(octets,repeat=4).

pattern is too generic of a name. Let's go with ipv4pattern.

Instead of ipv4pattern.match(ipv4), we can use ipv4pattern.fullmatch(ipv4) and drop the ^ and $ from the pattern. It's more readable that way. We could also change the pattern to the shorter r'\.'.join([r'(\d{1,3})']*4), but I don't find that as readable, so I won't do that.

You take several lines to build your octets list. I would go with list(range(0,3))+list(range(9,12))+..., but break it into two to avoid too long of a line.

You never use exceptions = [].

Your unit testing focuses completely on is_in_bounds. You never test that leading zeros aren't allowed, you never test that there are only four groups, and you never test that the pattern match succeeded. You also don't test that the validation fails if one of the groups is out of range, but the others are ok.

The final code:

import re
import logging
import itertools

logging.basicConfig(level=logging.ERROR)

ipv4pattern = re.compile(r'(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})')

def is_in_bounds(s:str) -> bool:
    return 0 <= int(s) <= 255

def has_leading_zeros(s:str) -> bool:
    return s != '0' and s.startswith('0')

def is_valid_ipv4(ipv4:str) -> bool:
    match = ipv4pattern.fullmatch(ipv4)
    if not match:
        return False
    groups = match.groups()
    if len(groups)!=4:
        return False
    return all( ( is_in_bounds(s) and not has_leading_zeros(s)
                        for s in match.groups() ) )

def validate_ipv4(ipv4: str, expectPass: bool = True) -> bool:
    logging.info(f"Validating IP '{ipv4}' START.")
    isvalid = is_valid_ipv4(ipv4)
    assert(isvalid == expectPass)
    if expectPass:
        logging.info(f"Validating IP '{ipv4}' SUCCESS.")
    else:
        logging.info(f"Validating IP '{ipv4}' FAILED, as expected.")

if __name__ == '__main__':
    octets =  list(range(  0,  3))+list(range(  9, 12))+list(range( 98,102))
    octets += list(range(198,202))+list(range(250,256))

    for i,j,k,l in itertools.product(octets,repeat=4):
        validate_ipv4(f'{i}.{j}.{k}.{l}')

    octets = list(range(-3,0))+list(range(256, 260))

    for i,j,k,l in itertools.product(octets,repeat=4):
        validate_ipv4(f'{i}.{j}.{k}.{l}', False)

Answer 3

assert is for debugging/testing

Don't use assert for runtime validation. Per the documentation, the assert statement assert expression is equivalent to:

if __debug__:
    if not expression: raise AssertionError

and if the interpreter is run with the -O flag, the assert statements are elided:

...the built-in variable debug is True under normal circumstances, False when optimization is requested (command line option -O). The current code generator emits no code for an assert statement when optimization is requested at compile time.

Answer 4

Return value

The function is supposed to return a boolean, but it doesn't. It returns None or an exception. To avoid confusion, decide what to return and adjust the type hint accordingly.

My suggestion is to return a boolean, as it is easier for the caller to deal with and avoids the additional argument expectFail. However, this might depend on your use case.

Logging inside the function

Unless it's for debugging, logging should be done outside of functions. This is to make functions easier to reuse. If the function validate_ipv4 returns a boolean, the caller can then log a message to the user. For example:

ipv4 = '192.0.0.1'
if validate_ipv4(ipv4):
    logging.info(f"Validating IP '{ipv4}' SUCCESS.")
else:
    logging.info(f"Validating IP '{ipv4}' FAILED.")

Validation

Using regex doesn't seem strictly necessary. An alternative is to split by . and do the validation for each octet.

Chaining generators

This part:

octets = []
octets.extend(range(0,3))
octets.extend(range(9,12))
octets.extend(range(98,102))
octets.extend(range(198,202))
octets.extend(range(250,256))

Can be done using itertools.chain:

from itertools import chain

octets = list(chain(range(3), 
                    range(9, 12), 
                    range(98, 102), 
                    range(198, 202), 
                    range(250, 256)))

Applying the suggestions:

def validate_ipv4(ipv4: str) -> bool:
    octets = ipv4.split(".")
    if len(octets) != 4:
        return False
    for octet in octets:
        if octet.startswith('0') and len(octet) > 1:
            return False
        try:
            value = int(octet)
        except ValueError:
            return False
        if value < 0 or value > 255:
            return False
    return True

Performance

I see that your concerns are mainly about "readability and simplicity". However, consider that not using regex seems to improve performances. This is the running time on my machine:

validate_ipv4 (original): 3.9611 s
validate_ipv4 (improved): 1.4649 s

Full code

import logging
import re
from itertools import chain, product
from time import perf_counter as pc

logging.basicConfig(level=logging.ERROR)

pattern = re.compile(r'^(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$')


def is_in_bounds(s):
    return int(s) >= 0 and int(s) <= 255


def is_leading_zeros(s):
    if s != '0':
        return s[0] != '0'
    return True


def validate_ipv4(ipv4: str, expectFail: bool = False) -> bool:
    logging.info(f"Validating IP '{ipv4}' START.")

    try:
        match = pattern.match(ipv4)
        assert (match)

        groups = match.groups()
        assert (len(groups) == 4)

        for s in match.groups():
            assert (is_leading_zeros(s))
            assert (is_in_bounds(s))

        logging.info(f"Validating IP '{ipv4}' SUCCESS.")

    except AssertionError:
        logging.info(f"Validating IP '{ipv4}' FAILED.", exc_info=True)
        if not expectFail:
            raise


def validate_ipv4_marc(ipv4: str) -> bool:
    octets = ipv4.split(".")
    if len(octets) != 4:
        return False
    for octet in octets:
        if octet.startswith('0') and len(octet) > 1:
            return False
        try:
            value = int(octet)
        except ValueError:
            return False
        if value < 0 or value > 255:
            return False
    return True


octets = list(chain(range(3),
                    range(9, 12),
                    range(98, 102),
                    range(198, 202),
                    range(250, 256)))

for f in validate_ipv4, validate_ipv4_marc:
    t0 = pc()
    for i, j, k, l in product(octets, repeat=4):
        f(f'{i}.{j}.{k}.{l}')
    t1 = pc()
    print(f'{f.__name__}: {round(t1 - t0, 4)} s')

Answer 5

Given that your focus is on readability and simplicity, rather than performance (which matters too), I would suggest using the netaddr module. Then all you need is something like:

import netaddr

if netaddr.valid_ipv4('192.168.0.1'):
    # do something

However, it has to be installed, this is not a built-in package. The reason why I am mentioning this is also because it is ready for IPv6, and that sooner or later you'll probably have to handle IPv6. And afaik it can also be used to manipulate MAC addresses. A regex that properly validates an IPv6 address is ugly, you don't want to be doing this.

Regexes are notoriously slow, so you may want to try a more rudimentary approach based on the feedback received, like splitting the string on the dots, then checking if each element is a valid number in the range 0-255. Although there are more steps involved the resulting code could be faster, depending on implementation, but it's something you have to try. The more elegant solution is not always the one that performs better.

But to address your code specifically, you have a hybrid validation routine that is not fully taking advantage of regexes. A regex can perfectly enforce the rule that each element should be in the range 0-255. Here's how:

\b(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\b

Courtesy of: www.regular-expressions.info

But it could be simplified with a backreference I think. If you google around you'll find plenty of alternatives, some of which may be unreliable so watch out and test extensively including the edge cases. I believe this snippet is correct though.

Yes, the regex has become more difficult to read, although this is just a single pattern which is repeated multiple times. If you focus on the repeated pattern it's not that hard to understand.

The upside is that your Python code is drastically simplified, since the regex takes care of everything, including the leading zeroes. If you will be going the regex route, exploit all the possibilities.