Skip to content

Commit

Permalink
Add useful Debug impl to PropertyBag and ConfigBag (#2612)
Browse files Browse the repository at this point in the history
## Motivation and Context
- it's hard to debug what properties are set, especially for layered
configuration

## Description
Add Debug Info for PropertyBag and Config bag:
```
PropertyBag { contents: ["aws_smithy_http::property_bag::test::test_extensions::MyType"] }
```

```

ConfigBag {
    layers: [
        Layer {
            name: "c",
            properties: [
                "aws_smithy_runtime_api::config_bag::Value<aws_smithy_runtime_api::config_bag::test::layered_property_bag::Prop3>",
                "aws_smithy_runtime_api::config_bag::Value<aws_smithy_runtime_api::config_bag::test::layered_property_bag::Prop4>",
            ],
        },
        Layer {
            name: "b",
            properties: [
                "aws_smithy_runtime_api::config_bag::Value<aws_smithy_runtime_api::config_bag::test::layered_property_bag::Prop3>",
                "aws_smithy_runtime_api::config_bag::Value<aws_smithy_runtime_api::config_bag::test::layered_property_bag::Prop2>",
            ],
        },
        Layer {
            name: "a",
            properties: [
                "aws_smithy_runtime_api::config_bag::Value<aws_smithy_runtime_api::config_bag::test::layered_property_bag::Prop1>",
            ],
        },
        Layer {
            name: "base",
            properties: [],
        },
    ],
}
```

There is still some work to do, but this is a start

## Testing
- [x] UT

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the
smithy-rs codegen or runtime crates
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the AWS
SDK, generated SDK code, or SDK runtime crates

----

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

---------

Co-authored-by: John DiSanti <jdisanti@amazon.com>
  • Loading branch information
rcoh and jdisanti authored Apr 26, 2023
1 parent cfd2245 commit 6af5d40
Show file tree
Hide file tree
Showing 3 changed files with 119 additions and 30 deletions.
7 changes: 7 additions & 0 deletions CHANGELOG.next.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,10 @@
# references = ["smithy-rs#920"]
# meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "client | server | all"}
# author = "rcoh"

[[smithy-rs]]
message = "The `Debug` implementation for `PropertyBag` now prints a list of the types it contains. This significantly improves debuggability."
author = "rcoh"
references = ["smithy-rs#2612"]
meta = { "breaking" = false, "tada" = false, "bug" = false }

101 changes: 74 additions & 27 deletions rust-runtime/aws-smithy-http/src/property_bag.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,38 @@
use std::any::{Any, TypeId};
use std::collections::HashMap;
use std::fmt;
use std::fmt::Debug;
use std::fmt::{Debug, Formatter};
use std::hash::{BuildHasherDefault, Hasher};
use std::ops::{Deref, DerefMut};
use std::sync::{Arc, Mutex};

type AnyMap = HashMap<TypeId, Box<dyn Any + Send + Sync>, BuildHasherDefault<IdHasher>>;
type AnyMap = HashMap<TypeId, NamedType, BuildHasherDefault<IdHasher>>;

struct NamedType {
name: &'static str,
value: Box<dyn Any + Send + Sync>,
}

impl NamedType {
fn as_mut<T: 'static>(&mut self) -> Option<&mut T> {
self.value.downcast_mut()
}

fn as_ref<T: 'static>(&self) -> Option<&T> {
self.value.downcast_ref()
}

fn assume<T: 'static>(self) -> Option<T> {
self.value.downcast().map(|t| *t).ok()
}

fn new<T: Any + Send + Sync>(value: T) -> Self {
Self {
name: std::any::type_name::<T>(),
value: Box::new(value),
}
}
}

// With TypeIds as keys, there's no need to hash them. They are already hashes
// themselves, coming from the compiler. The IdHasher just holds the u64 of
Expand Down Expand Up @@ -82,13 +108,8 @@ impl PropertyBag {
/// ```
pub fn insert<T: Send + Sync + 'static>(&mut self, val: T) -> Option<T> {
self.map
.insert(TypeId::of::<T>(), Box::new(val))
.and_then(|boxed| {
(boxed as Box<dyn Any + 'static>)
.downcast()
.ok()
.map(|boxed| *boxed)
})
.insert(TypeId::of::<T>(), NamedType::new(val))
.and_then(|val| val.assume())
}

/// Get a reference to a type previously inserted on this `PropertyBag`.
Expand All @@ -106,7 +127,16 @@ impl PropertyBag {
pub fn get<T: Send + Sync + 'static>(&self) -> Option<&T> {
self.map
.get(&TypeId::of::<T>())
.and_then(|boxed| (&**boxed as &(dyn Any + 'static)).downcast_ref())
.and_then(|val| val.as_ref())
}

/// Returns an iterator of the types contained in this PropertyBag
///
/// # Stability
/// This method is unstable and may be removed or changed in a future release. The exact
/// format of the returned types may also change.
pub fn contents(&self) -> impl Iterator<Item = &'static str> + '_ {
self.map.values().map(|tpe| tpe.name)
}

/// Get a mutable reference to a type previously inserted on this `PropertyBag`.
Expand All @@ -124,7 +154,7 @@ impl PropertyBag {
pub fn get_mut<T: Send + Sync + 'static>(&mut self) -> Option<&mut T> {
self.map
.get_mut(&TypeId::of::<T>())
.and_then(|boxed| (&mut **boxed as &mut (dyn Any + 'static)).downcast_mut())
.map(|val| val.as_mut().expect("type mismatch!"))
}

/// Remove a type from this `PropertyBag`.
Expand All @@ -141,8 +171,8 @@ impl PropertyBag {
/// assert!(props.get::<i32>().is_none());
/// ```
pub fn remove<T: Send + Sync + 'static>(&mut self) -> Option<T> {
self.map.remove(&TypeId::of::<T>()).and_then(|boxed| {
(boxed as Box<dyn Any + 'static>)
self.map.remove(&TypeId::of::<T>()).and_then(|tpe| {
(tpe.value as Box<dyn Any + 'static>)
.downcast()
.ok()
.map(|boxed| *boxed)
Expand All @@ -168,7 +198,16 @@ impl PropertyBag {

impl fmt::Debug for PropertyBag {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.debug_struct("PropertyBag").finish()
let mut fmt = f.debug_struct("PropertyBag");

struct Contents<'a>(&'a PropertyBag);
impl<'a> Debug for Contents<'a> {
fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
f.debug_list().entries(self.0.contents()).finish()
}
}
fmt.field("contents", &Contents(self));
fmt.finish()
}
}

Expand Down Expand Up @@ -225,22 +264,30 @@ impl From<PropertyBag> for SharedPropertyBag {
}

#[cfg(test)]
#[test]
fn test_extensions() {
#[derive(Debug, PartialEq)]
struct MyType(i32);
mod test {
use crate::property_bag::PropertyBag;

let mut extensions = PropertyBag::new();
#[test]
fn test_extensions() {
#[derive(Debug, PartialEq)]
struct MyType(i32);

extensions.insert(5i32);
extensions.insert(MyType(10));
let mut property_bag = PropertyBag::new();

assert_eq!(extensions.get(), Some(&5i32));
assert_eq!(extensions.get_mut(), Some(&mut 5i32));
property_bag.insert(5i32);
property_bag.insert(MyType(10));

assert_eq!(extensions.remove::<i32>(), Some(5i32));
assert!(extensions.get::<i32>().is_none());
assert_eq!(property_bag.get(), Some(&5i32));
assert_eq!(property_bag.get_mut(), Some(&mut 5i32));

assert_eq!(extensions.get::<bool>(), None);
assert_eq!(extensions.get(), Some(&MyType(10)));
assert_eq!(property_bag.remove::<i32>(), Some(5i32));
assert!(property_bag.get::<i32>().is_none());

assert_eq!(property_bag.get::<bool>(), None);
assert_eq!(property_bag.get(), Some(&MyType(10)));
assert_eq!(
format!("{:?}", property_bag),
r#"PropertyBag { contents: ["aws_smithy_http::property_bag::test::test_extensions::MyType"] }"#
);
}
}
41 changes: 38 additions & 3 deletions rust-runtime/aws-smithy-runtime-api/src/config_bag.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,40 @@
//! 1. A new layer of configuration may be applied onto an existing configuration structure without modifying it or taking ownership.
//! 2. No lifetime shenanigans to deal with
use aws_smithy_http::property_bag::PropertyBag;
use std::fmt::Debug;
use std::fmt::{Debug, Formatter};
use std::ops::Deref;
use std::sync::Arc;

/// Layered Configuration Structure
///
/// [`ConfigBag`] is the "unlocked" form of the bag. Only the top layer of the bag may be unlocked.
#[must_use]
#[derive(Debug)]
pub struct ConfigBag {
head: Layer,
tail: Option<FrozenConfigBag>,
}

impl Debug for ConfigBag {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
struct Layers<'a>(&'a ConfigBag);
impl Debug for Layers<'_> {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
let mut list = f.debug_list();
list.entry(&self.0.head);
let mut us = self.0.tail.as_ref();
while let Some(bag) = us {
list.entry(&bag.head);
us = bag.tail.as_ref()
}
list.finish()
}
}
f.debug_struct("ConfigBag")
.field("layers", &Layers(self))
.finish()
}
}

/// Layered Configuration Structure
///
/// [`FrozenConfigBag`] is the "locked" form of the bag.
Expand Down Expand Up @@ -55,12 +75,26 @@ enum Value<T> {
ExplicitlyUnset,
}

#[derive(Debug)]
struct Layer {
name: &'static str,
props: PropertyBag,
}

impl Debug for Layer {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
struct Contents<'a>(&'a Layer);
impl Debug for Contents<'_> {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
f.debug_list().entries(self.0.props.contents()).finish()
}
}
f.debug_struct("Layer")
.field("name", &self.name)
.field("properties", &Contents(self))
.finish()
}
}

fn no_op(_: &mut ConfigBag) {}

impl FrozenConfigBag {
Expand Down Expand Up @@ -258,6 +292,7 @@ mod test {
assert!(final_bag.get::<Prop2>().is_some());
// we unset prop3
assert!(final_bag.get::<Prop3>().is_none());
println!("{:#?}", final_bag);
}

#[test]
Expand Down

0 comments on commit 6af5d40

Please sign in to comment.