3
\$\begingroup\$

I am writing a crawler that is going to end up in production and I was trying to come up with a way to validate its page visits. It scrapes asp.net pages so each scraping process involves a few requests/posts/redirects etc.

What I've come up with is a validator decorator on each crawler.visit method and writing in a handful of assert that meet the "footprint" of the response I'm looking for. Part of me loves the solution and another part of me wonders if there's a better way as the decorator is going to be large and repetitive.

And I'm open to any general constructive criticisms you can offer.

Here is a simplified version of my crawler using login process as an example:

The login process requires:

  1. GET request to splash page and extract ASP FORM
  2. POST splash page with asp form from (1) and extract new form
  3. POST login page with form from (2) and login credentials

Each step results with a response with a unique set of response attributes/cookies/redirects etc.

import urls
import BaseREST
from Pages import SplashPage, LoginPage

class Crawler(BaseREST):

    def __init__(self):
        super().__init__()
        self.login()

    def login(self):
      
        splash_page = self.__visit_splash_page()

        login_page_payload = splash_page.get_base_asp_form()
        login_page = self.__visit_login_page(login_page_payload)

        login_payload = login_page.get_base_asp_form()     
        logged_in_res = self.__perform_login(login_payload)
        
        return logged_in_res
        
    @validate_page_action
    def __visit_splash_page(self):
        splash_res = self.get(urls.SPLASH_URL)
        return SplashPage(splash_res)
    
    @validate_page_action
    def __visit_login_page(self, payload):
        login_page_res = self.post(urls.SPLASH_URL, payload = payload)
        return LoginPage(login_page_res)
    
    @validate_page_action
    def __perform_login(self, payload):
        logged_in_res = self.post(urls.LOGIN_URL, payload = payload)
        return LoginPage(logged_in_res)
    
    

And here is my decorator. I'll be adding a custom errors if I continue down this path

def validate_page_action(page_action):

    def validate(*args, **kwargs):

        page = page_action(*args, **kwargs)
        res = page.response

        if page_action.__name__ == "__visit_splash_page":
            validate_splash_page(res)
        elif page_action.__name__ == "__visit_login_page":
            validate_login_page(res)
        elif page_action.__name__ == "__perform_login":
            validate_perform_login(res)
        else:
            some_unknown_page_action()

        return page
    
    def validate_splash_page(res):
        try:
            assert res.status_code == 200
            assert res.url == urls.SPLASH_URL
            assert res.request.method == 'GET'
            logger.info('SPLASH PAGE')
        except:
            logger.error('FAILED TO VISIT SPLASH PAGE')
        
    def validate_login_page(res):
        try:
            validate_redirect(res, urls.SPLASH_URL)
            assert res.status_code == 200
            assert res.url == urls.LOGIN_URL
            logger.info('LOGIN PAGE')
        except AssertionError as e:
            print(e)
            logger.error('FAILED TO VISIT LOGIN PAGE')

    def validate_perform_login(res):
        try:
            validate_redirect(res, urls.LOGIN_URL)
            assert res.status_code == 200
            assert res.url == urls.START_URL
            assert res.history[0].cookies.get_dict()['Login']
            logger.info('SUCCESSFULLY LOGGED IN')
        except AssertionError:
            logger.error('FAILED TO LOGIN')

    def validate_redirect(res, redirect_url):
        assert res.history[0].is_redirect
        assert res.history[0].request.method == "POST"
        assert res.history[0].status_code == 302
        assert res.history[0].url == redirect_url

    return validate

I'm also on the fence about where in the file structure this decorator should go. Same module as crawler? in a utils module?

\$\endgroup\$

2 Answers 2

3
\$\begingroup\$

assert

There is misuse of assert. As eloquently said elsewhere:

Asserts should be used to test conditions that should never happen. The purpose is to crash early in the case of a corrupt program state.

Exceptions should be used for errors that can conceivably happen, and you should almost always create your own Exception classes.

Don't use this in production code. And in fact, asserts are not even guaranteed to run. There are command line flags (-O) that disable them.

You are not writing a unit test here. Yes you should be doing plain, boring if else statements instead. And use plain exceptions where appropriate.

Instantiation

I would probably leave method login outside of __init__, and call it explicitly. When we instantiate a class, we may still want to add some more parameters before running it. Firing up requests right away might be inopportune sometimes. That being said, that's the only "real" function you have - for now.

Session

You could use requests.Session() if you are crawling page repeatedly on a server. This also makes it even easier to persist cookies, if need be. Oh yes, it does look like you are fetching one cookie, but in a way that is clumsy and bug-prone.

Decorator

I am not in disagreement with the idea of a decorator here. But it does nothing terribly useful. A decorator would be much more useful for other purposes, for example to implement Retry on failure, so that you can attempt to recover gracefully from crawling errors. That kind of stuff perhaps. At this stage it's not generic enough to be useful.

DRY (Don't repeat yourself)

Unfortunately there is a lot of repeated code. The job is always the same, what changes is the URL, the method and the payload and you can make it all more dynamic.

So these three functions: __visit_splash_page, __visit_login_page, __perform_login could be combined into one single function along these lines:

import requests

def __init__(self):
    super().__init__()
    self.session = requests.Session()
        
def crawl_page(self, method, url, payload):
    r = self.session.request(method=method, url=url, data=payload)
    return r

Payload is optional. The verb (GET, POST etc) can be a variable/argument too. So you do not have to use requests.get, requests.post etc. Consider this is as an example for streamlining existing code, not as a best-practice example. But you get the idea.

You could even add some optional parameters like expected status code, expected redirect URL but don't overdo it. A function should do just one thing but do it well. I find it more reasonable to have one function for crawling and another for data validation.

Exception handling

It's good that you are using logging, but you are sometimes discarding useful stack trace info like here:

except:
    logger.error('FAILED TO VISIT SPLASH PAGE')

The call has failed but you are not quite sure why. The clause is too broad here, just catch requests.exceptions.

To wrap it up, you have three quite different things to validate:

  • first, the call to requests - it could fail with a Timeout error for example -> use an exception block for this
  • the status code: you expect 200, or sometimes 301/302
  • the values returned, cookies etc

Misc

validate_redirect does not look very useful to me. When you want to make sure the login has succeed, you will look at the final URL, (for example it could end with "/dashboard"), or whether a certain cookie is present or you can also look for a specific word inside the HTML like "Logged in", or your username (use Beautiful Soup for parsing the HTML code). The redirect could have a different status code: 301, 308 and more. There can be multiple redirects too. In the end you don't really care about that. You just want to ascertain that you've landed on the right page, the how is not important.

Also, it seems to me that login, __visit_login_page, __perform_login should be combined into one single function that you could call do_login. In the background that function will perform one call to fetch the page, then a second call to POST credentials. From the POV of the developer it is more convenient to invoke a single method to log in, rather than a sequence of actions. Your class is supposed to make the job easier and hide some of the complexity. So it should be more succinct and more straightforward than using plain requests.

I'm also on the fence about where in the file structure this decorator should go. Same module as crawler? in a utils module?

I would keep it in the same module, but in a separate file. Although I am not convinced this is a terrific use case.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you for the feedback! I've since found that just about everyone is in agreement that this is assert abuse and I've refactored with custom exceptions.You're right, the validate_redirect could be completely removed. Session is not in this section of code, but the crawler does inherit it. I was on a fence here between DRY and legibility. I ended up liking how the login method is a bit more easy to follow. It's my first custom decorator so maybe I was a overzealous about a new toy. ASP.net redirects every which way, so this helps me make sure the crawler is where it thinks it is. \$\endgroup\$ Commented May 14 at 17:32
3
\$\begingroup\$

name mangling

    def login(self):      
        splash_page = self.__visit_splash_page()

Not sure why you requested _ _ dunder name mangling, and the comments + Review Context don't shed any light on that. Recommend you just go with single _ private method name. It is seldom the case that mangling is what you really want.

nested functions

Nesting makes it harder for a unit test to exercise the code you wrote. Consider breaking out the various validators so they're all at module top-level.

single decorator

And here is my decorator.

I don't notice any particular motivation for why you should have One to rule them All. Prefer individual decorators, rather than writing one giant dispatch section.

Maybe "decorator" is the right approach here, adding a single source line that starts with @. But maybe adding a single validator() call to each method would be equally effective, since the validators are focused just on input args without caring about the computed result.

diagnostic message

You (roughly) wrote:

    def validate_splash_page(res):
        try:
            assert 200 == res.status_code

If we log a failure, the very first thing a maintenance engineer will want to know is "ok, so what did we get back in the status code?" Rephrase it slightly, so we answer that question:

            assert 200 == res.status_code, res.status_code

organization

where in the file structure this decorator should go. Same module as crawler?

Yes, that sounds like a perfectly sensible place for it.

\$\endgroup\$
1
  • \$\begingroup\$ Great feedback thank you \$\endgroup\$ Commented May 14 at 19:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.