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

Add body validation to update follower index API endpoint. #63653

Merged
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
Original file line number Diff line number Diff line change
Expand Up @@ -145,9 +145,35 @@ export const updateFollowerIndex = (id, followerIndex) => {
if (isUsingAdvancedSettings) {
uiMetrics.push(UIM_FOLLOWER_INDEX_USE_ADVANCED_OPTIONS);
}

const {
maxReadRequestOperationCount,
maxOutstandingReadRequests,
maxReadRequestSize,
maxWriteRequestOperationCount,
maxWriteRequestSize,
maxOutstandingWriteRequests,
maxWriteBufferCount,
maxWriteBufferSize,
maxRetryDelay,
readPollTimeout,
} = followerIndex;

const request = httpClient.put(`${API_BASE_PATH}/follower_indices/${encodeURIComponent(id)}`, {
body: JSON.stringify(followerIndex),
body: JSON.stringify({
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: the stringify is not needed anymore as the httpClient takes care of it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, do you know whether this is also the case for public side httpClient? I seem to recall the JSON.stringify being required there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just did a test and confirmed we still need to stringify the body.

maxReadRequestOperationCount,
maxOutstandingReadRequests,
maxReadRequestSize,
maxWriteRequestOperationCount,
maxWriteRequestSize,
maxOutstandingWriteRequests,
maxWriteBufferCount,
maxWriteBufferSize,
maxRetryDelay,
readPollTimeout,
}),
});

return trackUserRequest(request, uiMetrics);
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,18 @@ export const registerFollowerIndexRoutes = ({ router, __LEGACY }: RouteDependenc
path: `${API_BASE_PATH}/follower_indices/{id}`,
validate: {
params: schema.object({ id: schema.string() }),
body: schema.object({
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to declare the schema outside the route handler so we can re-use and compose it for the create route.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've done this in #62890, so I'm going to leave it out of this PR.

maxReadRequestOperationCount: schema.maybe(schema.number()),
maxOutstandingReadRequests: schema.maybe(schema.number()),
maxReadRequestSize: schema.maybe(schema.string()), // byte value
maxWriteRequestOperationCount: schema.maybe(schema.number()),
maxWriteRequestSize: schema.maybe(schema.string()), // byte value
maxOutstandingWriteRequests: schema.maybe(schema.number()),
maxWriteBufferCount: schema.maybe(schema.number()),
maxWriteBufferSize: schema.maybe(schema.string()), // byte value
maxRetryDelay: schema.maybe(schema.string()), // time value
readPollTimeout: schema.maybe(schema.string()), // time value
}),
},
},
licensePreRoutingFactory({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,45 @@ export const registerHelpers = supertest => {

const loadFollowerIndices = () => supertest.get(`${API_BASE_PATH}/follower_indices`);

const getFollowerIndex = name => supertest.get(`${API_BASE_PATH}/follower_indices/${name}`);
const getFollowerIndex = (name, waitUntilIsActive = false) => {
const maxRetries = 10;
const delayBetweenRetries = 500;
let retryCount = 0;

const proceed = async () => {
const response = await supertest.get(`${API_BASE_PATH}/follower_indices/${name}`);

if (waitUntilIsActive && response.body.status !== 'active') {
retryCount += 1;

if (retryCount > maxRetries) {
throw new Error('Error waiting for follower index to be active.');
}

return new Promise(resolve => setTimeout(resolve, delayBetweenRetries)).then(proceed);
}

return response;
};

return {
expect: status =>
new Promise((resolve, reject) =>
proceed()
.then(response => {
if (status !== response.status) {
reject(new Error(`Expected status ${status} but got ${response.status}`));
}
return resolve(response);
})
.catch(reject)
),
then: (resolve, reject) =>
proceed()
.then(resolve)
.catch(reject),
};
};

const createFollowerIndex = (name = getRandomString(), payload = getFollowerIndexPayload()) => {
followerIndicesCreated.push(name);
Expand All @@ -24,6 +62,13 @@ export const registerHelpers = supertest => {
.send({ ...payload, name });
};

const updateFollowerIndex = (name, payload) => {
return supertest
.put(`${API_BASE_PATH}/follower_indices/${name}`)
.set('kbn-xsrf', 'xxx')
.send(payload);
};

const unfollowLeaderIndex = followerIndex => {
const followerIndices = Array.isArray(followerIndex) ? followerIndex : [followerIndex];
const followerIndicesToEncodedString = followerIndices
Expand Down Expand Up @@ -51,6 +96,7 @@ export const registerHelpers = supertest => {
loadFollowerIndices,
getFollowerIndex,
createFollowerIndex,
updateFollowerIndex,
unfollowAll,
};
};
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export default function({ getService }) {
loadFollowerIndices,
getFollowerIndex,
createFollowerIndex,
updateFollowerIndex,
unfollowAll,
} = registerFollowerIndicesnHelpers(supertest);

Expand Down Expand Up @@ -92,6 +93,31 @@ export default function({ getService }) {
});
});

describe('update()', () => {
it('should update a follower index advanced settings', async () => {
// Create a follower index
const leaderIndex = await createIndex();
const followerIndex = getRandomString();
const initialValue = 1234;
const payload = getFollowerIndexPayload(leaderIndex, undefined, {
maxReadRequestOperationCount: initialValue,
});
await createFollowerIndex(followerIndex, payload);

// Verify that its advanced settings are correctly set
const { body } = await getFollowerIndex(followerIndex, true);
expect(body.maxReadRequestOperationCount).to.be(initialValue);

// Update the follower index
const updatedValue = 7777;
await updateFollowerIndex(followerIndex, { maxReadRequestOperationCount: updatedValue });

// Verify that the advanced settings are updated
const { body: updatedBody } = await getFollowerIndex(followerIndex, true);
expect(updatedBody.maxReadRequestOperationCount).to.be(updatedValue);
});
});

describe('Advanced settings', () => {
it('hard-coded values should match Elasticsearch default values', async () => {
/**
Expand Down