Skip to content
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

Fixes session idle timeout #91070

Merged
merged 15 commits into from
Feb 16, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/plugins/console/public/lib/es/es.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@
import $ from 'jquery';
import { stringify } from 'query-string';

interface SendOptions {
asSystemRequest?: boolean;
}

const esVersion: string[] = [];

export function getVersion() {
Expand All @@ -20,13 +24,19 @@ export function getContentType(body: any) {
return 'application/json';
}

export function send(method: string, path: string, data: any) {
export function send(
method: string,
path: string,
data: any,
{ asSystemRequest }: SendOptions = {}
) {
const wrappedDfd = $.Deferred();

const options: JQuery.AjaxSettings = {
url: '../api/console/proxy?' + stringify({ path, method }, { sort: false }),
headers: {
'kbn-xsrf': 'kibana',
...(asSystemRequest && { 'kbn-system-request': 'true' }),
},
data,
contentType: getContentType(data),
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/console/public/lib/mappings/mappings.js
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ function retrieveSettings(settingsKey, settingsToRetrieve) {

// Fetch autocomplete info if setting is set to true, and if user has made changes.
if (settingsToRetrieve[settingsKey] === true) {
return es.send('GET', settingKeyToPathMap[settingsKey], null);
return es.send('GET', settingKeyToPathMap[settingsKey], null, true);
} else {
const settingsPromise = new $.Deferred();
if (settingsToRetrieve[settingsKey] === false) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,20 +41,26 @@ export const createSendRequestHelpers = (): SendRequestHelpers => {

// Set up successful request helpers.
sendRequestSpy
.withArgs(successRequest.path, {
body: JSON.stringify(successRequest.body),
query: undefined,
})
.withArgs(
successRequest.path,
sinon.match({
body: JSON.stringify(successRequest.body),
query: undefined,
})
)
.resolves(successResponse);
const sendSuccessRequest = () => sendRequest({ ...successRequest });
const getSuccessResponse = () => ({ data: successResponse.data, error: null });

// Set up failed request helpers.
sendRequestSpy
.withArgs(errorRequest.path, {
body: JSON.stringify(errorRequest.body),
query: undefined,
})
.withArgs(
errorRequest.path,
sinon.match({
body: JSON.stringify(errorRequest.body),
query: undefined,
})
)
.rejects(errorResponse);
const sendErrorRequest = () => sendRequest({ ...errorRequest });
const getErrorResponse = () => ({
Expand Down
13 changes: 11 additions & 2 deletions src/plugins/es_ui_shared/public/request/send_request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ export interface SendRequestConfig {
method: 'get' | 'post' | 'put' | 'delete' | 'patch' | 'head';
query?: HttpFetchQuery;
body?: any;
/**
* If set, flags this as a "system request" to indicate that this is not a user-initiated request. For more information, see
* HttpFetchOptions#asSystemRequest.
*/
asSystemRequest?: boolean;
}

export interface SendRequestResponse<D = any, E = any> {
Expand All @@ -22,11 +27,15 @@ export interface SendRequestResponse<D = any, E = any> {

export const sendRequest = async <D = any, E = any>(
httpClient: HttpSetup,
{ path, method, body, query }: SendRequestConfig
{ path, method, body, query, asSystemRequest }: SendRequestConfig
): Promise<SendRequestResponse<D, E>> => {
try {
const stringifiedBody = typeof body === 'string' ? body : JSON.stringify(body);
const response = await httpClient[method](path, { body: stringifiedBody, query });
const response = await httpClient[method](path, {
body: stringifiedBody,
query,
asSystemRequest,
});

return {
data: response.data ? response.data : response,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,21 +123,27 @@ export const createUseRequestHelpers = (): UseRequestHelpers => {

// Set up successful request helpers.
sendRequestSpy
.withArgs(successRequest.path, {
body: JSON.stringify(successRequest.body),
query: undefined,
})
.withArgs(
successRequest.path,
sinon.match({
body: JSON.stringify(successRequest.body),
query: undefined,
})
)
.resolves(successResponse);
const setupSuccessRequest = (overrides = {}, requestTimings?: number[]) =>
setupUseRequest({ ...successRequest, ...overrides }, requestTimings);
const getSuccessResponse = () => ({ data: successResponse.data, error: null });

// Set up failed request helpers.
sendRequestSpy
.withArgs(errorRequest.path, {
body: JSON.stringify(errorRequest.body),
query: undefined,
})
.withArgs(
errorRequest.path,
sinon.match({
body: JSON.stringify(errorRequest.body),
query: undefined,
})
)
.rejects(errorResponse);
const setupErrorRequest = (overrides = {}, requestTimings?: number[]) =>
setupUseRequest({ ...errorRequest, ...overrides }, requestTimings);
Expand All @@ -152,10 +158,13 @@ export const createUseRequestHelpers = (): UseRequestHelpers => {

// Set up failed request helpers with the alternative error shape.
sendRequestSpy
.withArgs(errorWithBodyRequest.path, {
body: JSON.stringify(errorWithBodyRequest.body),
query: undefined,
})
.withArgs(
errorWithBodyRequest.path,
sinon.match({
body: JSON.stringify(errorWithBodyRequest.body),
query: undefined,
})
)
.rejects(errorWithBodyResponse);
const setupErrorWithBodyRequest = (overrides = {}) =>
setupUseRequest({ ...errorWithBodyRequest, ...overrides });
Expand Down
88 changes: 51 additions & 37 deletions src/plugins/es_ui_shared/public/request/use_request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,49 +65,59 @@ export const useRequest = <D = any, E = Error>(
/* eslint-disable-next-line react-hooks/exhaustive-deps */
}, [path, method, queryStringified, bodyStringified]);

const resendRequest = useCallback(async () => {
// If we're on an interval, this allows us to reset it if the user has manually requested the
// data, to avoid doubled-up requests.
clearPollInterval();

const requestId = ++requestCountRef.current;

// We don't clear error or data, so it's up to the consumer to decide whether to display the
// "old" error/data or loading state when a new request is in-flight.
setIsLoading(true);

const response = await sendRequest<D, E>(httpClient, requestBody);
const { data: serializedResponseData, error: responseError } = response;

const isOutdatedRequest = requestId !== requestCountRef.current;
const isUnmounted = isMounted.current === false;

// Ignore outdated or irrelevant data.
if (isOutdatedRequest || isUnmounted) {
return;
}

// Surface to consumers that at least one request has resolved.
isInitialRequestRef.current = false;
const resendRequest = useCallback(
async (asSystemRequest?: boolean) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to extract the parameter from UseRequestConfig above in the useRequest call and use it here. As we are not passing it on L120 and we don't want the consumer to change the request configuration when calling resendRequest.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the terrible diff here, all I did was 1. add an asSystemRequest parameter to the anonymous callback function, and 2. construct the request payload using this new parameter.

Actually I don't think we want to expose this in UseRequestConfig -- I see it as an implementation detail. A consumer calls the useRequest function, which schedules itself to poll in the background. Consumers should not be able to determine whether this is treated as a system request. It's background polling, so it must be treated as such.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I understand. But then when we provide that handler to the consumer should we protect it and set to false on L145, or is it overkill?

const resendRequestForConsumer = useCallback(() => {
  return resendRequest(false);
}, [resendRequest]);

return {
    isInitialRequest: isInitialRequestRef.current,
    isLoading,
    error,
    data,
    resendRequest: resendRequestForConsumer, // Gives the user the ability to manually request data
  };

Copy link
Contributor Author

@jportner jportner Feb 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, now I understand now what you were trying to say 😄

It's not something we need to protect. As in, there's no danger of other unintended consequences if a consumer happened to use this parameter.

However, we don't have any consumers that require the ability to use this when manually requesting data. With that in mind, I think we should apply your suggestion until we have a reason to expose it.

Edit: changed in 1ef7e5a.

// If we're on an interval, this allows us to reset it if the user has manually requested the
// data, to avoid doubled-up requests.
clearPollInterval();

setError(responseError);
// If there's an error, keep the data from the last request in case it's still useful to the user.
if (!responseError) {
const responseData = deserializer
? deserializer(serializedResponseData)
: serializedResponseData;
setData(responseData);
}
// Setting isLoading to false also acts as a signal for scheduling the next poll request.
setIsLoading(false);
}, [requestBody, httpClient, deserializer, clearPollInterval]);
const requestId = ++requestCountRef.current;

// We don't clear error or data, so it's up to the consumer to decide whether to display the
// "old" error/data or loading state when a new request is in-flight.
setIsLoading(true);

// Any requests that are sent in the background (without user interaction) should be flagged as "system requests". This should not be
// confused with any terminology in Elasticsearch. This is a Kibana-specific construct that allows the server to differentiate between
// user-initiated and requests "system"-initiated requests, for purposes like security features.
const requestPayload = { ...requestBody, asSystemRequest };
const response = await sendRequest<D, E>(httpClient, requestPayload);
const { data: serializedResponseData, error: responseError } = response;

const isOutdatedRequest = requestId !== requestCountRef.current;
const isUnmounted = isMounted.current === false;

// Ignore outdated or irrelevant data.
if (isOutdatedRequest || isUnmounted) {
return;
}

// Surface to consumers that at least one request has resolved.
isInitialRequestRef.current = false;

setError(responseError);
// If there's an error, keep the data from the last request in case it's still useful to the user.
if (!responseError) {
const responseData = deserializer
? deserializer(serializedResponseData)
: serializedResponseData;
setData(responseData);
}
// Setting isLoading to false also acts as a signal for scheduling the next poll request.
setIsLoading(false);
},
[requestBody, httpClient, deserializer, clearPollInterval]
);

const scheduleRequest = useCallback(() => {
// If there's a scheduled poll request, this new one will supersede it.
clearPollInterval();

if (pollIntervalMs) {
pollIntervalIdRef.current = setTimeout(resendRequest, pollIntervalMs);
pollIntervalIdRef.current = setTimeout(
() => resendRequest(true), // This is happening on an interval in the background, so we flag it as a "system request".
pollIntervalMs
);
}
}, [pollIntervalMs, resendRequest, clearPollInterval]);

Expand Down Expand Up @@ -137,11 +147,15 @@ export const useRequest = <D = any, E = Error>(
};
}, [clearPollInterval]);

const resendRequestForConsumer = useCallback(() => {
return resendRequest();
}, [resendRequest]);

return {
isInitialRequest: isInitialRequestRef.current,
isLoading,
error,
data,
resendRequest, // Gives the user the ability to manually request data
resendRequest: resendRequestForConsumer, // Gives the user the ability to manually request data
};
};
2 changes: 1 addition & 1 deletion src/plugins/saved_objects_tagging_oss/common/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,4 @@
* Side Public License, v 1.
*/

export { Tag, TagAttributes, ITagsClient } from './types';
export { Tag, TagAttributes, GetAllTagsOptions, ITagsClient } from './types';
6 changes: 5 additions & 1 deletion src/plugins/saved_objects_tagging_oss/common/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,14 @@ export interface TagAttributes {
color: string;
}

export interface GetAllTagsOptions {
asSystemRequest?: boolean;
}

export interface ITagsClient {
create(attributes: TagAttributes): Promise<Tag>;
get(id: string): Promise<Tag>;
getAll(): Promise<Tag[]>;
getAll(options?: GetAllTagsOptions): Promise<Tag[]>;
Comment on lines -25 to +29
Copy link
Contributor

@pgayvallet pgayvallet Feb 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This interface is also used by the server-side

export class TagsClient implements ITagsClient {

and I think this option doesn't make any sense on the server.

But refactoring the code to use distinct interfaces is probably going to be a pain, so I'm ok keeping these changes for now as we do want that for 7.12.

delete(id: string): Promise<void>;
update(id: string, attributes: TagAttributes): Promise<Tag>;
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export function createReporter(config: AnalyicsReporterConfig): Reporter {
async http(report) {
const response = await fetch.post('/api/ui_counters/_report', {
body: JSON.stringify({ report }),
asSystemRequest: true,
});

if (response.status !== 'ok') {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ export const getHttpClient = () => {
const createIdString = (ids) => ids.map((id) => encodeURIComponent(id)).join(',');

/* Auto Follow Pattern */
export const loadAutoFollowPatterns = () => httpClient.get(`${API_BASE_PATH}/auto_follow_patterns`);
export const loadAutoFollowPatterns = (asSystemRequest) =>
httpClient.get(`${API_BASE_PATH}/auto_follow_patterns`, { asSystemRequest });

export const getAutoFollowPattern = (id) =>
httpClient.get(`${API_BASE_PATH}/auto_follow_patterns/${encodeURIComponent(id)}`);
Expand Down Expand Up @@ -100,7 +101,8 @@ export const resumeAutoFollowPattern = (id) => {
};

/* Follower Index */
export const loadFollowerIndices = () => httpClient.get(`${API_BASE_PATH}/follower_indices`);
export const loadFollowerIndices = (asSystemRequest) =>
httpClient.get(`${API_BASE_PATH}/follower_indices`, { asSystemRequest });

export const getFollowerIndex = (id) =>
httpClient.get(`${API_BASE_PATH}/follower_indices/${encodeURIComponent(id)}`);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export const loadAutoFollowPatterns = (isUpdating = false) =>
label: t.AUTO_FOLLOW_PATTERN_LOAD,
scope,
status: isUpdating ? API_STATUS.UPDATING : API_STATUS.LOADING,
handler: async () => await loadAutoFollowPatternsRequest(),
handler: async () => await loadAutoFollowPatternsRequest(isUpdating),
});

export const getAutoFollowPattern = (id) =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export const loadFollowerIndices = (isUpdating = false) =>
label: t.FOLLOWER_INDEX_LOAD,
scope,
status: isUpdating ? API_STATUS.UPDATING : API_STATUS.LOADING,
handler: async () => await loadFollowerIndicesRequest(),
handler: async () => await loadFollowerIndicesRequest(isUpdating),
});

export const getFollowerIndex = (id) =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,8 @@ const mapDispatchToProps = (dispatch) => {
loadIndices: () => {
dispatch(loadIndices());
},
reloadIndices: (indexNames) => {
dispatch(reloadIndices(indexNames));
reloadIndices: (indexNames, options) => {
dispatch(reloadIndices(indexNames, options));
},
};
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,11 @@ export class IndexTable extends Component {
componentDidMount() {
this.props.loadIndices();
this.interval = setInterval(
() => this.props.reloadIndices(this.props.indices.map((i) => i.name)),
() =>
this.props.reloadIndices(
this.props.indices.map((i) => i.name),
{ asSystemRequest: true }
),
REFRESH_RATE_INDEX_LIST
);
const { location, filterChanged } = this.props;
Expand Down
14 changes: 12 additions & 2 deletions x-pack/plugins/index_management/public/application/services/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ import { useRequest, sendRequest } from './use_request';
import { httpService } from './http';
import { UiMetricService } from './ui_metric';

interface ReloadIndicesOptions {
asSystemRequest?: boolean;
}

// Temporary hack to provide the uiMetricService instance to this file.
// TODO: Refactor and export an ApiService instance through the app dependencies context
let uiMetricService: UiMetricService;
Expand Down Expand Up @@ -78,11 +82,17 @@ export async function loadIndices() {
return response.data ? response.data : response;
}

export async function reloadIndices(indexNames: string[]) {
export async function reloadIndices(
indexNames: string[],
{ asSystemRequest }: ReloadIndicesOptions = {}
) {
const body = JSON.stringify({
indexNames,
});
const response = await httpService.httpClient.post(`${API_BASE_PATH}/indices/reload`, { body });
const response = await httpService.httpClient.post(`${API_BASE_PATH}/indices/reload`, {
body,
asSystemRequest,
});
return response.data ? response.data : response;
}

Expand Down
Loading