StorageHealthCheck - problem with overwriting file_name by some backends

[Quadric]

There is an issue when some storage backend overwrites requested file_name to something else (for ex. avoiding duplicates) - and when it happens there is a problem with this backend implementation. Please look at this method on StorageHealthCheck

        storage = self.get_storage()
        # save the file
        file_name = storage.save(
            file_name, ContentFile(content=file_content)
        )
        # read the file and compare
        if not storage.exists(file_name):
            raise ServiceUnavailable('File does not exist')
        with storage.open(file_name) as f:
            if not f.read() == file_content:
                raise ServiceUnavailable('File content does not match')

Look at this line:

file_name = storage.save(
    file_name, ContentFile(content=file_content)
)

storage is allowed to overwrite the requested file_name - which is OK, and later the rest of code for this method is also OK - but since then this "file_name" may change.

Thats why this method:

def check_status(self):
        try:
            # write the file to the storage backend
            file_name = self.get_file_name()
            file_content = self.get_file_content()
            self.check_save(file_name, file_content)
            self.check_delete(file_name)
            return True
        except Exception:
            raise ServiceUnavailable('Unknown exception')

may not work OK because 'check_save' and 'check_delete' uses the "original" requested file_name - but as we've seen this "file_name" could be overwritten by check_save method - making method check_delete looking for invalid file_name.

It could be fixed by adding "return file_name" to "check_save" method, like this:

def check_save(self, file_name, file_content):
        storage = self.get_storage()
        # save the file
        file_name = storage.save(
            file_name, ContentFile(content=file_content)
        )
        # read the file and compare
        if not storage.exists(file_name):
            raise ServiceUnavailable('File does not exist')
        with storage.open(file_name) as f:
            if not f.read() == file_content:
                raise ServiceUnavailable('File content does not match')
        return file_name

And use that file name in check_status method:

def check_status(self):
        try:
            # write the file to the storage backend
            file_name = self.get_file_name()
            file_content = self.get_file_content()
            file_name = self.check_save(file_name, file_content)
            self.check_delete(file_name)
            return True
        except Exception:
            raise ServiceUnavailable('Unknown exception')

What do you think?

[codecov-io]

Codecov Report

Merging #200 into master will increase coverage by 0.07%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #200      +/-   ##
==========================================
+ Coverage   74.27%   74.35%   +0.07%     
==========================================
  Files          32       32              
  Lines         346      347       +1     
==========================================
+ Hits          257      258       +1     
  Misses         89       89

Impacted Files	Coverage Δ
health_check/storage/backends.py	90.69% <100%> (+0.22%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b532f5...e0112be. Read the comment docs.

[codingjoe]

Thanks @Quadric very good catch!

[codingjoe]

Released in 3.7.1