Common Builder for Default & Async #63
Conversation
src/main/java/software/amazon/encryption/s3/materials/KmsKeyring.java
Outdated
Show resolved
Hide resolved
src/main/java/software/amazon/encryption/s3/materials/KmsKeyring.java
Outdated
Show resolved
Hide resolved
src/main/java/software/amazon/encryption/s3/materials/KmsKeyring.java
Outdated
Show resolved
Hide resolved
| return haveOneNonNull; | ||
| } | ||
|
|
||
| static CryptographicMaterialsManager checkCMM(CryptographicMaterialsManager _cryptoMaterialsManager, Keyring _keyring, SecretKey _aesKey, PartialRsaKeyPair _rsaKeyPair, String _kmsKeyId, boolean _enableLegacyUnauthenticatedModes, SecureRandom _secureRandom, Provider _cryptoProvider) { |
There was a problem hiding this comment.
Hmmmmmmmmmmmm.........
I like that this reduces duplication a lot...but I don't like that it's a method with like 8 parameters...
I believe that there is a cleaner solution available, but I am not sure what it is right now...I'm thinking something like how the keyring's builder(s) work(s), where all the common stuff between sync and async exists in a base class which sync/async extend from, so we can use inheritance instead of composition (current approach) and not have to move around so many fields.
I do think that this change moves us in the right direction, and I don't think now is the right time to spend awhile figuring out a better solution, so I won't propose any changes. But I don't think this is what the final code should look like..
For now, I will say we should at least rename this to buildCMM.
| if (onlyOneNonNull(_cryptoMaterialsManager, _keyring, _aesKey, _rsaKeyPair, _kmsKeyId)) { | ||
| if (S3EncryptionClientUtilities.onlyOneNonNull(_cryptoMaterialsManager, _keyring, _aesKey, _rsaKeyPair, _kmsKeyId)) { |
There was a problem hiding this comment.
nit - since I think we need to refactor again, don't worry about doing this now, unless you want to, but we might as well move checkKeyOptions() itself into the util class (or at least move the exception throw inside)
There was a problem hiding this comment.
I tried moving checkKeyOptions() but there are _cryptoMaterialsManager etc. which are required. Moving exception will be good, but I thought it does not reduce much code.
imabhichow
changed the title
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.