Currently, prepareUpload uses a String to represent the duration for retentionPeriod
This can be quite awkward when developing in Java. If you're dynamically setting the retentionPeriod, you have to start performing string manipulation to get there, which is easy to get wrong. This problem could potentially get worse if you start to introduce other units of time aside from weeks
It also perhaps isn't the best representation of time in Java, when things such as ChronoUnit are available to users, with a String being a relatively loose data type. If a developer were to write some code using prepareUpload, it would be easy for them to accidentally pass in a different string without realising, or for the String being passed in before hand to be manipulated.
What is the Solution?
Introducing a strict Class helps to solve most of the issues above
The new class, RetentionPeriodDuration, strictly takes an amount as an Integer, and a unit of time as a ChronoUnit.
This solves the string manipulation problem; users now only have to manage integers and an enum, which is a lot simpler to manage in Java than the strings.
It solves the issue of passing in the wrong string as it uses a unique Class
After construction, the object is immutable (i.e. only getters are available), which solves the issue of the data being manipulated accidentally before being passed into prepareUpload
This solution also would make it a lot easier to validate the values for the retention period up-front (instead of just at the Notifications API) if desired in the future
It would also be possible to modify the class to allow most sensible values of ChronoUnit (e.g. Months / Years) if the days unit were to be added to the API in the future. This is because you could modify the constructor to convert the amount to days depending on the ChronoUnit (e.g. WEEKS would multiply the amount by 7).
Other Notes
This solution is completely backwards compatible. It overloads prepareUpload instead of replacing it (and for the most part uses the original function), so existing users of the function won't be affected and maintenance should be fairly simple
I've written a complete unit testing set for the new class and updated some of the existing tests to check the overload works as expected
As a result of the above, I've only moved the minor patch number. I have updated the documentation to reflect the new functionality.
Checklist
[x] I’ve used the pull request template
[x] I’ve written unit tests for these changes
[x] I’ve update the documentation (in DOCUMENTATION.md)
Local branch/PR from https://github.com/alphagov/notifications-java-client/pull/210
What problem does the pull request solve?
prepareUploaduses aStringto represent the duration forretentionPeriodretentionPeriod, you have to start performing string manipulation to get there, which is easy to get wrong. This problem could potentially get worse if you start to introduce other units of time aside from weeksChronoUnitare available to users, with aStringbeing a relatively loose data type. If a developer were to write some code using prepareUpload, it would be easy for them to accidentally pass in a different string without realising, or for the String being passed in before hand to be manipulated.What is the Solution?
RetentionPeriodDuration, strictly takes an amount as anInteger, and a unit of time as aChronoUnit.prepareUploadChronoUnit(e.g. Months / Years) if the days unit were to be added to the API in the future. This is because you could modify the constructor to convert the amount to days depending on theChronoUnit(e.g. WEEKS would multiply the amount by 7).Other Notes
prepareUploadinstead of replacing it (and for the most part uses the original function), so existing users of the function won't be affected and maintenance should be fairly simpleChecklist
DOCUMENTATION.md)src/main/resources/application.propertiespom.xml