If the ignore cache setting is enabled, getS3fsObject still grabs from the cache for directories, since these are not objects in S3 (only files). The problem is some logic around this code was determining the conditions incorrectly, resulting in a call to S3 even for directories, which then fails. This simple patch I believe applies the logic the way it is intended. It certainly fixed my issue.
| Comment | File | Size | Author |
|---|---|---|---|
| do_not_query_s3_for_directory_info_when_cache_disabled.patch | 839 bytes | shiraz dindar |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !38changes, plain diff MR !37
Comments
Comment #2
shiraz dindarComment #3
cmlaraI thinks I saw this while working on the 4.x branch and forgot to back port it to 3.x so it sounds reasonable this might be an issue ( I have not sat down and written the truth table out to verify however)
I would personally like to keep the ignore_cache check before any others as it is the most likely check to be FALSE as it is unusual and not recommended to have the cache disabled.
This could use some tests in S3fsMetadataTest::testIgnoreCache()
Comment #4
shiraz dindarHi again, yes, so for some context please see https://www.drupal.org/project/s3fs/issues/3345094#comment-14947120.
The same applies here, meaning I wouldn't necessarily expect you to roll the patch, but thought I would share to help others.
That said, patch seems more straight ahead than the last. I looked at the logic there --
if (!empty($this->config['ignore_cache']) && !($metadata && !$metadata['dir']))-- and indeed how it ANDs with the NOTs in there actually results in, if the ignore cache setting is enabled, getS3fsObject still tries to grab directories from S3. ie. not the stated purpose as in the comment.I"m reasonably confident that I fixed this in the right way and that you'll want to get this, or a variation of this, committed, but again, not pressure as far as I'm concerned.
Thanks!
Shiraz
Comment #5
cmlaraThe request for tests additions is because this is an area of code we have had repeat issues with, I added some tests last time however if there is an issue that indicates the existing tests are not sufficient and that we could have a regression in the future if we don't update them now.
The desire to have ignore_cache as the first check is because I guesstimate somewhere above 99% of the time this method is called ignore_cache will not be enabled, we can return early based on this without an extra check for if the metadata record, its a small optimization and it allows the code to signal what the most common scenario is to future editors.
Comment #6
deimos commentedPatch solves my problem.
Thank you.
Comment #10
cmlaraLooking at this the code is nearly impossible to test in the 3.x architecture A the time I had made that request I was deep into 4.x dev where it had been massively refactored and was much easier to mock up. In hindsight I should have noticed this and provided some advice on how we could have moved this forward.
Given how many times I've personally have made mistakes with this logic I see significant value in making this section testable in 8.x.3.x. In order to do so without massive refactoring I suggest we move the determination logic to a trait where we can unit test this small portion of logic.
I've opened MR38 with this moved to a trait, and added a test.
I did a quick test with both MR37 and the previous patch inside the trait after writing the tests and both appeared to still have edge cases associated with the logic where they would either incorrectly categorized a couple of execution paths, or error out due to a missing array key.
I'm generally not great at Test Driven Design as I often do not see all the variables until I've started developing the methods(possibly because the methods are too complex) however in this case I'm thankful I wrote the test first as I will admit this took a couple of iterations to make pass.
Thoughts?
Comment #11
cmlaraShould have set NR last week.
Comment #12
cmlara