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

Implement simple JSON serializer #411

Merged
merged 5 commits into from
May 25, 2021
Merged

Implement simple JSON serializer #411

merged 5 commits into from
May 25, 2021

Conversation

jdisanti
Copy link
Collaborator

This adds a JSON serializer for #161. At this point, I've vetted that it meets the requirements of the code generator, although the generator is not finished yet. Posting this as its own PR to keep the reviews bite sized.

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

@jdisanti jdisanti requested a review from rcoh May 24, 2021 19:54
Copy link
Collaborator

@rcoh rcoh left a comment

Choose a reason for hiding this comment

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

LGTM!

Code looks good, just noted a few places where test coverage could be improved.

rust-runtime/smithy-json/src/serialize.rs Show resolved Hide resolved
rust-runtime/smithy-json/src/serialize.rs Show resolved Hide resolved
}

/// Writes a number `value` (already converted to string) with the given `key`.
pub fn number(&mut self, key: &str, value: &str) -> &mut Self {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I will leave this up to you, but I think we will probably want this library to handle numeric JSON conversion: https://github.com/awslabs/smithy-rs/blob/42c1f215a5f88cd58871117dee3a67fa5b0b318f/rust-runtime/smithy-types/src/lib.rs#L57

Otherwise, we'll need to handle it upstream in a bunch of different places

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, I wasn't aware of the Number type in smithy-types. That simplifies things considerably. Originally, I was thinking I would need to pull in the num crate in order to write a generic number function, or alternatively make a separate number function for each number type and have the code generator choose, but I settled for just calling to_string() everywhere this number function is called.

Will refactor.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also moved the timestamp handling code out of the code gen and into the serializers so that it's not repeated everywhere.

rust-runtime/smithy-json/src/escape.rs Show resolved Hide resolved
rust-runtime/smithy-json/src/escape.rs Show resolved Hide resolved
rust-runtime/smithy-json/src/escape.rs Show resolved Hide resolved
rust-runtime/smithy-json/src/escape.rs Show resolved Hide resolved
/// Writes a string `value` to the array without escaping it.
pub fn trusted_string(&mut self, value: &str) -> &mut Self {
self.comma_delimit();
append_string(&mut self.json, value);
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be append_trusted_string

Suggested change
append_string(&mut self.json, value);
append_trusted_string(&mut self.json, value);

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would probably renamed this string_unchecked — it's not that we trust/don't trust the string as much that we aren't going to check that it doesn't need to be escaped

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'll go with string_unchecked

let formatted = value.fmt(format);
match format {
Format::EpochSeconds => json.push_str(&formatted),
_ => append_string(json, &formatted),
Copy link
Collaborator

Choose a reason for hiding this comment

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

this can be append_trusted_string

}
Number::Float(value) => {
// If the value is NaN, Infinity, or -Infinity
if value.to_bits() & F64_EXPONENT_MASK == F64_EXPONENT_MASK {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you use value.is_nan()?

if value.to_bits() & F64_EXPONENT_MASK == F64_EXPONENT_MASK {
json.push_str("null");
} else {
let mut buffer = ryu::Buffer::new();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would add a comment that ryu::Buffer::new() and itoa::Buffer:new() are cheap. I assumed this allocated until I read the code, might as well save some future person from trying to optimize this.

In fact, itoa::write just calls itoa::Buffer::new()!

rust-runtime/smithy-json/src/serialize.rs Show resolved Hide resolved
self
}

/// Writes an Instant `value` with the given `key` and `format`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// Writes an Instant `value` with the given `key` and `format`.
/// Writes an Instant `value` to this array with `format`.

formatted.push_str("\"}");

assert_eq!(
serde_json::to_string(&TestStruct { value: s }).unwrap(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

you don't need to wrap the value in a structure here, you can call to_string on a string directly: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=dcc0f9d691e95c859419cc46cc2cc4e1

this applies here & can also simplify the float tests as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TIL. Thanks, will simplify.

started: bool,
}

impl<'a> JsonObjectWriter<'a> {
Copy link
Collaborator

@rcoh rcoh May 25, 2021

Choose a reason for hiding this comment

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

not sure how this currently plays out in codegen but I might consider:

pub struct JsonValueWriter<'a> {
  json: &'a mut String,
}

// all writing of values goes through this one place
impl JsonValueWriter {
   pub fn null(self) {
     self.push_str("null");
   }
   pub fn boolean(self, value) {
      // ...
   }
   pub fn object(self) -> JsonObjectWriter { ... }
}

// then instead of having lots of duplicated methods between array and object:
impl JsonObjectWriter<'a> {
  pub fn key(&mut self, key: &str) -> JsonValueWriter {
    // write "key": with the comma tracking etc.
    JsonValueWriter { json: self.json }
  }
}

// then the usage would look like:
let mut obj_1 = JsonObjectWriter::new(&mut output);
obj_1.key("foo").boolean(true);
let mut nested = obj_1.key("nested").object();
nested.key("asdf").string("hello!");
nested.finish();
obj_1.finish();

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 really like this idea. I'll simplify down to this in a follow up PR.

@jdisanti jdisanti merged commit 2b072e0 into smithy-lang:main May 25, 2021
@jdisanti jdisanti deleted the json-refactor branch May 25, 2021 17:07
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