Fix calling delete on instances using nested ModelMocker context managers

[nfantone]

Fixes #186.

Ok, this was more involved than I anticipated.

As explained in #186, .delete() operations on model instances within the scope of more than one ModelMocker is currently broken.

with ModelMocker(Manufacturer), ModelMocker(Car, outer=False):
    car = Car.objects.create(speed=10)
    car.delete()

    manufacturer = Manufacturer.objects.create(name='foo')
    manufacturer.delete() # <-- Calls original Django delete method instead of deleting mock

The above would also fail for any model other than Car, which is the last ModelMocker in the context chain.

The underlying reason for this is the fact that a ModelMocker sets up two global or "shared" mocks for a pair of methods in django.db.models.deletion.Collector: collect and delete. They are re-used across all Django model instances to handle deletion, regardless of whether they have been mocked or not.

If declaring nested ModelMocker context managers, each one will attempt to set up the same "shared" mocks, overriding the last one. However, the logic in place today is flawed as, even though these particular mocks are supposed to be global,

• they rely on local state and MockSet objects to resolve the delete() query.
• they fallback to invoking the original Django methods if the class of the model being deleted does not match the class used to instantiate the last ModelMocker (i.e.: the innermost context manager or decorator). This is not right as .delete() may be called from any one mocked model.

In addition,

• a caller failing to pass outer=False to all mockers except the first, will cause shared patchers to be prematurely stopped for all managers and models, likely resulting in failed tests.
• ModelMocker instances are not thread-safe and the result of running tests in parallel that rely on them is undetermined.
• Their internal state is shared between unit tests and is never reset after being initialised.

In order to fix this, I initially attempted to patch the existing logic — but, while doing so and trying to understand the implementation, I realised I couldn't fully grasp the original reason why these mocks needed to be "global" in the first place. I noticed this was done +7y ago by @stphivos, so I'm sure there were some factors behind the decision. There may be something that I'm missing here — please, let me know if there is.

In the end, I opted for simplifying the whole thing:

• removed all "shared" mocks completely;
• outer=False is no longer needed (passing it now is a no-op);
• declared a local mock for the delete method;
• explicitly mocked models will rely on MockSet to resolve their queries, while any others will continue using Django as normal/expected.

By removing any shared state, nesting and mixing any number of ModelMocker managers becomes a non-issue and the result is always predictable.

[nfantone]

@stefan6419846 Apologies for the push, but I'm trying to get this over the line.

Do you think this could be reviewed? I've successfully applied changes from this branch, along with all others I've recently contributed, to my local project at work which features a large Django codebase. Having this merged would be very helpful.

[stefan6419846]

You have mentioned some possible design decisions from quite some years ago in your initial comment where you are not sure (as well as me being not sure about). Thus I hesitated to merge this for now.

[nfantone]

Thanks. Yes, I did bring up issues with an old implementation. That's why I was looking for a review — not expecting this to be immediately merged. Is it that you'd like @stphivos to take a look?

But also, if left unaddressed, it means that very basic Django operations, like delete, are not fully supported by the library.

The more I look at it, the more it seems to me that this is (and has always been) an accidental omission.

[stefan6419846]

For me no direct oddities became visible with your change. Let's merge this for now. If it turns out that we indeed need further changes on this, we can always revisit this with some follow-up changes later on.

[nfantone]

@stefan6419846 Thank you for this.

Any chance we can get this and all other new additions released soon?

[stefan6419846]

As there is not release automation for PyPI and I do not have the necessary permissions, we will have to wait for @stphivos in each case.

[nfantone]

@stphivos Would you be able to help here, please?

[nfantone]

@stphivos Ping? I could really use a new release with all recent additions.

[nfantone]

@stefan6419846 Hi. Any way you could support here? Maybe you have another way of contacting @stphivos?

Otherwise, I'll fork the repo and try publishing it myself.

[stphivos]

@nfantone apologies for the delay i've been incredibly busy lately. I will push a new release later today and also check how this can be automated with PyPI for future releases.

[nfantone]

@stphivos Thank you so much. Apologies for the push — I appreciate people are busy. It was just that changes are critical for our internal work.

Let me know if I can help out.

[stefan6419846]

@nfantone I have upload permissions for PyPI now and I am going to try getting the release done in the next hours. For the future, we might switch to trusted publishing, but this requires additional setup and I do not want to force anyone to work more than possible.

[stefan6419846]

Okay, there apparently already has been a release, at least partly. PyPI and GitHub should be in sync now.