-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
S3Client::GetObjectAsync doesn't give you access to the data #44
Comments
You have found one of the grueling design choices we had to make here. Initially we intended to have the async methods trigger 1 or more callbacks and didn't want users mutating other users data. Also, most of the result objects do not need non-const access to get at the underlying data. Streaming Responses however, are the exception to this rule. Since seeking the stream will by definition alter the state of the resultobject, we decided to make the interface such that you had to consciously decide, "I am going to mutate the state of this object that other users may also have access to." In this case, we decided that pushing a const_cast to the user was the lesser of all evils. |
# By Michal Wrzeszcz # Via Michał Wrzeszcz (2) and Michal Wrzeszcz (1) * commit '62d468f131f6b975b223b436e64e5b0f1f50dbb7': Cover - disable during performance testing
I just spent some hours looking through the code, expecting to find some obvious way for getting the data that I had overlooked. I also googled for examples showing how to use GetObjectAsync but didn't find any. Then I finally found this issue ticket. I can't believe that pushing a const_cast to the user is the expected way to access the data. This is poor API design in my opinion. |
* Wrap aws_http_request
The callback to S3Client::GetObjectAsync provides a const GetObjectOutcome& which can only provide const GetObjectResult which won't give you access to GetBody. Unless, I'm missing something, this means that if I use GetObjectAsync I can't get the downloaded data? const_casting the problem away seems to work for now...
The text was updated successfully, but these errors were encountered: