[SYNPY-1453] Guard around modified time item in cache

[BryanFauble]

Problem:

1. In the cache if a cache value was a dictionary but it did not contain the exact key of modified_time the code would return back the parent dict - causing further code to fail that was expecting a string to be returned.

Solution:

1. Adding a guard statement in _get_cache_modified_time that verified that the data returned is going to be a string or the value for modified_time

Testing:

Added unit tests around the logic that verify the behavior if the cache map:

1. Is a dict but does not contain modified_time key
2. Is a dict but contains neither of the expected keys
3. The entry is an empty dict
4. The entry is None

[sonarcloud]

Quality Gate passed

Issues
5 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[thomasyu888]

Thanks for the quick bug fix, just a comment

[thomasyu888]

I understand how this scenario occurs, but I'm confused as to how the cache map would get into that state. I know the old cache map looked something like this, but I thought we had tested downloading files between different cache map states.

{"/Users/tyu/.synapseCache/115/134016115/SYNAPSE_TABLE_QUERY_134016115.csv": "2024-03-07T06:29:19.000Z"}

[BryanFauble]

I look around the old and new code and I couldn't really find an explanation. The only case I'm thinking is if the cache map entry looked like:

{"/Users/tyu/.synapseCache/115/134016115/SYNAPSE_TABLE_QUERY_134016115.csv": {}}

or

{"/Users/tyu/.synapseCache/115/134016115/SYNAPSE_TABLE_QUERY_134016115.csv": {"someOtherValue": 123}}

I compared the previous code from 3.1.0 to the code in the file now and I do not see anywhere we are intentionally setting the value to an empty dict, or a dict that does not contain the expected keys.

As for testing I had verified that going forward on client versions handled this gracefully.

[thomasyu888]

🔥 LGTM!