Preventing floods while saving logs

Question

I had a logging function…

@task
def log_request(label, target, request_as_dict):
    # Detect flood

    # In general this task do not process misuse. But it can send
    # an alarm to another celery task if something strange happened.
    if incr_with_ttl(INDICATOR_KEY, INDICATOR_TTL) >= INDICATOR_THRESHOLD:
        process_misuse.apply_async()
        redis.delete(INDICATOR_KEY)

    # Save entry
    fields = request_as_dict['fields']
    fields.update({
        "label": label,
        "type": 1 if target else 2, # 1: target, 2: extra

        "suspicion": suspicion_rating(request_as_dict),
        "raw_request": request_as_dict
    })
    RequestEntry.objects.create(**fields)

… that I rewrote using two inner functions for readability.

@task
def log_request(label, target, request_as_dict):

    def detect_flood():
        '''
        In general this task do not process misuse. But it can send
        an alarm to another celery task if something strange happened.
        '''
        if incr_with_ttl(INDICATOR_KEY, INDICATOR_TTL) >= INDICATOR_THRESHOLD:
            process_misuse.apply_async()
            redis.delete(INDICATOR_KEY)

    def save_entry():
        fields = request_as_dict['fields']
        fields.update({
            "label": label,
            "type": 1 if target else 2, # 1: target, 2: extra

            "suspicion": suspicion_rating(request_as_dict),
            "raw_request": request_as_dict
        })
        RequestEntry.objects.create(**fields)

    save_entry()
    detect_flood()

Is the rewrite an improvement?

Answer 1

You have recognized that this function is actually doing two things. Compartmentalizing the functionality, therefore, is a good idea. However, that's not the best way to go about it.

My first observation is that you call save_entry() before detect_flood(), whereas the inner functions are defined in the other order. That's misleading.

My second minor observation is that the docstring doesn't really describe what detect_flood() does. It seems like it should be more of a comment.

I would consider save_entry() to be the real work for log_request(). The flood detection code has very little to do with it. In fact, detect_flood() doesn't even use any of the parameters label, target, and request_as_dict. Therefore, the flood detection functionality would be better written as a @rate_limited decorator instead.

Answer 2

Yes this is fine.

By the single responsibility principle, and to achieve functional cohesion, a function should do one thing. As the responsibility of this function takes two logical steps, it makes sense to move those steps to their own functions. And since these functions are only used once, and so they would not be useful not desirable in the global namespace, it makes sense to hide them as inner functions.

Answer 3

When you do fields.update(), you're actually modifying request_as_dict. That is unexpected behaviour from the caller's point of view, especially since you didn't warn about it in a docstring.

A more appropriate implementation would be

@task
def log_request(label, target, request_as_dict):
    # Detect flood
    # …

    # Save entry
    RequestEntry.objects.create(**dict(
        request_as_dict['fields'],
        "label": label,
        "type": 1 if target else 2, # 1: target, 2: extra
        "suspicion": suspicion_rating(request_as_dict),
        "raw_request": request_as_dict
    ))