get_pushrule::Response has incompatible PushRule type

[timokoesters]

ruma::api::client::r0::push::get_pushrule::Response takes a ruma::api::client::r0::push::PushRule, which is not compatible with any of the pushrule types defined in ruma::events::push_rules. IMO there should be an enum over all pushrule types and get_pushrule::Response returns that enum.

[jplatte]

A PR to implement this would be appreciated. It would probably make sense to move all shared push rule types into ruma_common::push at the same time (and stop re-exporting stuff from ruma_common::push in the ruma-events & ruma-client-api since we'll want people to use the re-exports from ruma::push instead).

EDIT: I implemented the above suggestion. Now the only thing left to do is define that enum in ruma-common and convert ruma-client-api to use it.

[Kinrany]

Anyone working on this one? Let me try 🙂

[Kinrany]

So right now ruma_common::push has three push rule types: PushRule, ConditionalPushRule and PatternedPushRule.

There's also ruma_client_api::push::PushRule.

And all of them refer to the same spec, correct?

ConditionalPushRule and PatternedPushRule seem to be just PushRule with extra data. Do they need to be separate types, or could they be just (PushRule, Vec<PushCondition>) and (PushRule, String)?

ruma_client_api and ruma_common seem to disagree on whether it's possible to have both conditions and a pattern.

Looking at the spec, it seems the presence of conditions and patterns depends on the rule's kind. So we could replace all the types with an enum like this:

enum PushRuleKind {
  Override { conditions },
  Content { pattern },
  Room,
  Sender,
  Underride { conditions },
}

Does this make sense?

One thing I'm not sure about is that the spec seemingly allows for any default rules to have conditions. Not sure if it's possible to have default rules other than those in the spec, but the spec does have a default content rule .m.rule.contains_user_name. Do we know whether it should be possible for rules other than override and underride to have conditions?

[jplatte]

We definitely want to remove the more general definition with optional fields and use an enum of PushRule | PatternedPushRule | ConditionalPushRule instead. The tricky part is deserialization (in particular, I can't think of what we would do if both conditions and pattern are present on a rule). Serialization could just use #[serde(untagged)].

[jplatte]

The solution here may just be to get rid of the need for an enum altogether by splitting the relevant endpoints into multiple one's? Actually we could also have sth. like that and still expose one endpoint publically I think, but explaining that here would be too much work right now (I'm on mobile). Will investigate in the coming days.

[Kinrany]

My knowledge of the context is limited, but I think having ConditionalPushRule and PatternedPushRule is a bit dangerous in that the spec might introduce rules with both later.

Is it important that underride and override use the same type? Can we use a trait for that?

[jplatte]

the spec might introduce rules with both later

Possibly. I don't see why that would be 'dangerous' though.

Is it important that underride and override use the same type? Can we use a trait for that?

I don't see why they would use different types. Not sure what you mean with the trait.

[Kinrany]

Bad wording, sorry D:

The worst possible outcome I can think of would be having to make a breaking change and refactor dependent code, without the spec itself making a breaking change.

Say, if the content rules got conditions too. We'd have to add a new ConditionalAndPatternedPushRule type, change all functions that work on content rules to use that type, and change all functions that work on conditions to also allow the new type.

That would be a bit easier if we had a separate type for each rule kind, plus ConditionalPushRule and PatternedPushRule as traits that can be implemented on any of the rule types. Changing content rules to also have conditions would be just impl ConditionalPushRule for ContentPushRule.

So not at all critical, but a nice to have. Though this could be just me overengineering if this part of the spec doesn't change all that often...

[jplatte]

I think we'll have to do some trickery to avoid breaking changes in the future either way. Even if the spec just renames something, we'd want to follow that, and they surely won't consider that a breaking change but for us it would clearly be if we mirrored the change directly.

I do feel like you're overengineering this. I am probably doing the same by thinking about splitting the GET endpoint into three, one for each kind of push rule (though that would prevent some invalid things such as returning a rule with a pattern for a kind of rule where that's illegal, and sidestep any deserialization issues... 😅).

[Kinrany]

Haha, oh well.

Is there a minimal improvement we can do now, without knowing what the best design is?

We could, as @timokoesters suggested, replace ruma_client_api::push::PushRule with an enum. This way we can at least remove some duplication and make sure these types won't become incompatible again.

Something like this?

enum ruma_client_api::push::PushRule {
  Override(ConditionalPushRule),
  Content(PatternedPushRule),
  Room(PushRule),
  Sender(PushRule),
  Underride(ConditionalPushRule),
}

But I haven't looked into how hard this would be to serialize.

Hmm, is serialization always this tricky? I wonder if it would make sense to have a separate set of simple, less strict types used only for serialization.

[jplatte]

Something like this?

I was thinking more of

enum AnyPushRule {
    Normal(PushRule),
    Patterned(PatternedPushRule),
    Conditional(ConditionalPushRule),
}

Serialization should be trivial, it should just be #[derive(Serialize)] + #[serde(untagged)]. Deserialization could also work that way, but then an object containing both pattern and conditions would get accepted as AnyPushRule::Patterned in my example or AnyPushRule::Conditional if that was declared before the Patterned variant. We should probably just error in such a case instead.

The way this would probably be implemented is by having a private catch-all struct like the current ruma_client_api::push::PushRule, and depending on which fields are Some and None, we would convert that to Normal, Patterned or Conditional, or return an error.

[jplatte]

@Kinrany Hey, it's been a while. Are you still planning to finish your work on this?

[Kinrany]

Oh yeah. I should have probably said something, sorry. Couldn't find the time last weekend, but the next one should be fine.

Still not sure about using just three enum variants. But the hardest part here is deserialization, and I have no idea what the implementation is going to look like before I try, so I have nothing to say about that for now :)

Hmm, does it block anything, or are you just asking to make sure issues don't get stale? :0

[jplatte]

But the hardest part here is deserialization, and I have no idea what the implementation is going to look like before I try, so I have nothing to say about that for now :)

I wrote this before:

The way this would probably be implemented is by having a private catch-all struct like the current ruma_client_api::push::PushRule, and depending on which fields are Some and None, we would convert that to Normal, Patterned or Conditional, or return an error.

And still think that this would probably be the easiest way to solve this.

Hmm, does it block anything, or are you just asking to make sure issues don't get stale? :0

It's not blocking anything, was just wondering whether you were stuck / not interested anymore or sth. like that.

[Kinrany]

And still think that this would probably be the easiest way to solve this.

Yep, shouldn't be too hard, just comparatively hard. I just have to see for myself what exactly this is going to look like :D

[Kinrany]

Right, so I'm not sure how exactly deserialization should work here, given that we must allow extra properties for backwards compatibility reasons.

It's impossible to deserialize AnyPushRule (from this comment) properly without knowing which variant to expect. Any PatternedPushRule might be a PushRule with an extra property defined in a later version of the spec. And for any object with both pattern and conditions we'll really have to choose the variant somehow.

Hmm, can we get rid of the three variants completely and just make these two properties optional?

Alternatively we must deserialize into different types depending on the kind of push rule we expect.

[jplatte]

Okay, it seems like you didn't understand the implementation strategy I proposed before. Let me phrase it in terms of some code:

pub enum AnyPushRule {
    // definition from your (now closed) PR
    // ...
}

// notice: not public
#[derive(Deserialize)]
struct PushRuleDeHelper {
    // definition with `Option`al properties, like the existing ruma_client_api::r0::push::PushRule
    // ...
}

impl<'de> Deserialize<'de> for AnyPushRule {
    fn deserialize<D>(deserializer: D) -> Result<AnyPushRule, D::Error>
    where
        D: Deserializer<'de>,
    {
        let rule = match PushRuleDeHelper::deserialize(deserializer)?;
        match (rule.conditions.is_some(), rule.pattern.is_some()) {
            (true, true) => Err(D::Error::custom(/* ... */)),
            (true, false) => Ok(Self::Conditional(ConditionalPushRule { actions: rule.actions,  /* ... */ })),
            // ...
        }
    }
}

@Kinrany Does that make sense? If it does, feel free to update your existing PR to it and re-open, that's probably less work than starting from scratch.

[Kinrany]

The helper struct does make sense, yeah. My confusion is about the behavior we need here, not the implementation. AFAIK the spec does not guarantee the absence of pattern and conditions in rules that are not supposed to have them currently.

So perhaps the correct thing to do is to have a struct with optional fields for deserialization (same as PushRule from ruma-client-api), and then manually check the presence of the fields later.

[jplatte]

AFAIK the spec does not guarantee the absence of pattern and conditions in rules that are not supposed to have them currently.

Well, you're right about that. Splitting the endpoint in three also messes with otherwise backwards-compatible changes. How about this then: Move the existing ruma_client_api::r0::push::PushRule to ruma_common::push::AnyPushRule and implement From<PushRule>, From<ConditionalPushRule> and From<PatternedPushRule> for AnyPushRule (and possibly the reverse TryFrom conversions). So basically what you suggested, plus some conversion code.

CC @timokoesters

[Kinrany]

Yep, will do 🙂