Update ClientContext.client to allow it to be unset

[bpholt]

Fixes #502.

When I was going through this writing the ContextPlatformSuites, I noticed that there are several other fields that are documented as nullable in the Java documentation. I'm not sure whether we want to fix all these by making them Options in the public API (which would probably mean a major version bump), find another way to deal with them, or just leave it alone until someone notices it in practice.

[armanbilge]

I noticed that there are several other fields that are documented as nullable in the Java documentation. I'm not sure whether we want to fix all these by making them Options in the public API (which would probably mean a major version bump)

Yikes! I guess we should issue it and consider it for a major bump. I took a quick look at the JavaDocs but I don't see where nullability is indicated?

[armanbilge]

Because this is sealed, I think we could actually (1) leave maybeClient abstract and (2) make client final and move the default implementation here. This might be a bit less awkward since we could implement the deprecated method in terms of the replacement.

[armanbilge]

Also, I think a name like clientOption is a bit more discoverable (e.g. auto-completes on client).

[bpholt]

I thought so too, but MiMa says

[error]  * abstract method clientOption()scala.Option in class feral.lambda.ClientContext is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("feral.lambda.ClientContext.clientOption")

Is that safe to exclude (since it's sealed)?

[armanbilge]

Seems like something that should be upstreamed?

[bpholt]

Yep, that's probably right. I'll do that in a separate PR.

[armanbilge]

This feels like a lot of boilerplate (which in theory could also have implementation mistakes), is it really worth it? 😅 versus converting one or two examples and just relying on case class equality.

[bpholt]

This feels like a lot of boilerplate

Yeah, agreed. 😅 But…

Unless I'm misunderstanding you, we don't have case class equality available here, since the case classes are all private for bincompat reasons.

I tried adding extends Product to our facade traits and then doing something generic, but the names don't match between the facade and our traits, and anyway the changes started to spiral quite a bit.

I tried to encode all the nullable values into the ScalaCheck generators so that we'd see failures if we incorrectly did things like clientContext.client.installationId. I'm open to suggestions, but obviously the previous amount of coverage didn't catch this kind of issue.

[armanbilge]

Unless I'm misunderstanding you, we don't have case class equality available here, since the case classes are all private for bincompat reasons.

Well, we always have Object#equals available, and because the implementations are (private) case classes we know that there is a reasonable and correct implementation of equals. So I meant we can just rely on that.

but obviously the previous amount of coverage didn't catch this kind of issue.

No it didn't, but this new amount of coverage also wouldn't catch this kind of issue. Where "this kind of issue" is that the documentation we relied on when originally defining/testing these datatypes did not mention the possibility of nullability/undefinedness.

I tried to encode all the nullable values into the ScalaCheck generators so that we'd see failures

The use of generators lets us capture the combinatorial explosion of values that could be simultaneously nullable (to the best of our knowledge), but I don't feel convinced that the value of this kind of coverage carries its weight in generator boilerplate.

---

Meanwhile, IMO a more effective kind of coverage is to simply incorporate the linked example from #502 into our test suite.

[bpholt]

I noticed that there are several other fields that are documented as nullable in the Java documentation. I'm not sure whether we want to fix all these by making them Options in the public API (which would probably mean a major version bump)

Yikes! I guess we should issue it and consider it for a major bump. I took a quick look at the JavaDocs but I don't see where nullability is indicated?

logGroupName and logStreamName are both nullable on runtime.Context, and then there are two fields that we're missing that are both null by default: tenantId and xrayTraceId.

The JS documentation doesn't mention potential nulls or undefineds, so I guess it's trial and error there. It makes me a little nervous to be making ClientContext.client an UndefOr but not doing the same for ClientContext.env, since they're described in the same block in the docs. It might be fine though.