EventAdapter implementation (#1)

[ilslv]

Part of #1

Synopsis

For now there is no way to deal with schema evolution.

Solution

Introduce es::Adapter, which allows to transform incoming Stream of events into Stream of transformed Events by defining Strategy on every VersionedEvent leaf.

Checklist

• Created PR:
  • In draft mode
  • Name contains Draft: prefix
  • Name contains issue reference
  • Has k:: labels applied
  • Has assignee
• Documentation is updated (if required)
• Tests are updated (if required)
• Changes conform code style
• CHANGELOG entry is added (if required)
• FCM (final commit message) is posted
  • and approved
• Review is completed and changes are approved
• Before merge:
  • Milestone is set
  • PR's name and description are correct and up-to-date
  • Draft: prefix is removed
  • All temporary labels are removed

[tyranron]

@ilslv we're close to a relatively perfect implementation, but to be the one, still more bikesheddings are required. Especially, in terms of context stuff.

[tyranron]

Given that there will be more examples in future, it's better to have prefix arcana-example for all of them, so arcana-example-chat would be better here.

[tyranron]

Instead of having

impl adapter::Returning for Adapter {
    type Error = Infallible;
    type Transformed = event::Chat;
}

which is quite confusing, let's desolve this to something like this:

#[derive(Clone, Copy, Debug, EventAdapter)] // let's also make alias `event::Adapter`
#[adapter(into = event::Chat)]
#[adapter(err = Infallible)] // optional, `Infallible` should be default
pub struct Adapter;

[tyranron]

Without this type alias the bound is actually shorter and clearer.

[tyranron]

This part is quite akward. As far as I see, we use this dyn AnyContext only in places where we don't really require any real context. We pay for dynamic dispatch out of nothing.

What stops us from using () or some custom EmptyContext?

[tyranron]

Many issues here goes from coherence problem, thus having impl<T: ?Sized> Borrow<T> for T in std really gives us headache here.

BUT!!!1

We do really have here the situation where our code is between user's ones, so we do have control on both ends. This allows us to introduce custom wrapper types to overcome coherence rules in a way we need, use those wrappers in our inner machinery and trait bounds, and unwrap on user's ends, so library users don't ever touch them! (We do similar trick with our event::Initial wrapper)

Let's see...

#[derive(Clone, Copy, Debug, RefCast)]
#[repr(transparent)]
pub struct Context<T: ?Sized>(T);

#[derive(Clone, Copy, Debug, RefCast)]
#[repr(transparent)]
pub struct InnerContext<T: ?Sized>(T);

impl<T: ?Sized> Borrow<()> for Context<T> {
    fn borrow(&self) -> &() {
        &()
    }
}

impl<X: ?Sized, Y: ?Sized + Borrow<X>> Borrow<InnerContext<X>> for Context<Y> {
    fn borrow(&self) -> &Inner<X> {
        RefCast::ref_cast(self.0.borrow())
    }
}

Now we have impls working in the way we need, and they don't impose any coherence problems.

The last part we need is to chage bounds like:

Ctx: Borrow<Ctx2>

to

Context<Ctx>: Borrow<Ctx2>

on one side, and correct our Strategys impls to contain either type Context = () or type Context = InnerContex<Inner::Context>.

And, also, wrapping/unwrapping on both ends.

Let's give this idea a shot!

[ilslv]

@tyranron Context<Ctx>: Borrow<Ctx2> isn't really possible because of compiler error: `?Trait` bounds are only permitted at the point where a type parameter is declared. I'll try to work around it.

[ilslv]

@tyranron

I've managed to work around this by wrapping Ctx inside Context<Ctx> in Adapter blanket impl and later working with it like Ctx. Default Strategies work just fine with it.

But there is another problem: Custom Strategy. So there's a tradeoff:

1. Require users to wrap dyn Trait in InnerContext and leave it as is in case of ().
2. Lift that requirement and leave them dyn AnyContext-like approach.

This tradeoff arises, because for option 1 we need to provide additional support on our side, that will break option 2.

[tyranron]

@ilslv

Context<Ctx>: Borrow<Ctx2> isn't really possible because of compiler error: `?Trait` bounds are only permitted at the point where a type parameter is declared.

Yeah, that's why I was (and am) insisting on using associative types. So we have the concrete type to check there from the downstream traits.

But there is another problem: Custom Strategy. So there's a tradeoff:

1. Require users to wrap dyn Trait in InnerContext and leave it as is in case of ().
2. Lift that requirement and leave them dyn AnyContext-like approach.

This tradeoff arises, because for option 1 we need to provide additional support on our side, that will break option 2.

As I've said before, it shouldn't be the case, because we can omit making users writing InnerContext in their signatures, by just using them in bounds.

Let's discuss it in a talk.

[tyranron]

@ilslv so OK, let's just use in customize either type Context = (); or type Context = Borrowed<Something>. Where the Borrowed is just renamed InnerContext.

Also, move Borrowed to a top-level spell module, so it can be imported as use arcana::spell::Borrowed;.

I've though quite a while about this and haven't come up with a better solution. We always may adjust it in future once the one appears.

[tyranron]

Also, this should be named somehow more meaningful.

[ilslv]

@tyranron new nightly release kinda broke our implementation, but I've managed to fix it.

What changed

Discussion lead to rfc: 1 and 2.

TLDR: GATs now are requiring Self: 'gat_lifetime. This isn't set in stone and is subject to change.

What broke out implementation

Adding Self: 'out to all TransformedStreams should have fix the problem, but it looks like implementation of this feature is broken for now.

Basically in addition to Self: 'out compiler requires every parameter in trait to be : 'out. This causes problems to Transformer trait and its 'ctx lifetime parameter: MRE.

Workaround for now is to split Transformer into TransformerTypes which consists only of associated types and Transformer: TransformerTypes which has only fn transform() method inside.

UPD: Issue has been resolved, so Transformer and TransformerTypes are united again.

[50U10FCA7]

@ilslv Maybe it's better to define a trait for this?

For now its impossible to generalize over the events of Event/VersionedEvent, but such thing will be useful for loading events from some storage/repo for example.

playground: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=c3c3ccffdb866045cfc9ba4a286b37d2

fork showcase: https://github.com/50U10FCA7/arcane/tree/1-EventAdapter-fork

[ilslv]

@50U10FCA7

Maybe it's better to define a trait for this?

Of course it should be hidden under a trait, just at the time there were some fundamental limitations, that wouldn't allow it.