feat: rotate cache folder

[marandaneto]

📢 Type of change

• Bugfix
• New feature
• Enhancement
• Refactoring

📜 Description

feat: rotate cache folder

💡 Motivation and Context

if the cache folder is full, we don't cache them anymore, ideally, we'd delete the oldest files making room for the new ones

💚 How did you test it?

📝 Checklist

• I reviewed submitted code
• I added tests to verify changes
• All tests passing
• No breaking changes

🔮 Next steps

[bruno-garcia]

Other than a note on exists before deleting which we can discuss,

[philipphofmann]

Good job, maybe we can split up CacheStrategy as I suggested in a comment.

[philipphofmann]

m: I think we could split up this class into two classes. For me, we have two different responsibilities in here. We have some base functionality that both DiskCache and SessionCache need. And we have the sorting and deleting of the files. What about renaming CacheStrategy to something like SentryCache or BaseCache, although I'm not a fan of naming something base, ... and extracting sortFilesOldestToNewest and rotateCacheIfNeeded to a class called FileCacheStrategy or something similar. I'm a huge fan of composition over inheritance. So with the extracted class, we wouldn't have to create CustomCache in the CacheStrategyTest to be able to test the deleting and sorting of the files.

[marandaneto]

not sure if it's worth creating another extra class to achieve this, actually only we wouldn't have to create CustomCache in the CacheStrategyTest is not enough argument as this is not added to the classpath, only on tests.

I still would need a class + an interface, ideally DiskCache and SessionCache will be merged very soon (when events become purely envelopes) and this class might lose its meaning, I mean, it'll be a single class anyway

opinions @bruno-garcia ?

[bruno-garcia]

I'm also a huge fan of composition over inheritence but IMHO this is a good use of inheritance.

Also, this stuff is internal so no need to over engineer this and break it apart. If it grows in the upcoming changes, we can refactor then.

That said, IMHO LGTM

[philipphofmann]

I already stated my opinion and still with your arguments for keeping as it is I'm in favor of splitting it up, but I have no strong opinion on this as it's just a small class. I'm fine with keeping as it is. No need for further discussion IMO.

[philipphofmann]

🥇 LGTM