Properties of components with multiple values get dropped when converting from Calendar<'_> to Calendar

[daveterra]

When parsing a calendar with icalendar::Calendar::from_str (which I believe is the recommended method), properties with multiple values are flattened and only the last value for a given key is used.

This happens in the From<Component<'_>> impl for InnerComponent:

impl From<Component<'_>> for InnerComponent {
    fn from(component: Component) -> Self {
        Self {
            properties: component
                .properties
                .into_iter()
                .map(|p| (p.name.clone().into_owned().into(), p.into()))
                .collect(),
            components: component.components.into_iter().map(Other::from).collect(),
            multi_properties: Default::default(),
        }
    }
}

Since BTreeMap is used for InnerComponent properties only one value per key is stored. Multi_properties does not seem to be used in the case of parsing from string.

[daveterra]

I'd be happy to help with this by submitting a pull request. I did see a comment in component.rs that multi_properties should be multi-map. I've found in my brief testing that simply changing the type of properties from BTreeMap to something like multimap achieves the desired results.

This may be undesired as the spec does not allow for multiple properties of the same type in all cases. However, this would at least allow for the consumer to verify if multiple properties were used and in violation of the spec.

[hoodie]

Hi, thanks for the help, I'd be happy to get some input here!

The multimap would be a great approach, but exactly that spec compliance is the reason (IIRC) why I didn't make use of it yet. However, maybe it is not too difficult to handle the cases where multiple values are undesired different, by either rejecting subsequent values or overwriting them. I haven't thought this through entirely yet. Maybe we could also just put a check into the parser. Then only the multi-properties field would be a multimap and we decide (by knowing the spec) which values go where.

What do you think? AFAIK there are not too many props that qualify for the multi-map, maybe we can just handle those few special caes.

[daveterra]

I think that is a good approach, having the parser filter properties that "MAY occur more than once" (as the spec says) and adding those to multi-properties field instead of the properties field. A quick count shows that there are only about 12 properties that can occur more than once and they seem to be the same for each component (event, todo, etc.).

I didn't look too closely at the sub components yet, but it may be safe to ignore those for now and just have them continue to use the properties field.

I'll work on putting something together as a PR and we can see what that looks like.

[hoodie]

fixed by #82