Move buildEsQuery to a package by lukasolson · Pull Request #23345 · elastic/kibana

lukasolson · 2018-09-19T23:21:43Z

This PR moves the buildESQuery module (including filters and kuery) into a separate package so that it can be imported on both the client and server (in preparation to support Kuery in Timelion and TSVB).

/cc @timroes @lukeelmers

To do:

Figure out why errors are no longer handled the same way
Fix tests

elasticmachine · 2018-09-19T23:35:06Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

timroes · 2018-09-20T13:08:41Z

.eslintignore

-/src/ui/public/kuery/ast/kuery.js
-/src/ui/public/kuery/ast/legacy_kuery.js
+/src/utils/kuery/ast/kuery.js
+/src/utils/kuery/ast/legacy_kuery.js


Out of interest: why are we ignoring those two files for linting? :)

They're generated by PEG

elasticmachine · 2018-09-21T01:56:35Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

Bargs

Looks great overall, just had a couple minor questions. Tested some queries and filters in the UI and everything seemed to work still.

Bargs · 2018-09-28T17:15:02Z

src/utils/es_query/from_kuery.js

        throw parseError;
      }
-      throw new Error(
-        `It looks like you're using an outdated Kuery syntax. See what changed in the [docs](${queryDocs.kueryQuerySyntax})!`


Why did you remove the link to the docs? Without any direction I think it'll be impossible for a user to find the right information.

I didn't want it to rely on something in public/ (which is where documentationLinks is). I guess I could just make the invoking function in public/ check for this type of error and add the links.

The documentation_links module could also probably be moved to utils?

That module relies on ui/metadata which I'm not sure could be moved to utils...

It only uses the branch name though. I think the stuff in metadata ultimately comes from the server, so the branch name should be accessible somewhere.

Bargs · 2018-09-28T17:16:39Z

src/utils/es_query/from_kuery.js

-    catch (parseError) {
-      if (parseError instanceof NoLeadingWildcardsError) {
-        throw parseError;
-      }


What was this condition doing and why was it removed?

Because I got rid of the NoLeadingWildcardsError since it relied on KbnError in public. Instead, we just throw a regular error. This condition wasn't actually really necessary anyway since we end up throwing the error anyway below (unless it parses successfully in the old syntax). So in theory, the only thing that changes by removing this condition is when something would parse successfully in the old grammar but also has a leading wildcard, which I can't imagine of a scenario.

Bargs · 2018-09-28T17:28:26Z

src/ui/public/courier/search_source/index.js

+  decorateQuery,
+  buildQueryFromFilters,
+  luceneStringToDsl
+} from '../../../../utils/es_query';


Do we even need to export these from here now are can existing users of this module just import the functions from the new directory?

We could, it just seemed like fewer changes to do it this way rather than go through each of the imports and change them. I'm open to changing this if you think it makes sense.

Maybe I missed some, but it looked to me like the imports had already been changed. Intellij's refactoring tools should handle it. I'd just rip the band aid off cleanly so folks in the future aren't confused about where they should be importing from.

Made the change. Wasn't even bad at all

Bargs · 2018-09-28T17:30:03Z

src/ui/public/courier/search_source/search_source.js

+          try {
+            flatData.body.query = buildESQuery(flatData.index, flatData.query, flatData.filters);
+          } catch (e) {
+            throw new KbnError(e.message, KbnError);


Ok now that I see this, I'm assuming that's why you removed the wildcard error type?

Bargs · 2018-09-28T17:31:30Z

src/ui/public/kuery/index.js

-export * from './ast';
-export * from './filter_migration';
-export * from './node_types';
+export * from '../../../utils/kuery';


Can we just delete this module now?

Same comment as above, we could, but we'd have to update all of the references, which is fine by me if you think it's best.

lukasolson · 2018-10-01T23:34:33Z

Responded to your feedback @Bargs. I also updated it so it retains the link to the docs in the outdated syntax error.

elasticmachine · 2018-10-02T00:20:06Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-10-02T18:10:30Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

lukeelmers

code makes sense & LGTM - just added one question

lukeelmers · 2018-10-03T16:05:25Z

src/ui/public/courier/search_source/search_source.js

+            if (e.message === 'OutdatedKuerySyntaxError') {
+              const message =  `It looks like you're using an outdated Kuery syntax.
+                See what changed in the [docs](${documentationLinks.query.kueryQuerySyntax})!`;
+              throw new KbnError(message, KbnError);


Rather than adding the message / link back here, would it be better to add a class for this error type to errors.js? It seems that's a convention we've followed elsewhere, but not sure if it's something we are sticking with.

ppisljar · 2018-10-17T12:11:21Z

couldn't we move this to a package, so it could then be imported with import { ... } from '@kbn/kuery' ?

lukasolson · 2018-10-17T14:18:09Z

@ppisljar Yeah, that makes a lot of sense. I'll see if I can get that working.

elasticmachine · 2018-11-14T20:50:06Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-11-15T17:26:51Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

This reverts commit 991d46f.

elasticmachine · 2018-11-15T17:58:28Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-11-15T18:59:05Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-11-19T18:17:39Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-11-20T16:41:37Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-11-20T19:07:37Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

markov00 · 2018-11-21T15:12:09Z

@mattapperson @sqren @weltenwort Would you mind taking another quick look at this PR if we miss something for Infra/APM? We are trying to get this merged soon because we have some other PRs that depends on this.
Nothing really changed since your comments, we just fixed the CI failures that was related to a wrong babel configuration.
Thanks

weltenwort · 2018-11-22T13:25:26Z

still looks good as far as the infra ui usage is concerned 👍

sorenlouv · 2018-11-22T14:07:46Z

Everything looks good from APM POV 👍

markov00 · 2018-11-22T14:41:37Z

@Bargs @lukasolson I've merged on master, but there is a conflict on backporting this PR because of this missing backported PR: #15640.
It was a breacking change that goes only into 7.0, so I will let you decide if we should backport to 6.6 as this PR label says or not

spalger · 2018-11-22T20:52:54Z

@lukasolson when backporting this PR please remove the x-pack/yarn.lock file, it was removed when we moved to yarn workspaces.

* fix: move buildEsQuery to utils * fix: tests that I broke * fix: add back link to the docs * fix: don't export from ui/ and link to utils * fix: move to a package * fix: move error to errors.js * fix: paths for peg task * fix: update reference to kuery * fix: build step for transpilation * fix: add typescript declaration file * fix: test * tmp: debug individual tests * debug: add debug stuff for reporting tests * try to debug test * Testing splitting reporting jobs in two * Testing splitting each job * Fix ci yaml * Skipping job to check failing test * debug - adding a catch to jobResponseHandler on report * Testing a different job and enabling verbose mode * Testing verbose on phantom_api skipping other CI tests * Fix script mode * fix: try running tests in chromium * fix: move out of devDependencies * fix: remove commented test * Revert "fix: try running tests in chromium" This reverts commit 991d46f. * Revert testing changes * Fixing build for phantomjs * Revert CI configuration to master. Remove verbose logging for tests

* Move buildEsQuery to a package (#23345) * fix: move buildEsQuery to utils * fix: tests that I broke * fix: add back link to the docs * fix: don't export from ui/ and link to utils * fix: move to a package * fix: move error to errors.js * fix: paths for peg task * fix: update reference to kuery * fix: build step for transpilation * fix: add typescript declaration file * fix: test * tmp: debug individual tests * debug: add debug stuff for reporting tests * try to debug test * Testing splitting reporting jobs in two * Testing splitting each job * Fix ci yaml * Skipping job to check failing test * debug - adding a catch to jobResponseHandler on report * Testing a different job and enabling verbose mode * Testing verbose on phantom_api skipping other CI tests * Fix script mode * fix: try running tests in chromium * fix: move out of devDependencies * fix: remove commented test * Revert "fix: try running tests in chromium" This reverts commit 991d46f. * Revert testing changes * Fixing build for phantomjs * Revert CI configuration to master. Remove verbose logging for tests * remove x-pack/yarn.lock, accidentally added back in #23345 * Fix import sorting

fix: move buildEsQuery to utils

e27ba53

lukasolson added WIP Work in progress Feature:Query Bar Querying and query bar features Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// Feature:KQL KQL labels Sep 19, 2018

timroes reviewed Sep 20, 2018

View reviewed changes

fix: tests that I broke

1308202

Merge branch 'master' into fix/filtersEverywhere

0480b9c

lukasolson requested a review from Bargs September 24, 2018 21:38

lukasolson self-assigned this Sep 24, 2018

Bargs reviewed Sep 28, 2018

View reviewed changes

lukasolson added 2 commits October 1, 2018 13:25

Merge branch 'master' into fix/filtersEverywhere

72fbe52

fix: add back link to the docs

7c93a4b

fix: don't export from ui/ and link to utils

391060f

lukeelmers self-requested a review October 3, 2018 15:25

lukeelmers approved these changes Oct 3, 2018

View reviewed changes

lukasolson changed the title ~~[WIP] Move buildEsQuery to utils~~ Move buildEsQuery to utils Oct 4, 2018

lukasolson added review and removed WIP Work in progress labels Oct 4, 2018

lukasolson added 4 commits October 17, 2018 07:44

Merge branch 'master' into fix/filtersEverywhere

fb63b64

Merge branch 'master' into fix/filtersEverywhere

dafa660

fix: move to a package

a96841a

fix: move error to errors.js

09a69ed

fix: move out of devDependencies

04f5e37

lukasolson added 2 commits November 15, 2018 10:31

fix: remove commented test

9fd38fe

Revert "fix: try running tests in chromium"

7db5961

This reverts commit 991d46f.

markov00 added 2 commits November 19, 2018 18:31

Revert testing changes

df364f0

Merge branch 'master' into pr/23345

408ddb4

Fixing build for phantomjs

ffa8657

Revert CI configuration to master. Remove verbose logging for tests

ee4ddc2

This was referenced Nov 21, 2018

Decoupling UI Settings from buildEsQuery #25994

Merged

Enable KQL support for TSVB #26006

Merged

mattapperson approved these changes Nov 22, 2018

View reviewed changes

markov00 merged commit e36767a into elastic:master Nov 22, 2018

spalger pushed a commit that referenced this pull request Nov 22, 2018

remove x-pack/yarn.lock, accidentally added back in #23345

f981cdf

lukasolson pushed a commit to lukasolson/kibana that referenced this pull request Nov 27, 2018

remove x-pack/yarn.lock, accidentally added back in elastic#23345

f9dd04f

lukasolson mentioned this pull request Nov 27, 2018

[6.x] Move buildEsQuery to a package #26306

Merged

peteharverson mentioned this pull request Nov 29, 2018

[6.x] [ML] Adds support for kuery to job wizards (#26094) #26403

Merged

lukasolson deleted the fix/filtersEverywhere branch December 2, 2019 22:34

Conversation

lukasolson commented Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Sep 19, 2018

💔 Build Failed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Sep 21, 2018

💚 Build Succeeded

Uh oh!

Bargs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukasolson commented Oct 1, 2018

Uh oh!

elasticmachine commented Oct 2, 2018

💔 Build Failed

Uh oh!

elasticmachine commented Oct 2, 2018

💔 Build Failed

Uh oh!

lukeelmers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ppisljar commented Oct 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukasolson commented Oct 17, 2018

Uh oh!

elasticmachine commented Nov 14, 2018

💔 Build Failed

Uh oh!

elasticmachine commented Nov 15, 2018

💔 Build Failed

Uh oh!

elasticmachine commented Nov 15, 2018

💔 Build Failed

Uh oh!

elasticmachine commented Nov 15, 2018

💔 Build Failed

Uh oh!

elasticmachine commented Nov 19, 2018

💔 Build Failed

Uh oh!

elasticmachine commented Nov 20, 2018

💚 Build Succeeded

lukasolson commented Sep 19, 2018 •

edited

Loading

ppisljar commented Oct 17, 2018 •

edited

Loading