fix character escaping for elasticsearch to work correctly #16025
Conversation
Alexandr-Nedomets
requested review from
harshach,
karanh37,
chirag-madlani and
Sachin-chaurasiya
as code owners
Alexandr-Nedomets
had a problem deploying
to
test
GitHub Actions
Failure
Alexandr-Nedomets
had a problem deploying
to
test
GitHub Actions
Failure
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
harshach
added
the
safe to test
|
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
Jest test CoverageUI tests summary
|
13 failed tests on run #33990 ↗︎
Details:
|
||||||||||||||||||||||||||||||||||||||||||
| Test | Artifacts | |
|---|---|---|
| Airflow Ingestion > Create & Ingest Airflow service |
Screenshots
Video
|
|
Pages/Glossary.spec.ts • 2 failed tests • cypress-ci-mysql-pr-8843531936-1
Pages/DataQualityAndProfiler.spec.ts • 1 failed test • cypress-ci-mysql-pr-8843531936-1
| Test | Artifacts | |
|---|---|---|
| Data Quality and Profiler should work properly > Add and ingest mysql data |
Screenshots
Video
|
|
Features/IncidentManager.spec.ts • 1 failed test • cypress-ci-mysql-pr-8843531936-1
| Test | Artifacts | |
|---|---|---|
| Incident Manager > Basic Scenario > Acknowledge table test case's failure |
Screenshots
Video
|
|
Features/QueryEntity.spec.ts • 2 failed tests • cypress-ci-mysql-pr-8843531936-1
| Test | Artifacts | |
|---|---|---|
| Query Entity > Update owner, description and tag |
Screenshots
Video
|
|
| Query Entity > Verify query filter |
Screenshots
Video
|
|
The first 5 failed specs are shown, see all 9 specs in Cypress Cloud.
Review all test suite changes for PR #16025 ↗︎
|
@karanh37 can you review and merge |
| @@ -188,7 +188,9 @@ export const customServiceComparator = (a: string, b: string): number => { | |||
| export const replacePlus = (fqn: string) => fqn.replaceAll('+', ' '); | |||
|
|
|||
| export const ES_RESERVED_CHARACTERS: Record<string, string> = { | |||
| '+': '\\+', | |||
There was a problem hiding this comment.
Hi this is breaking at other places.
The idea behind this method is just to escape special characters.
We should not be encoding characters here.
There was a problem hiding this comment.
Refer Glossary Assets page.
Glossary terms with special names are not able to fetch assets because we are encoding here.
There was a problem hiding this comment.
Hi!
The error may occur because when querying the api /api/v1/search/query, the searchValue must be converted using the escapeESReservedCharacters method.
If you are wrong, describe the problem in more detail
| '&': '%26', | ||
| '#': '%23', |
There was a problem hiding this comment.
@Alexandr-Nedomets can we escap the required chars here instead replacing it with encoding?
I have validated that we need to escape this chars instead encoding it. I will merge your PR once you push the changes.
| '&': '%26', | |
| '#': '%23', | |
| '&': '\\&', | |
| '#': '\\#', | |
| '+': '\\+', |
There was a problem hiding this comment.
Hi, @chirag-madlani
If you fix it like this, the search will not work correctly.
This fix will not fix the problems @karanh37 wrote about.
Head branch was pushed to by a user without write access
|
Describe your changes:
Fix character escaping for elasticsearch to work correctly
in more detail in ISSUE-16001
Fixes ISSUE-16001
Type of change: