Allow RootContext to create child contexts for streams.

[dgn]

To fix #6

Without this, it's impossible (or just incredibly hard, I tried for hours) to read filter config from within an HttpContext or StreamContext. It requires users to always supply a RootContext, which adds a bit of boilerplate, but IMO is a reasonable way to connect RootContext with its children in Rust, as there's no easy way to manage global state.

I'm still very new to Rust so this might be the wrong way to do it. Let's discuss.

[PiotrSikora]

Since we know the context type, could this be turned into a single new_context function that returns various context types?

Alternatively, we could allow plugins to implement multiple traits/interfaces, in which case get_type would be an unnecessary limitation, although I don't think this could work with the current ABI...

[dgn]

if I had to choose I'd prefer the former (ie have a single new_context fn that creates whatever type), but at this point I think I like the more explicit approach more- because in the end we would have to be type-casting in the SDK again to call trait-specific fn's. but I'm open to suggestions

[PiotrSikora]

Nit: move impl Context before impl RootContext to match order for HttpBody.

[dgn]

done

[PiotrSikora]

Looking at this example, I wonder if we couldn't do something type-specific here, e.g.

proxy_wasm::set_root_context(|_| -> Box<dyn RootContext> { Box::new(HttpHeadersRoot) });
proxy_wasm::set_http_context(|root_context, context_id| -> Box<dyn HttpContext> {
    root_context.new_http_context(context_id)
});

with dispatcher taking care of resolving root_context_id to root_context before calling this callback?

[PiotrSikora]

On the second thought, that might be useless, since we cannot expose multiple interfaces right now anyway.

[PiotrSikora]

Also, could you merge/rebase on top of current master branch? Thanks!

[PiotrSikora]

Thanks! Could you also add an example showing developers how to use this feature? Maybe add a new header which value is passed via configuration to examples/http_headers.rs?

Note to self: merge this after tagging v0.1.3 (pending fix for #40).

[PiotrSikora]

Nit: move struct HttpBody; after definition of HttpBodyRoot.

[PiotrSikora]

Please add:

#[repr(u32)]
#[derive(Debug)]

(it's not used in the ABI right now, but it's going to be soon, so we might as well define it properly now)

Also, could you move it below Status?

[dgn]

done

[PiotrSikora]

One more thing. I think it would be useful if we could retain backwards compatibility, and merge this without breaking all existing plugins, even if there aren't too many in the wild right now.

One way to do this is to fallback to the standalone constructors in case root_context.get_type() returns ContextType::RootContext or None, and perhaps revert changes to examples/http_auth_random.rs to make sure that the old code path still works.

[dgn]

+1 to remain backwards-compatible, but I think we should then deprecate the old way to create contexts and remove it in 0.2.0

[dgn]

that would also allow us to remove NoopRoot in 0.2.0

[PiotrSikora]

The "old way" is fine for plugins that don't have configuration (e.g. where everything is already in the code), and the "new way" can double the code size with unnecessary boilerplate.

But it's something to consider for sure.

[PiotrSikora]

Box<dyn HttpContext> -> Option<Box<dyn HttpContext>> and return None.

[dgn]

done

[PiotrSikora]

Ditto.

[dgn]

done

[PiotrSikora]

Option<ContextType> and return None.

[PiotrSikora]

This won't be needed with Option<T>.

[dgn]

done

[PiotrSikora]

This won't be needed with Option<T>.

[dgn]

removed

[PiotrSikora]

header_content: String::new(),

but you could also use:

header_context: None,

if you change header_content into Option<String>.

[PiotrSikora]

Nit: name this HttpConfigHeaderRoot and move it before HttpConfigHeader to match style used in other examples.

[PiotrSikora]

This seems expensive, since presumably each context will own a copy of the header value.

It's fine in this example, since the value is small, but in general, wouldn't it be better to store a reference to the parent context and access configuration from there?

[PiotrSikora]

The "old way" is fine for plugins that don't have configuration (e.g. where everything is already in the code), and the "new way" can double the code size with unnecessary boilerplate.

But it's something to consider for sure.

[PiotrSikora]

Could you re-add RootContext as 0 here? Thanks.

[dgn]

Why? How would that be different from returning None?

[PiotrSikora]

I meant only adding enum value here, not using it in this PR, since the ContextType is part of the updated ABI and I wanted to avoid renumbering later... But since it's not transmitted over the ABI boundary, it probably doesn't matter too much.

[PiotrSikora]

I meant only adding enum value here, not using it in this PR, since the ContextType is part of the updated ABI and I wanted to avoid renumbering later... But since it's not transmitted over the ABI boundary, it probably doesn't matter too much.