Code review comment for lp:~blake-rouse/maas/simplestreams-file-handler-connectionwrapper

Looks like a sensible change — I even wonder how much need there is for a transaction at all in this case, but I don't see it doing any harm either. I'll have to take some of the Django mechanics on faith (and on its test coverage, obviously). It does make me wonder if we make the same provision when writing the same large object?

I'm approving, but with some notes for small things that I think need fixing.

.

Can you make it clear that _set_up is designed to be idempotent for lazy initialisation? If I had to maintain it under pressure I might miss that, I could imagine myself missing that and breaking the idempotency.

(Pedantic note #1: the docstring for _set_up uses “Setup,” the noun, as a verb.)

.

(Pedantic note #2: the leading comment for TestConnectionWrapper.make_file_for_client looks like the sentence got jumbled.)

.

In test_download_calls__get_new_connection:

        # The connection is not made until first access of the content. Read
        # the content so it called.
        b''.join(response.streaming_content)

Well commented, but mind the typo. :) Our usual way to force the full iteration is to use list():

        list(response.streaming_content)

The same snippet also occurs in test_download_connection_is_not_same_as_django_connections. Short as it is, it may be worth extracting into a function. That way it's a "thing" and you don't need to explain it in the tests, apart from the name and the docstring.

.

In test_download_connection_is_not_same_as_django_connections:

        # This is very import test. The connection that is used by the wrapper
        # cannot be the same as the connection using for all other webrequests,
        # as this will cause transactional errors to occur.

Could you add just a bit more background to those transactional errors? The "to occur" part is just filler to substitute for an actual reason IMHO: you can leave it out without losing anything — except then it becomes more obvious that something is missing.

(Pedantic note #3: I could have sworn that first sentence was written in Asia!)