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

Generate request body struct #30

Merged
merged 7 commits into from
Nov 17, 2020
Merged

Generate request body struct #30

merged 7 commits into from
Nov 17, 2020

Conversation

rcoh
Copy link
Collaborator

@rcoh rcoh commented Nov 16, 2020

Issue #, if available: #29

Currently stacked on #28 but will be unstacked once that lands.

Description of changes:
This PR is split into 2 commits:

  1. ecba396 refactors the LibRs generator to support generating private modules
  2. c7b9c09 generates Body structure shapes for HTTP based protocols

To enable serialization & deserialization, we generate synthetic structs representing the request body. Currently, the serialization method returns a stub, however, in a subsequent PR, serde will be used to generate actual request bodies.

Full output can be viewed in the build artifact, but as an example:

#[derive(::std::clone::Clone, ::std::cmp::PartialEq, ::std::fmt::Debug)]
pub struct BatchGetItemInputBody<'a> {
    pub return_consumed_capacity: &'a Option<ReturnConsumedCapacity>,
    pub request_items: &'a Option<HashMap<String, KeysAndAttributes>>,
}

Inside the operation impl:

    fn body(&self) -> RestoreTableFromBackupInputBody {
        RestoreTableFromBackupInputBody {
            global_secondary_index_override: &self.global_secondary_index_override,
            local_secondary_index_override: &self.local_secondary_index_override,
            sse_specification_override: &self.sse_specification_override,
            target_table_name: &self.target_table_name,
            backup_arn: &self.backup_arn,
            billing_mode_override: &self.billing_mode_override,
            provisioned_throughput_override: &self.provisioned_throughput_override,
        }
    }
    pub fn build_body(&self) -> String {
        let _ = self.body();
        String::new()
    }

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Running all the tests in the same cargo workspace allows test runners to avoid recompiling dependencies every time. This allows us to drop parallel tests which seemed to be causing OOMs.
@rcoh rcoh requested a review from a team November 16, 2020 19:20
To enable serialization & deserialization, we generate synthetic structs representing the request body. Currently, the serialization method returns a stub, however, in a subsequent PR, serde will be used to generate actual request bodies.
@rcoh rcoh changed the title Russell/generate bodies Generate request body struct Nov 16, 2020
@@ -52,6 +62,27 @@ abstract class HttpProtocolGenerator(
}
}

open fun toBodyImpl(implBlockWriter: RustWriter, inputShape: StructureShape, inputBody: StructureShape?) {
if (inputBody != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if this is null?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the body is an empty string / empty vec.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(to clarify, this code isn't the final code, just a placeholder until all the serialization code lands)

Then the body-checking part of the protocol tests will be enabled

Comment on lines 78 to 82
// TODO: use serde to serialize the body
if (inputBody != null) {
write("let _ = self.body();")
}
write("String::new()")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're not going to handle this now, maybe replace this with unimplmented!()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would normally, but for testing purposes I wanted to actually call this method from the protocol tests and I want the protocol tests to keep passing. Rest assured it will be replaced with real code shortly.

}
}
}
implBlockWriter.rustBlock("pub fn build_body(&self) -> String") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that the return type should be a String. I can imagine several non-UTF-8 inputs and sources; I think that a Vec<u8> can work here?

If not a Vec<u8>, bytes::Bytes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's an interesting thought—I don't have especially strong feelings. Will probably stay away from bytes::Bytes for the moment, although I think we may end up there (or somewhere similar eventually).

My thinking today is that for protocols that are guaranteed to return UTF-8 data, we might as well indicate that. (eg. that way you know these bytes are safe to print, etc.)

Comment on lines 36 to 37
}.mapNotNull {
it.body
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you leave a comment describing what does .mapNotNull does? Is it a monadic bind?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a kotlin built in that is equivalent to map { xyz }.filter { it != null }

Base automatically changed from russell/tests-in-workspace to main November 17, 2020 16:35
@rcoh rcoh merged commit 4f327bc into main Nov 17, 2020
@rcoh rcoh deleted the russell/generate-bodies branch November 17, 2020 16:39
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.

2 participants