feat(iot-dev): Deprecate existing upload to blob APIs in favor of more granular APIs #808
Conversation
…e granular APIs Customers should be free to use whatever version of the Azure Storage SDK that they want, and to configure it however they want. Because of that, the existing file upload API has been split into the three service calls: Get SAS uri from Iot Hub, upload to Azure Storage blob using iothub provided credentials, and then notify Iot hub that a file upload has completed.
timtay-microsoft
requested review from
abhipsaMisra,
azabbasi,
barustum,
bikamani,
drwill-ms,
prmathur-microsoft and
vinagesh
as code owners
|
/azp run Java Prod |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run Java Prod |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| } | ||
|
|
||
| /** | ||
| * Update the status information in the collection, and return the new json. |
There was a problem hiding this comment.
Comment says the method return the new job. Method signature does not not return anything.
There was a problem hiding this comment.
I'm not sure why we had javadoc for a private method, so I'll just delete the whole thing
| * @param statusDescription is the description of the status code. | ||
| * @throws IllegalArgumentException if one of the parameters is null, empty, or not valid. | ||
| */ | ||
| private void updateStatus(Boolean isSuccess, Integer statusCode, String statusDescription) throws IllegalArgumentException |
There was a problem hiding this comment.
Nothing is thrown in this method.
There was a problem hiding this comment.
See above, I'll delete this javadoc
| } | ||
|
|
||
| /** | ||
| * Convert this class in a valid json. |
| } | ||
|
|
||
| /** | ||
| * @return Get the correlationId that correlates this FileUploadCompletionNotification to the earlier request to get the SAS URI |
There was a problem hiding this comment.
Seems like this description describes the API and not the return value. Maybe remove the 'Get'?
|
|
||
| /** | ||
| * Empty constructor: Used only to keep GSON happy. | ||
| */ |
There was a problem hiding this comment.
do you need a @SuppressWarnings("unused") for this constructor?
There was a problem hiding this comment.
This repo doesn't really pay much attention to warnings, so no
| * @throws IllegalArgumentException if one of the parameters is null, empty, or not valid. | ||
| */ | ||
| public FileUploadCompletionNotification(String correlationId, Boolean isSuccess) | ||
| throws IllegalArgumentException |
There was a problem hiding this comment.
is this 'throws' needed since the method doesn't throw anything?
| * @throws IllegalArgumentException if one of the parameters is null, empty, or not valid. | ||
| */ | ||
| public FileUploadCompletionNotification(String correlationId, Boolean isSuccess, Integer statusCode, String statusDescription) | ||
| throws IllegalArgumentException |
There was a problem hiding this comment.
is this 'throws' needed since the method doesn't throw anything?
There was a problem hiding this comment.
Good catch, there is nothing thrown here anymore so I'll update the method signature and the javadocs
| * @param json the json to parse. | ||
| */ | ||
| public FileUploadCompletionNotification(String json) | ||
| { |
There was a problem hiding this comment.
do we need a throws IllegalArgumentException for thie constructor?
There was a problem hiding this comment.
No, because IllegalArgumentException is a runtime exception, so java methods don't need to declare them. https://www.tutorialspoint.com/how-to-handle-the-runtime-exception-in-java
| } | ||
| } | ||
| } | ||
|
|
||
| /* Tests_SRS_FILE_UPLOAD_STATUS_21_004: [The toJson shall return a string with a json that represents the contend of the FileUploadStatusParser.] */ |
There was a problem hiding this comment.
FileUploadStatusParser -> FileUploadCompletionNotification
There was a problem hiding this comment.
Actually, I should just delete these comments. They are a relic of an out-dated practice we used to do. They aren't worth keeping around anymore
| assertFileUploadRequest(fileUploadSasUriRequest, VALID_BLOB_NAME); | ||
| } | ||
|
|
||
| /* Tests_SRS_FILE_UPLOAD_REQUEST_21_004: [The toJson shall return a string with a json that represents the contend of the FileUploadSasUriResponse.] */ |
| assertFileUploadRequest(fileUploadSasUriRequest, VALID_BLOB_NAME); | ||
| } | ||
|
|
||
| /* Tests_SRS_FILE_UPLOAD_REQUEST_21_004: [The toJson shall return a string with a json that represents the contend of the FileUploadSasUriResponse.] */ |
There was a problem hiding this comment.
FileUploadSasUriResponse -> FileUploadSasUriRequest
|
/azp run Java Prod |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run Java Prod |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run Java Prod Basic, SDL, horton-java-gate |
|
Azure Pipelines successfully started running 3 pipeline(s). |
Customers should be free to use whatever version of the Azure Storage SDK that they want, and to configure it however they want. Because of that, the existing file upload API has been split into the three service calls: Get SAS uri from Iot Hub, upload to Azure Storage blob using iothub provided credentials, and then notify Iot hub that a file upload has completed.
Same as the recent effort on C# SDK that can be seen here: Azure/azure-iot-sdk-csharp#1403