minghuaw / fe2o3-amqp

A rust implementation of the AMQP1.0 protocol based on serde and tokio.
MIT License
58 stars 7 forks source link

Implemented public `dispose` and `dispose_all` methods in Receiver #229

Closed Levyks closed 6 months ago

Levyks commented 6 months ago

This PR will make it easier to implement the approach discussed in #22 and #225 by exposing a method that does all types of disposition, instead of one for each type accept/reject/release/modify.

With this, it will be possible to have a single mpsc::channel::<(DeliveryInfo, DeliveryState)> that handles all types of disposition without having to implement a switch by ourselves, considering that, under the hood, the method called in inner will always be the same (dispose or dispose_all).

minghuaw commented 6 months ago

Thank you for the PR.

I didn't want to give user access to directly setting the DeliveryState because it contains non-terminal variants (ie. DeliveryState::Received) [1]. We could probably introduce a new type TerminalDeliveryState (a shorter name if possible would be great) that only contains four variants

// Or just name it `TerminalState`?
enum TerminalDeliveryState {
    Accepted(Accepted),
    Rejected(Rejected),
    Released(Released),
    Modified(Modified),
}

// TODO: impl trait to convert to and from DeliveryState

Then the dispose method would take only these four terminal delivery state.

pub fn dispose(&mut self, delivery_info: DeliveryInfo, term_state: TerminalDeliveryState) -> Result<(), DispositionError> { }

What do you think?

Refs:

Levyks commented 6 months ago

yeah, that makes sense, although I'm not sure if TerminalState would be a better name than TerminalDeliveryState, even if it's a bit on the long side of things, I cannot really think of a better name, but it's your call after all.

for now, I will implement this enum and commit in a bit

Levyks commented 6 months ago

wait, I've just found the Outcome enum and according to the comment it appears to be the same thing as a terminal delivery state, can't we just use that instead?

EDIT: or would that Declared variant under the transaction feature flag be a deal breaker for this use-case?

/// A terminal delivery state is also referred to as Outcome
#[derive(Debug, Clone)]
pub enum Outcome {
    /// 3.4.2 Accepted
    Accepted(Accepted),

    /// 3.4.2 Rejected
    Rejected(Rejected),

    /// 3.4.4 Released
    Released(Released),

    /// 3.4.5 Modified
    Modified(Modified),

    /// 4.5.5 Declared
    #[cfg_attr(docsrs, doc(cfg(feature = "transaction")))]
    #[cfg(feature = "transaction")]
    Declared(Declared),
}
minghuaw commented 6 months ago

EDIT: or would that Declared variant under the transaction feature flag be a deal breaker for this use-case?

Yeah. Declared shouldn't be something that user can use with a receiver. Note that the new enum probably should not be placed in fe2o3-amqp-types as it is not defined in the spec

Levyks commented 6 months ago

I've just implemented and commited it, do you have any problems with putting the enum directly on the receiver module or is that ok with you?