-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable force refresh by forceRefresh query parameter #2402
base: main
Are you sure you want to change the base?
Enable force refresh by forceRefresh query parameter #2402
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.