Enable force refresh by forceRefresh query parameter#2402
Enable force refresh by forceRefresh query parameter#2402opeco17 wants to merge 3 commits intospring-cloud:mainfrom
Conversation
|
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. |
|
Thank you for the review. 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
}
} |
|
Personally I would prefer approach B, but I would remove the constructor and just use setters and getters. |
|
I've implemented |
spencergibb
left a comment
There was a problem hiding this comment.
Request context should include more than just force refresh. As many as possible, if not all of them
|
Should we add both query parameters and path parameters to In my opinion, adding just query parameters is better because adding path parameters requires huge change. |
ff91175 to
5da9072
Compare
|
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. |
|
I agree with you What about to add just query parameters to I can work on another PR to complete |
|
I would't remove |
|
I've created another PR to move the existing parameters to |
Fixes #2401
Force refresh can be enabled when both of the following are true.