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

Valueless keys #12

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 77 additions & 1 deletion src/v2/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ impl<'de> Deserialize<'de> for ConvertToString {
///
/// ```text
/// struct Example {
/// #[serde(deserialize_with = "deserialize_hash_or_key_value_list")]
/// #[serde(deserialize_with = "deserialize_map_or_key_value_list")]
Copy link
Owner

Choose a reason for hiding this comment

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

Good catch!

/// pub args: BTreeMap<String, RawOr<String>>,
/// }
/// ```
Expand Down Expand Up @@ -168,6 +168,82 @@ where
deserializer.deserialize_map(MapOrKeyValueListVisitor)
}

/// Like `deserialize_map_or_key_value_list`, but allowing missing values
/// (e.g. for environment variables)
pub fn deserialize_map_or_key_optional_value_list<'de, D>(
deserializer: D,
) -> Result<BTreeMap<String, Option<RawOr<String>>>, D::Error>
where
D: Deserializer<'de>,
{
/// Declare an internal visitor type to handle our input.
struct MapOrKeyOptionalValueListVisitor;

impl<'de> Visitor<'de> for MapOrKeyOptionalValueListVisitor {
type Value = BTreeMap<String, Option<RawOr<String>>>;

// We have a real map.
fn visit_map<V>(self, mut visitor: V) -> Result<Self::Value, V::Error>
where
V: MapAccess<'de>,
{
let mut map: BTreeMap<String, Option<RawOr<String>>> = BTreeMap::new();
while let Some(key) = visitor.next_key::<String>()? {
if map.contains_key(&key) {
let msg = format!("duplicate map key: {}", &key);
return Err(<V::Error as de::Error>::custom(msg));
}
let ConvertToString(val) = visitor.next_value::<ConvertToString>()?;
let raw_or_value = raw(val)
.map_err(|e| <V::Error as de::Error>::custom(format!("{}", e)))?;
map.insert(key, Some(raw_or_value));
}
Ok(map)
}

// We have a key/value list. Yuck.
fn visit_seq<V>(self, mut visitor: V) -> Result<Self::Value, V::Error>
where
V: SeqAccess<'de>,
{
lazy_static! {
// Match a key/value pair.
static ref KEY_VALUE: Regex =
Regex::new("^([^=]+)(=(.*))?$").unwrap();
}

let mut map: BTreeMap<String, Option<RawOr<String>>> = BTreeMap::new();
while let Some(key_value) = visitor.next_element::<String>()? {
let caps = KEY_VALUE.captures(&key_value).ok_or_else(|| {
let msg = format!("expected KEY[=value], got: <{}>", &key_value);
<V::Error as de::Error>::custom(msg)
})?;
let key = caps.get(1).unwrap().as_str();
let value = caps.get(3).map(|v| v.as_str());
if map.contains_key(key) {
let msg = format!("duplicate map key: {}", key);
return Err(<V::Error as de::Error>::custom(msg));
}
let optional_raw_or_value = match value.map(|value| {
raw(value.to_owned())
.map_err(|e| <V::Error as de::Error>::custom(format!("{}", e)))
}) {
None => None,
Some(x) => Some(x?),
};
map.insert(key.to_owned(), optional_raw_or_value);
}
Ok(map)
}

fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
write!(formatter, "a map or a key/[value] list")
}
}

deserializer.deserialize_map(MapOrKeyOptionalValueListVisitor)
Copy link
Owner

Choose a reason for hiding this comment

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

Just last night, I discovered in serde-rs/serde#1125 (comment) that there has been a breaking change to the serde crate, and it is no longer safe to call deserialize_map as a hint when when override both visit_map and visit_seq. This needs to be deserialize_any (and I need to fix this everywhere else in compose_yml as well). Sorry for this last minute surprise. :-(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed this, and in the other obvious places too.

}

/// Given a map, deserialize it normally. But if we have a list of string
/// values, deserialize it as a map keyed with those strings, and with
/// `Default::default()` used as the value.
Expand Down
8 changes: 5 additions & 3 deletions src/v2/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,8 @@ pub struct Service {

/// Environment variables and values to supply to the container.
#[serde(default, skip_serializing_if = "BTreeMap::is_empty",
deserialize_with = "deserialize_map_or_key_value_list")]
pub environment: BTreeMap<String, RawOr<String>>,
deserialize_with = "deserialize_map_or_key_optional_value_list")]
Copy link
Owner

Choose a reason for hiding this comment

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

Have we looked at all the other callers of deserialize_map_or_key_value_list and verified that none of them require similar conversions? I can check this if you want, but it will probably have to wait until sometime next week at least.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested all of them with docker_compose and the only one that doesn't accept valueless keys is driver_opts. All the others do.

This raises some mildly interesting questions in cage, such as what to do if io.fdy.cage.srcdir is set but null.

pub environment: BTreeMap<String, Option<RawOr<String>>>,

/// Expose a list of ports to any containers that link to us.
#[serde(default, skip_serializing_if = "Vec::is_empty")]
Expand Down Expand Up @@ -305,7 +305,9 @@ impl Service {
let mut new_env = BTreeMap::new();
for rel_path in &self.env_files {
let env_file = EnvFile::load(&base.join(&rel_path.value()?))?;
new_env.append(&mut env_file.to_environment()?);
new_env.extend(env_file.to_environment()?
.into_iter()
.map(|(k, v)| (k, Some(v))));
Copy link
Owner

@emk emk Dec 21, 2017

Choose a reason for hiding this comment

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

If environments now have type BTreeMap<String, Option<RawOr<String>>>, then we should consider updating to_environment to also return that type, instead of patching it up here. Or is something more complicated going on that we need to think about first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out env_files also accept valueless keys, so we should just handle them there and produce the same BTreeMap type.

}
new_env.append(&mut self.environment.clone());
self.environment = new_env;
Expand Down