spring-cloud / spring-cloud-config

External configuration (server and client) for Spring Cloud
Apache License 2.0
1.95k stars 1.29k forks source link

Enable force refresh by forceRefresh query parameter #2402

Open opeco17 opened 5 months ago

opeco17 commented 5 months ago

Fixes https://github.com/spring-cloud/spring-cloud-config/issues/2401

Force refresh can be enabled when both of the following are true.

  1. allowForceRefresh is true in configuration of Config Server
  2. forceRefresh query parameter is true like http://localhost:8888/foo-default.yml?forceRefresh=true
ryanjbaxter commented 5 months ago

This would be a good enhancement for our next major release where we can introduce some breaking changes. We think it would be to explore introducing a "context object" so that next time we need to pass a parameter like this down the stack we aren't changing a bunch of method signatures to do so.

opeco17 commented 5 months ago

@ryanjbaxter

Thank you for the review. May I know which approach you prefer regarding context implementation?

Approach A (Example)

public class RequestContext {
    private static final ThreadLocal<Map<String, String>> context = new ThreadLocal<>();

    public static void setContext(Map<String, String> value) {
        context.set(value);
    }

    public static Map<String, String> getContext() {
        return context.get();
    }

    public static void clear() {
        context.remove();
    }
}
public class Service {
    public void doSomething() {
        String forceRefreshStr = RequestContext.get("forceRefresh");
        boolean forceRefresh = false;
        if (forceRefreshStr != null) {
            boolean forceRefres = Boolean.parseBoolean(forceRefreshStr);
        }
        // do something
    }
}

Approach B (Example)

public class RequestContext {
    private boolean forceRefresh;

    public RequestContext(boolean forceRefresh) {
        this.forceRefresh = forceRefresh;
    };

    public boolean getForceRefresh() {
        return forceRefresh;
    }
}
public class Service {
    public void doSomething(RequestContext ctx) {
        boolean forceRefresh = ctx.getForceRefresh();
        // do something
    }
}
ryanjbaxter commented 5 months ago

Personally I would prefer approach B, but I would remove the constructor and just use setters and getters.

opeco17 commented 5 months ago

@ryanjbaxter

I've implemented RequestContext to pass query parameters to downstream methods. When we introduce new query parameters that need to be passed to downstream methods like forceRefresh next time, we can add them to RequestContext.

opeco17 commented 5 months ago

@spencergibb

Should we add both query parameters and path parameters to RequestContext or just query parameters?

In my opinion, adding just query parameters is better because adding path parameters requires huge change. Once I've added just query parameters to RequestContext.

ryanjbaxter commented 5 months ago

I think we would want query and path parameters (name, profile, label, etc).

And I agree it is a big changes, hence why it makes sense for a major release. IMO I tink the request context change should be separated out into its own PR.

opeco17 commented 5 months ago

@ryanjbaxter

I agree with you

What about to add just query parameters to RequestContext in this PR and work on other parameters in another PR? Or it's also reasonable to remove RequestContext from this PR completely and introduce RequestContext in another PR in my opinion

I can work on another PR to complete RequestContext as quickly as possible once the direction is decided

ryanjbaxter commented 5 months ago

I would't remove RequestContext from this PR. What I would suggest is we introduce RequestContext and move the existing parameters to it in another PR and then in this PR add forceRefresh to RequestContext. We would need to merge the first PR before merging this PR.

opeco17 commented 5 months ago

@ryanjbaxter @spencergibb

I've created another PR to move the existing parameters to RequestContext. Could you please review https://github.com/spring-cloud/spring-cloud-config/pull/2408?