Refactor getConfiguration to use RealmContext (Part 2)

[gh-yzou]

This is the last PR to deal with issue #1775.

This PR refactors loadTasks to take RealmContext as a parameter, and completely removes the usage of getConfiguration(@nonnull PolarisCallContext ctx, String configName).

[gh-yzou]

@dimas-b I added the change here to address your comment in the other PR #1780 (comment)

[dimas-b]

thx!

[dimas-b]

This looks functionally correct to me, but it contradicts the @Nonnull annotation on the realm context parameter now 🤷 Let's go back to testRealmContext to be aligned with annotations.

[gh-yzou]

sg! added back

[adutra]

I wonder if it wouldn't be safer to pass CallContext otherwise you could in theory pass a PolarisCallContext that does not correspond to the passed RealmContext.

[gh-yzou]

Yeah, I though about this also, then i was thinking about shall we be more consistent with the APIs in the interface, since all other APIs doesn't take CallContext, but PolarisCallContext in. @dimas-b @flyrain are you OK to just pass the whole CallContext in for this API?

[dimas-b]

PolarisCallContext and RealmContext together in the same method call do raise an alarm in my head ;)

This is what I call "API skew" - both parameters are associated with a realm, but there's no guarantee the realm ID is the same. Moreover, there's no clarity what implementations should use for getting the realm ID.

CallContext might be acceptable in the short term if the ripple effects of using it are not too large.

In general, I'd like Polaris code to get all "context" stuff injected via CDI, though.

[dimas-b]

To be clear: I'm -1 on using PolarisCallContext and RealmContext together in the same method.

+0 on using CallContext instead.

[dimas-b]

Also for context: I personally do not have a good understanding of how PolarisCallContext and CallContext were envisioned to be used initially. My comments are based on what exists in code ATM.

[collado-mike]

+1 on using CallContext. TBH, I'd be fine to replace all of the PolarisCallContext params in this interface with CallContext.

[flyrain]

+1 on using CallContext. More broadly, I'd consider consolidating CallContext and PolarisCallContext into one class. Any concerns?

[dimas-b]

I'm fine with consolidating CallContext and PolarisCallContext into one class... but I reserve the right to object when I see the specific change set for that :) Note that ATM the former is an interface, the latter a class 🤷

[gh-yzou]

After @flyrain 's change #1806, we do not need to change the interface anymore, will rebase after couple of follow up PRs

[adutra]

nit: my IDE is warning me about nullability issues, since the return types are marked @Nullable :-)

The following change removes the warnings:

    Boolean featureDefaultValue =
        configurationStore.getConfiguration(realmContext, trueByDefaultKey);
    assertThat(featureDefaultValue).isTrue();

[gh-yzou]

updated, Thanks!

[dimas-b]

Let's rebase after #1806 merges... I hope it might simplify this PR.

[gh-yzou]

@dimas-b Thanks a lot for the heads up! I am aware of that change, and I am currently helping on some of the follow up work to position the whole polaris call context is a cleaner way, once finished will rebase the change.

[gh-yzou]

@dimas-b @adutra i rebased the PR based on the recent change for merge PolarisCallContext and CallContext. This PR is now much simple, there is no need to change the interface anymore. Please take one more look of this PR.

[dimas-b]

This looks functionally correct to me, but it contradicts the @Nonnull annotation on the realm context parameter now 🤷 Let's go back to testRealmContext to be aligned with annotations.

[dimas-b]

@flyrain : over to you for review (just to be in sync)