allowlist for copy request added #6352
Conversation
| @@ -53,12 +54,104 @@ public final class SdkPojoConversionUtils { | |||
| new HashSet<>(Arrays.asList("ChecksumSHA1", "ChecksumSHA256", "ContentMD5", "ChecksumCRC32C", "ChecksumCRC32", | |||
| "ChecksumCRC64NVME", "ContentLength")); | |||
|
|
|||
| private static final Set<String> PUT_OBJECT_TO_UPLOAD_PART_ALLOWED_FIELDS = new HashSet<>(Arrays.asList( | |||
There was a problem hiding this comment.
How do we keep these allowlists up to date over time (ie, if/when there are service model updates that add new fields)? The PutObject to UploadPart translation uses an ignore list instead of allow list - is there a reason we choose allow rather than ignore for this?
There was a problem hiding this comment.
Based on offline discussion, I think the allow list here makes sense so I'm good moving forward with it.
…ferent than the value in target pojo
| } | ||
| } | ||
|
|
||
| private static void validateRequestFields(SdkPojo sourceObject, SdkPojo targetBuilder, Set<String> allowedFields) { |
There was a problem hiding this comment.
Can we add some test cases for this?
There was a problem hiding this comment.
To preserve backward compatibility, all current fields are on the the allow list. I can do a local test by changing the model and see if it fails
|
|
||
| if (!invalidFields.isEmpty()) { | ||
| throw SdkClientException.create( | ||
| String.format("The following fields are not allowed: %s", |
There was a problem hiding this comment.
Are there currently any fields not on the allow list and do we know if any of them have different default values? IE - is there a risk of now raising an exception for cases that was previously working for customers?
There was a problem hiding this comment.
All current fields are on the allow list so there shouldn't be cases that raise exception. The existing unit test for SdkPojoConversionUtils should already covered that.
…dk-java-v2 into bole/add-allowlist-copy-upload
|
|
This pull request has been closed and the conversation has been locked. Comments on closed PRs are hard for our team to see. If you need more assistance, please open a new issue that references this one. |
Motivation and Context
Adding the field allowlist when converting PutObjectRequest and CopyObjectRequest to transfer manager request like UploadPartRequest. This is to prevent copying unexpected field to the the target request.
Modifications
Added two allowlist, which is all the current fields in PutObjectRequest and CopyObjectRequest. This is to preserve backward compatibility
Added a function validateRequestFields to validate the fields set in the original request is in the allowlist
Testing
Screenshots (if appropriate)
Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License