fix: added PostContext handling properties and context as nested variants

[chriswk]

No description provided.

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

Package	Version	Score	Details

Scanned Manifest Files

[sighphyre]

This is a very small change in terms of code but it's quite a large and significant change in terms of where Edge goes from this point on and how that impacts the evolution of the SDK ecosytem. I have some thoughts (oh no!).

• Can we get some tests for the new behavior? The existing tests cover the legacy (and slightly insane) behavior quite nicely but the new branch in the code is not covered. It's got quite a lot of subtlety to it and I think future you and me deserve some tests to know what current you and me were thinking
• Do we need to continue with the madness of destructuring top level properties into the context's properties object? I realize we need to in the GET requests because that's existing behavior and I agree we need to maintain it in the existing POST behavior because we added it and don't know who's using it. But this is new behavior we're adding here and insofar as we know, only one client is using that and is not doing top level properties. I'd like to avoid that behavior if we can. I really regret that we added that, it's cause a huge amount of confusion for some users and I don't want to make this problem worse
• This new behavior does 3 layers of properties mapping. Once from base, once from base.context and once from base.context.properties. We should probably write down the order of preference somewhere in the docs, because someone's going to cut themselves on this
• This also adds all the base properties of the context , like userId and sessionId into the props. Is it possible we can not do that? I don't know what the ramifications of that are but I'd rather not find out

Some suggestions though. If you're willing to tighten up the new behavior going forward without impacting the old behavior, I think we can do this:

#[derive(Deserialize, Serialize, Debug, Clone)]
#[serde(rename_all = "camelCase")]
pub struct PostContext {
    pub context: Option<Context>,
    #[serde(flatten)]
    pub properties: Option<Context>,
    #[serde(flatten)]
    pub extra_properties: HashMap<String, String>,
}

impl From<PostContext> for Context {
    fn from(input: PostContext) -> Self {
        if let Some(context) = input.context {
            context
        } else {
            IncomingContext {
                context: input.properties.unwrap_or_default(),
                extra_properties: input.extra_properties,
            }
            .into()
        }
    }
}

This lets us maintain the existing legacy behavior and reuse that code path, while denying that same behavior where we don't need it. It's also lets us use the Rust type system a bit more effectively rather than falling back to a string map. Works with existing tests but I do think we need to cover the weird cases a bit more effectively.

I'd also like to suggest renaming the properties field to something like flat_context or something, just so it's clear what we expect to be contained in that mapping.

Gosh, you write 10 lines and code and suddenly your colleague has 500 lines of opinions. I'm so sorry.

[chriswk]

No need to be sorry here. I think your suggestion makes sense. Let's have a f2f sync on this, I was struggling to come up with other cases here. My one argument is that the one client we know uses this is the proxy-client, which we promise should work with just swapping from a proxy backend to the edge backend.

But I think your suggestion is less invasive than my solution. Let's see if we can get it to work?

[sighphyre]

No need to be sorry here. I think your suggestion makes sense. Let's have a f2f sync on this, I was struggling to come up with other cases here. My one argument is that the one client we know uses this is the proxy-client, which we promise should work with just swapping from a proxy backend to the edge backend.

But I think your suggestion is less invasive than my solution. Let's see if we can get it to work?

Ye! I'm excited!

[sighphyre]

Yes LGTM!