Support contextprovider as a method parameter #101

rameshmalla · 2024-01-08T12:01:26Z

Closes #100

Accept context provider as an argument in BaiganConfig proxy class's

bocytko · 2024-01-08T12:13:44Z

.../java/org/zalando/baigan/proxy/handler/ContextAwareConfigurationMethodInvocationHandler.java

@@ -76,7 +85,7 @@ private Class<?> getClass(final Object proxy) {
        return interfaces[0];
    }

-    private Object getConfig(final String key) {
+    private Object getConfig(final String key, final List<ContextProvider> contextProviders) {


As this is a list, I'm missing a test case for more than 1 provider to showcase what happens if there is a conflict in their selection (or how precedence on the providers works in case of matches/no matches).

Thanks for bringing this @bocytko, I would like to get some suggestions around this if we should limit the number of context provider to 1 or if there is a need to support multiple context providers.

What would be the value of supporting multiple ones? (or not)
You used a list for some reason.

You used a list for some reason.

It was just a side effect of array of args in the method Object handleInvocation(Object proxy, Method method, Object[] args) to simplify logic.

I modified the code to select only the first context provider when there is more than one, as the current ContextProvider is generic enough to support multiple variables. There is a warning message if there are more than one context provider used as method parameter.

Actually, I would allow multiple context providers. If multiple contexts are required, it could be impractical to add them all to the same context provider. For conflicts, you have the options

fail on getting config

specify behavior (e.g. take first / last)

Makes sense, I modified the code to accept multiple context and throw an exception if there are multiple contexts containing the same context key.

This reverts commit a00b7d2.

lukasniemeier-zalando · 2024-01-22T18:09:25Z

Thanks for taking a look at this, it's certainly an interesting capability for the library to explore 🚀

Looking at the requirement from a bird's eye view, also taking the discussion of #101 (comment) into account, I wonder: why use ContextProvider at all here? It seems that ContextProvider is like it is (with its convoluted cousin ContextProviderRetriever) as its trying to be independent from the individual configuration key/method.

In order to support passing context directly into the configuration method, what do you think of something like this:

@BaiganConfig
public interface ExpressToggle {
    String isEnabled(@Context String countryCode);
}

In the ContextAwareConfigurationMethodInvocationHandler we wouldn't mix the existing facilities of ContextProvider, but rather populate the context map additionally with the values coming from the annotated method parameters. This avoids library users to work with ContextProvider at all...

@rameshmalla wdyt?

rameshmalla · 2024-01-23T05:05:38Z

Thanks for taking a look at this, it's certainly an interesting capability for the library to explore 🚀

Looking at the requirement from a bird's eye view, also taking the discussion of #101 (comment) into account, I wonder: why use ContextProvider at all here? It seems that ContextProvider is like it is (with its convoluted cousin ContextProviderRetriever) as its trying to be independent from the individual configuration key/method.

In order to support passing context directly into the configuration method, what do you think of something like this:
@BaiganConfig
public interface ExpressToggle {
    String isEnabled(@Context String countryCode);
}
In the ContextAwareConfigurationMethodInvocationHandler we wouldn't mix the existing facilities of ContextProvider, but rather populate the context map additionally with the values coming from the annotated method parameters. This avoids library users to work with ContextProvider at all...

@rameshmalla wdyt?

The rationale behind using context provider is to take advantage of the flexibility of the interface, and capitalize on the pre-existing knowledge base of the context provider. Usage of annotated method arguments indeed looks like a promising option, having said that we have to assess how we could fit this into a scenario where we do not need spring support to use baigan-config.

There are some clean-ups lined up where the baigan conventional way of using context provider (using springs ContextConfiguration) could be removed and use the current approach in this PR as the only way to pass on context to evaluate conditions.

In my opinion, at this juncture, we can narrow down the scope by reusing the existing context provider. Subsequently, we can explore the viability of incorporating @Context annotations and method arguments and how they might evolve. I am confident that a clearer understanding will emerge as we proceed with the cleanup process.

lukas-c-wilhelm · 2024-01-24T14:36:38Z

Hi @rameshmalla, thanks a lot for your initiative. While previously working on Baigan, I consciously left this whole topic of context untouched intentionally, as I did not have a clear view on it. But I do agree that it requires some work. So thanks a lot for taking a stab at it.

My overall view is that the whole concept of a context and the related conditions is quite convoluted. We should first describe the purpose of it and its usage in the readme so that we are clear on it. Then we can continue to evolve it. The change proposed in this PR does actually only add complexity, without a path to remove existing complexity. Now that we can have structured types as configuration values, it is possible to build configuration with context with reasonable effort. It's a fair point to try to make this easier for library users, but we should have a clear approach to it. Or is this a misconception of mine?

rameshmalla · 2024-01-25T11:33:22Z

My overall view is that the whole concept of a context and the related conditions is quite convoluted. We should first describe the purpose of it and its usage in the readme so that we are clear on it

I completely agree with you. Indeed, the concepts of context and conditions were not adequately documented, and the implementation is not straightforward. This pull request addresses that issue. Following these changes, I plan to submit a cleanup pull request (https://github.com/rameshmalla/baigan-config/pull/1/files) that will deprecate the legacy context implementation, resulting in a simpler and more straightforward context implementation.

On the documentation front, I took the initiative to create a wiki page for baigan (https://github.com/zalando-stups/baigan-config/wiki/Baigan-Conditions). This wiki not only covers the basic usage of baigan but also provides explanations for the concepts of context and conditions

Therefore, with the wiki, the current pull request, and the upcoming cleanup pull request, we should have a reasonably clear understanding of context and conditions.

rameshmalla · 2024-01-31T09:19:01Z

@lukas-c-wilhelm @lukasniemeier-zalando @bocytko let me know if you need more details on this implementation. If it looks good, we can merge this PR and start the cleanup process of legacy code.

lukas-c-wilhelm · 2024-02-28T08:59:22Z

src/test/java/org/zalando/baigan/e2e/configs/SomeConfiguration.java

@@ -11,6 +12,7 @@ public interface SomeConfiguration {
    SomeConfigObject someConfig();
    String someValue();
    Boolean isThisTrue();
+    Boolean toggleFlag(CustomContextProvider customContextProvider,CustomContextProvider secondProvider);


Have you considered just passing a Context object with a context name and value or directly a Map? This could be varargs to pass arbitrary contexts and not having to pass any if none is desired. What is the added value of having the ContextProvider?

It would basically put the ContextProvider::getContextParam call to the user, but would also make them more flexible in how they want to generate the context.

Absolutely, and there is also an almost similar suggestion from @lukasniemeier-zalando here. The main reason for reusing ContextProvider as it is flexible enough to support multiple contexts such as below

public class WebContextProvider implements ContextProvider { private final Set<String> PARAMS = ImmutableSet.of("customer", "login_type"); @Override public String getContextParam(final String name) { if ("customer".equals(name)) { return environment.getAllowedCustomer(); } else if ("customer".equals(name)) { return environment.getType(); } else { return null; } } @Override public Set<String> getProvidedContexts() { return PARAMS; } }

and also the main reason is to keep the changes small.

Well, okay. I'm not fully sold on that point. For your example, other solutions would work equally well. However, I don't want to block this here as I agree with moving into a direction that simplifies the whole thing. If @lukasniemeier-zalando has no objections, I'm fine with this step.

lukas-c-wilhelm · 2024-02-28T08:59:56Z

Thanks for the change. Combined with the planned cleanup, it is definitely an improvement!

Overall, I wonder how useful the context feature is now that we can have structured configs (e.g. for "equal", one could do this relatively easily with a Map). Do we know of real life usage examples that could guide the evolution of this concept?

lukas-c-wilhelm · 2024-02-28T09:01:19Z

I have some comments, but none of them are blockers.

lukas-c-wilhelm · 2024-02-28T09:11:39Z

.../java/org/zalando/baigan/proxy/handler/ContextAwareConfigurationMethodInvocationHandler.java

@@ -95,6 +106,16 @@ private Object getConfig(final String key) {
            context.put(param, provider.getContextParam(param));
        }

+        if (!CollectionUtils.isEmpty(contextProviders)) {


If we later remove the old way of providing ContextProvider instances, are there cases where it becomes less convenient for the user to provide the context? E.g. are context providers as request-scoped beans supported today?

Yes, it will be a breaking change. However, based on simple github search, we've found that the usage of the context provider by our clients has been minimal due to its less-than-obvious implementation. Instead, clients are opting to add additional logic after fetching configuration values, a task that could have been easily handled by context providers.

lukas-c-wilhelm · 2024-02-29T07:36:19Z

.../java/org/zalando/baigan/proxy/handler/ContextAwareConfigurationMethodInvocationHandler.java

+            contextProviders.forEach(contextProvider -> {
+                contextProvider
+                        .getProvidedContexts()
+                        .forEach(contextParam -> {
+                            if(context.containsKey(contextParam)){
+                                throw new RuntimeException("Cannot have more than one context provider for the same context key "+contextParam);
+                            }
+                            context.put(contextParam, contextProvider.getContextParam(contextParam));
+                        });
+            });


Just noting: if we used the reflection approach suggested by @lukasniemeier-zalando , this check could be done on context creation instead of at query time, making this more efficient and fail earlier.

lukasniemeier-zalando · 2024-04-08T11:29:58Z

After some discussions with the team and @rameshmalla we are concluding that we take in this change. We will support @rameshmalla to work on the planned follow-ups, clean-ups and iterations right away. Many thanks!

lukasniemeier-zalando · 2024-04-08T11:30:03Z

👍

Support contextprovider as a method parameter

4c224a8

bocytko reviewed Jan 8, 2024

View reviewed changes

rameshmalla added 3 commits January 8, 2024 18:56

Always take the first context provider

2b4c6a8

Merge branch 'zalando-stups:main' into main

a75cc64

Merge branch 'zalando-stups:main' into main

ba31883

rameshmalla marked this pull request as ready for review January 22, 2024 09:24

rameshmalla added 2 commits January 22, 2024 14:56

Updated readme to refer to wiki

a00b7d2

Revert "Updated readme to refer to wiki"

a3f571b

This reverts commit a00b7d2.

lukas-c-wilhelm reviewed Feb 28, 2024

View reviewed changes

handle duplicate contexts

1adcf04

lukas-c-wilhelm reviewed Feb 29, 2024

View reviewed changes

lukasniemeier-zalando merged commit 269bd0e into zalando-stups:main Apr 8, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support contextprovider as a method parameter #101

Support contextprovider as a method parameter #101

rameshmalla commented Jan 8, 2024 •

edited

Loading

bocytko Jan 8, 2024

rameshmalla Jan 8, 2024

bocytko Jan 8, 2024

rameshmalla Jan 8, 2024

lukas-c-wilhelm Feb 28, 2024

rameshmalla Feb 28, 2024

lukasniemeier-zalando commented Jan 22, 2024

rameshmalla commented Jan 23, 2024 •

edited

Loading

lukas-c-wilhelm commented Jan 24, 2024

rameshmalla commented Jan 25, 2024

rameshmalla commented Jan 31, 2024

lukas-c-wilhelm Feb 28, 2024

rameshmalla Feb 28, 2024

lukas-c-wilhelm Feb 29, 2024

lukas-c-wilhelm commented Feb 28, 2024

lukas-c-wilhelm commented Feb 28, 2024

lukas-c-wilhelm Feb 28, 2024

rameshmalla Feb 28, 2024

lukas-c-wilhelm Feb 29, 2024

lukasniemeier-zalando commented Apr 8, 2024

lukasniemeier-zalando commented Apr 8, 2024

Support contextprovider as a method parameter #101

Support contextprovider as a method parameter #101

Conversation

rameshmalla commented Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukasniemeier-zalando commented Jan 22, 2024

rameshmalla commented Jan 23, 2024 • edited Loading

lukas-c-wilhelm commented Jan 24, 2024

rameshmalla commented Jan 25, 2024

rameshmalla commented Jan 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukas-c-wilhelm commented Feb 28, 2024

lukas-c-wilhelm commented Feb 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukasniemeier-zalando commented Apr 8, 2024

lukasniemeier-zalando commented Apr 8, 2024

rameshmalla commented Jan 8, 2024 •

edited

Loading

rameshmalla commented Jan 23, 2024 •

edited

Loading