Skip to content

Commit

Permalink
[Console] Fix the end range of selected requests (#189747)
Browse files Browse the repository at this point in the history
## Summary

Fixes #189366
Fixes #186773

This PR refactors how the request body is being extracted from the
editor to use for "sendRequest" and "copyAsCurl" functions. Previously
the editor actions provider would rely on the parser to get a JSON
object or several for request body. The downside of this implementation
was when the parser would not be able to fully process the json object.
That could lead to potential text loss and the editor would process the
requests in a way that was not always obvious to the user. For example,
the editor would highlight the request with the json object, but when
sending it to ES the request body would be completely ignored.
Instead this PR suggests to use the "raw" text from the editor for
actions and give the user more transparency and control over the
requests. We also don't need to keep the information about requests in
the parser, which might affect browser performance for longer texts.

### Checklist

Delete any items that are not applicable to this PR.

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [ ] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [ ] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)


### Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to
identify risks that should be tested prior to the change/feature
release.

When forming the risk matrix, consider some of the following examples
and how they may potentially impact the change:

| Risk | Probability | Severity | Mitigation/Notes |

|---------------------------|-------------|----------|-------------------------|
| Multiple Spaces—unexpected behavior in non-default Kibana Space.
| Low | High | Integration tests will verify that all features are still
supported in non-default Kibana Space and when user switches between
spaces. |
| Multiple nodes—Elasticsearch polling might have race conditions
when multiple Kibana nodes are polling for the same tasks. | High | Low
| Tasks are idempotent, so executing them multiple times will not result
in logical error, but will degrade performance. To test for this case we
add plenty of unit tests around this logic and document manual testing
procedure. |
| Code should gracefully handle cases when feature X or plugin Y are
disabled. | Medium | High | Unit tests will verify that any feature flag
or plugin combination still results in our service operational. |
| [See more potential risk
examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) |


### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
  • Loading branch information
yuliacech authored Aug 20, 2024
1 parent 1053ec6 commit f3a6527
Show file tree
Hide file tree
Showing 14 changed files with 474 additions and 209 deletions.
35 changes: 9 additions & 26 deletions packages/kbn-monaco/src/console/parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,24 +37,7 @@ export const createParser = () => {
requestStartOffset = at - 1;
requests.push({ startOffset: requestStartOffset });
},
addRequestMethod = function(method) {
const lastRequest = getLastRequest();
lastRequest.method = method;
requests.push(lastRequest);
requestEndOffset = at - 1;
},
addRequestUrl = function(url) {
const lastRequest = getLastRequest();
lastRequest.url = url;
requests.push(lastRequest);
requestEndOffset = at - 1;
},
addRequestData = function(data) {
const lastRequest = getLastRequest();
const dataArray = lastRequest.data || [];
dataArray.push(data);
lastRequest.data = dataArray;
requests.push(lastRequest);
updateRequestEnd = function () {
requestEndOffset = at - 1;
},
addRequestEnd = function() {
Expand Down Expand Up @@ -409,26 +392,26 @@ export const createParser = () => {
request = function () {
white();
addRequestStart();
const parsedMethod = method();
addRequestMethod(parsedMethod);
method();
updateRequestEnd();
strictWhite();
const parsedUrl = url();
addRequestUrl(parsedUrl);
url();
updateRequestEnd();
strictWhite(); // advance to one new line
newLine();
strictWhite();
if (ch == '{') {
const parsedObject = object();
addRequestData(parsedObject);
object();
updateRequestEnd();
}
// multi doc request
strictWhite(); // advance to one new line
newLine();
strictWhite();
while (ch == '{') {
// another object
const parsedObject = object();
addRequestData(parsedObject);
object();
updateRequestEnd();
strictWhite();
newLine();
strictWhite();
Expand Down
21 changes: 8 additions & 13 deletions packages/kbn-monaco/src/console/parser.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,7 @@ describe('console parser', () => {
const { requests, errors } = parser(input) as ConsoleParserResult;
expect(requests.length).toBe(1);
expect(errors.length).toBe(0);
const { method, url, startOffset, endOffset } = requests[0];
expect(method).toBe('GET');
expect(url).toBe('_search');
const { startOffset, endOffset } = requests[0];
// the start offset of the request is the beginning of the string
expect(startOffset).toBe(0);
// the end offset of the request is the end of the string
Expand All @@ -38,22 +36,19 @@ describe('console parser', () => {
const input = 'GET _search\nPOST _test_index';
const { requests } = parser(input) as ConsoleParserResult;
expect(requests.length).toBe(2);
expect(requests[0].startOffset).toBe(0);
expect(requests[0].endOffset).toBe(11);
expect(requests[1].startOffset).toBe(12);
expect(requests[1].endOffset).toBe(28);
});

it('parses a request with a request body', () => {
const input =
'GET _search\n' + '{\n' + ' "query": {\n' + ' "match_all": {}\n' + ' }\n' + '}';
const { requests } = parser(input) as ConsoleParserResult;
expect(requests.length).toBe(1);
const { method, url, data } = requests[0];
expect(method).toBe('GET');
expect(url).toBe('_search');
expect(data).toEqual([
{
query: {
match_all: {},
},
},
]);
const { startOffset, endOffset } = requests[0];
expect(startOffset).toBe(0);
expect(endOffset).toBe(52);
});
});
3 changes: 0 additions & 3 deletions packages/kbn-monaco/src/console/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,6 @@ export interface ErrorAnnotation {
export interface ParsedRequest {
startOffset: number;
endOffset?: number;
method: string;
url?: string;
data?: Array<Record<string, unknown>>;
}
export interface ConsoleParserResult {
errors: ErrorAnnotation[];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { debounce, range } from 'lodash';
import { ConsoleParsedRequestsProvider, getParsedRequestsProvider, monaco } from '@kbn/monaco';
import { i18n } from '@kbn/i18n';
import { toMountPoint } from '@kbn/react-kibana-mount';
import { XJson } from '@kbn/es-ui-shared-plugin/public';
import { isQuotaExceededError } from '../../../../services/history';
import { DEFAULT_VARIABLES } from '../../../../../common/constants';
import { getStorage, StorageKeys } from '../../../../services';
Expand All @@ -33,13 +34,14 @@ import {
replaceRequestVariables,
SELECTED_REQUESTS_CLASSNAME,
shouldTriggerSuggestions,
stringifyRequest,
trackSentRequests,
getRequestFromEditor,
} from './utils';

import type { AdjustedParsedRequest } from './types';
import { StorageQuotaError } from '../../../components/storage_quota_error';
import { ContextValue } from '../../../contexts';
import { containsComments, indentData } from './utils/requests_utils';

const AUTO_INDENTATION_ACTION_LABEL = 'Apply indentations';
const TRIGGER_SUGGESTIONS_ACTION_LABEL = 'Trigger suggestions';
Expand All @@ -48,6 +50,7 @@ const DEBOUNCE_HIGHLIGHT_WAIT_MS = 200;
const DEBOUNCE_AUTOCOMPLETE_WAIT_MS = 500;
const INSPECT_TOKENS_LABEL = 'Inspect tokens';
const INSPECT_TOKENS_HANDLER_ID = 'editor.action.inspectTokens';
const { collapseLiteralStrings } = XJson;

export class MonacoEditorActionsProvider {
private parsedRequestsProvider: ConsoleParsedRequestsProvider;
Expand Down Expand Up @@ -173,12 +176,12 @@ export class MonacoEditorActionsProvider {
const selectedRequests: AdjustedParsedRequest[] = [];
for (const [index, parsedRequest] of parsedRequests.entries()) {
const requestStartLineNumber = getRequestStartLineNumber(parsedRequest, model);
const requestEndLineNumber = getRequestEndLineNumber(
const requestEndLineNumber = getRequestEndLineNumber({
parsedRequest,
nextRequest: parsedRequests.at(index + 1),
model,
index,
parsedRequests
);
startLineNumber,
});
if (requestStartLineNumber > endLineNumber) {
// request is past the selection, no need to check further requests
break;
Expand All @@ -198,13 +201,31 @@ export class MonacoEditorActionsProvider {
}

public async getRequests() {
const model = this.editor.getModel();
if (!model) {
return [];
}

const parsedRequests = await this.getSelectedParsedRequests();
const stringifiedRequests = parsedRequests.map((parsedRequest) =>
stringifyRequest(parsedRequest)
);
const stringifiedRequests = parsedRequests.map((parsedRequest) => {
const { startLineNumber, endLineNumber } = parsedRequest;
const requestTextFromEditor = getRequestFromEditor(model, startLineNumber, endLineNumber);
if (requestTextFromEditor && requestTextFromEditor.data.length > 0) {
requestTextFromEditor.data = requestTextFromEditor.data.map((dataString) => {
if (containsComments(dataString)) {
// parse and stringify to remove comments
dataString = indentData(dataString);
}
return collapseLiteralStrings(dataString);
});
}
return requestTextFromEditor;
});
// get variables values
const variables = getStorage().get(StorageKeys.VARIABLES, DEFAULT_VARIABLES);
return stringifiedRequests.map((request) => replaceRequestVariables(request, variables));
return stringifiedRequests
.filter(Boolean)
.map((request) => replaceRequestVariables(request!, variables));
}

public async getCurl(elasticsearchBaseUrl: string): Promise<string> {
Expand Down Expand Up @@ -388,12 +409,6 @@ export class MonacoEditorActionsProvider {
return null;
}

// if the current request doesn't have a method, the request is not valid
// and shouldn't have an autocomplete type
if (!currentRequest.method) {
return null;
}

// if not on the 1st line of the request, suggest request body
return AutocompleteType.BODY;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ export const getDocumentationLinkFromAutocomplete = (
* Helper function that filters out suggestions without a name.
*/
const filterTermsWithoutName = (terms: ResultTerm[]): ResultTerm[] =>
terms.filter((term) => term.name !== undefined);
terms.filter((term) => term.name !== undefined && term.name !== '');

/*
* This function returns an array of completion items for the request method
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ export const END_OF_URL_TOKEN = '__url_path_end__';
* In this case autocomplete suggestions should be triggered for an url.
*/
export const methodWhitespaceRegex = /^\s*(GET|POST|PUT|PATCH|DELETE)\s+$/i;
/*
* This regex matches a string that starts with a method (optional whitespace before the method)
*/
export const startsWithMethodRegex = /^\s*(GET|POST|PUT|PATCH|DELETE)/i;
/*
* This regex matches a string that has
* a method and some parts of an url ending with a slash, a question mark or an equals sign,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,11 @@ export {
export {
getRequestStartLineNumber,
getRequestEndLineNumber,
stringifyRequest,
replaceRequestVariables,
getCurlRequest,
trackSentRequests,
getAutoIndentedRequests,
getRequestFromEditor,
} from './requests_utils';
export {
getDocumentationLinkFromAutocomplete,
Expand Down
Loading

0 comments on commit f3a6527

Please sign in to comment.