Skip to content
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

Use AWS.Response to handle streaming/raw/parsing #384

Merged
merged 22 commits into from
Sep 29, 2021
Merged

Conversation

omus
Copy link
Member

@omus omus commented Jun 18, 2021

The idea is to use a struct in AWS.jl that can be used to handle the automatic parsing that is currently used to turn XML/JSON into a dict while also giving the option of accessing the raw output as a string or stream without all the keywords currently needed to be specified.

Depends on:

Related:

Closes:

Update: The tests in this PR run using the deprecated behaviour. Mainly I did that here to prove this change is non-breaking. For the updated tests see #458

@omus
Copy link
Member Author

omus commented Jun 18, 2021

At the moment I've made the basic AWS.Response structure and I am using it as part of submit_request. I've also introduced a shim into the service request calls to re-introduce the legacy behaviour while I'm working through the details

@omus
Copy link
Member Author

omus commented Jun 18, 2021

bors try

bors bot added a commit that referenced this pull request Jun 18, 2021
@bors
Copy link
Contributor

bors bot commented Jun 18, 2021

try

Build failed:

src/AWSMetadata.jl Outdated Show resolved Hide resolved
src/utilities/response.jl Outdated Show resolved Hide resolved
src/utilities/response.jl Outdated Show resolved Hide resolved
@omus omus force-pushed the cv/response-struct branch from 6b4fd54 to 6b38d5b Compare June 18, 2021 17:07
@omus
Copy link
Member Author

omus commented Jun 18, 2021

I'll pick this up again next week. Lots of details to work though yet but I think overall this is looking like the right approach. There is still a question of how to make this very user friendly for newcomers

src/utilities/response.jl Outdated Show resolved Hide resolved
src/utilities/response.jl Outdated Show resolved Hide resolved
src/utilities/response.jl Outdated Show resolved Hide resolved
src/utilities/response.jl Outdated Show resolved Hide resolved
src/utilities/response.jl Show resolved Hide resolved
test/AWS.jl Outdated Show resolved Hide resolved
test/AWS.jl Outdated Show resolved Hide resolved
test/AWS.jl Outdated Show resolved Hide resolved
test/AWS.jl Outdated Show resolved Hide resolved
test/AWS.jl Outdated Show resolved Hide resolved
@omus omus force-pushed the cv/response-struct branch from 503aee8 to bb045bf Compare September 3, 2021 13:55
test/patch.jl Outdated Show resolved Hide resolved
@omus
Copy link
Member Author

omus commented Sep 3, 2021

bors try

bors bot added a commit that referenced this pull request Sep 3, 2021
@bors
Copy link
Contributor

bors bot commented Sep 3, 2021

try

Build failed:

@omus
Copy link
Member Author

omus commented Sep 3, 2021

Things feel like they are starting to align here. Overall the concept of returning a AWS.Response type seems to be the right approach: it allows us to use dispatch to process the response in several different ways and allows end users to process returned data with whatever tooling they want (#438).

The change is less breaking than I originally expected as the AWS.Response implements the dictionary interface which provides some backwards compatibility. I need to think through the various other ways things were previously being handled yet to make sure I'm not missing something though.

I'm sure I've got some rounds of debugging to do yet but the only major change I anticipate is I still may want to support passing in a user supplied IO type. The reason for this is that it allows users to write directly to disk or to some other source where as currently we're always using an IOBuffer. I also need to validate the alternate download backends are working correctly.

Update: User supplied IO instance is now supported

@omus
Copy link
Member Author

omus commented Sep 3, 2021

bors try

bors bot added a commit that referenced this pull request Sep 3, 2021
@bors
Copy link
Contributor

bors bot commented Sep 3, 2021

try

Build failed:

@omus
Copy link
Member Author

omus commented Sep 3, 2021

bors try

bors bot added a commit that referenced this pull request Sep 3, 2021
@bors
Copy link
Contributor

bors bot commented Sep 3, 2021

try

Build failed:

@omus omus force-pushed the cv/response-struct branch from d556cf6 to c027974 Compare September 3, 2021 16:39
@omus
Copy link
Member Author

omus commented Sep 3, 2021

bors try

bors bot added a commit that referenced this pull request Sep 3, 2021
@bors
Copy link
Contributor

bors bot commented Sep 3, 2021

try

Build failed:

@omus
Copy link
Member Author

omus commented Sep 3, 2021

bors try

bors bot added a commit that referenced this pull request Sep 3, 2021
@bors
Copy link
Contributor

bors bot commented Sep 3, 2021

try

Build failed:

@omus omus force-pushed the cv/response-struct branch from 77a33e1 to 5244a9e Compare September 29, 2021 20:51
@omus
Copy link
Member Author

omus commented Sep 29, 2021

Validated that #458 is working this these rebased changes: #458 (comment)

@omus
Copy link
Member Author

omus commented Sep 29, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Sep 29, 2021

@bors bors bot merged commit ca3ed0c into master Sep 29, 2021
@bors bors bot deleted the cv/response-struct branch September 29, 2021 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants