Merge pull request #105 from bcgsc/task/KBDEV-1426-containstext-special-characters-handling-2

Task/kbdev 1426 containstext special characters handling 2

Author: mathieulemieux

.github/workflows/npm-test.yml

@@ -9,6 +9,7 @@ jobs:
   build:
     runs-on: ubuntu-latest
     strategy:
+      fail-fast: false
       matrix:
         node: [16, 18.18.2, 20] # KBDEV-1292. Forcing v18.18.2 because having issues with checks using 18.20.7 (default v18 on GH)
         orientdb: ['3.0', '3.1', '3.2.13']
@@ -37,6 +38,7 @@ jobs:
           GKB_DB_HOST: localhost
           GKB_DBS_PASS: root # default password for new db
           GKB_LOG_LEVEL: ERROR
+          ORIENTDB_VERSION: ${{ matrix.orientdb }} # KBDEV-1426. Skipping some tests on OrientDB 3.1+
       - uses: EnricoMi/publish-unit-test-result-action@v1.6
         with:
           github_token: ${{ secrets.GITHUB_TOKEN }}

src/repo/query_builder/constants.js

@@ -68,6 +68,8 @@ const SIMILARITY_EDGES = [
     'GeneralizationOf',
 ];
 
+const SEPARATOR_CHARS = ':;,.|+*/\\=!?[]()';
+
 module.exports = {
     DEFAULT_NEIGHBORS: 3,
     DIRECTIONS,
@@ -77,6 +79,7 @@ module.exports = {
     MIN_WORD_SIZE: 3,
     OPERATORS,
     PARAM_PREFIX,
+    SEPARATOR_CHARS,
     SIMILARITY_EDGES,
     TREE_EDGES,
 };

src/repo/query_builder/fixed.js

@@ -25,7 +25,7 @@ const {
     SIMILARITY_EDGES,
     TREE_EDGES,
 } = require('./constants');
-const { castRangeInt } = require('./util');
+const { castRangeInt, splitIntoKeywords } = require('./util');
 
 const disambiguationClause = (cond, edges = SIMILARITY_EDGES) => `TRAVERSE both(${edges.map((e) => `'${e}'`).join(', ')}) FROM ${cond} MAXDEPTH ${MAX_NEIGHBORS}`;
 
@@ -456,19 +456,25 @@ const keywordSearch = ({
     if (model.isEdge) {
         throw new ValidationError(`Cannot keyword search edge classes (${target})`);
     }
-
     if (!keyword) {
         throw new ValidationError('Missing required keyword parameter');
     }
 
-    // remove any duplicate words
-    const wordList = operator === OPERATORS.CONTAINSTEXT
-        ? keyword.split(/\s+/).map((word) => word.trim().toLowerCase())
+    let wordList = operator === OPERATORS.CONTAINSTEXT
+        ? splitIntoKeywords(keyword)
         : [keyword.trim().toLowerCase()];
 
+    // words needs to be 3 letters or more
+    wordList = operator === OPERATORS.CONTAINSTEXT
+        ? wordList.filter((word) => word.length >= 3)
+        : wordList;
+
+    // need at least one word
     if (wordList.length < 1) {
         throw new ValidationError('missing keywords');
     }
+
+    // remove any duplicate words
     const keywords = Array.from(new Set(wordList)).filter((k) => k).sort();
 
     const params = {};
@@ -569,4 +575,4 @@ class FixedSubquery {
     }
 }
 
-module.exports = { FixedSubquery };
+module.exports = { FixedSubquery, keywordSearch };

src/repo/query_builder/fragment.js

@@ -3,9 +3,9 @@ const { RecordID: RID } = require('orientjs');
 
 const { ValidationError, schema, util } = require('@bcgsc-pori/graphkb-schema');
 
-const { OPERATORS, PARAM_PREFIX } = require('./constants');
+const { OPERATORS, PARAM_PREFIX, SEPARATOR_CHARS } = require('./constants');
 const { FixedSubquery } = require('./fixed');
-const { getQueryableProps } = require('./util');
+const { getQueryableProps, hasSeparatorChars } = require('./util');
 
 const NUMBER_ONLY_OPERATORS = [OPERATORS.GT, OPERATORS.GTE, OPERATORS.LT, OPERATORS.LTE];
 
@@ -85,6 +85,26 @@ class Comparison {
             );
         }
 
+        if (
+            this.operator === OPERATORS.CONTAINSTEXT
+            && !['CategoryVariant', 'PositionalVariant', 'Variant'].includes(this.name)
+        ) {
+            const v = String(this.value);
+
+            if (v.includes(' ')) {
+                throw new ValidationError(
+                    'CONTAINSTEXT should not be used in conjunction with whitespaces',
+                );
+            }
+            if (hasSeparatorChars(v)) {
+                throw new ValidationError(
+                    `CONTAINSTEXT should not be used in conjunction with index separator characters ( ${
+                        SEPARATOR_CHARS
+                    } )`,
+                );
+            }
+        }
+
         if (this.valueIsIterable) {
             if (this.operator === OPERATORS.CONTAINS) {
                 throw new ValidationError(
@@ -470,4 +490,4 @@ class Subquery {
     }
 }
 
-module.exports = { Subquery };
+module.exports = { Comparison, Subquery };

src/repo/query_builder/util.js

@@ -4,7 +4,7 @@ const {
     util,
     schema,
 } = require('@bcgsc-pori/graphkb-schema');
-const { MAX_LIMIT, MAX_NEIGHBORS } = require('./constants');
+const { MAX_LIMIT, MAX_NEIGHBORS, SEPARATOR_CHARS } = require('./constants');
 
 /**
  * Format a value as an Integer. Throw an error if it is not an integer or does not
@@ -133,10 +133,40 @@ const displayQuery = ({ query: statement, params = {} }) => {
     return result;
 };
 
+/**
+ * Check if a keyword contains any separatorChars
+ *
+ * @param {string} kw a keyword to test for separatorChars
+ * @param {string} sep a string of all separatorChars to test against
+ * @returns {boolean}
+ */
+const hasSeparatorChars = (kw, sep = SEPARATOR_CHARS) => [...kw].some((chr) => sep.includes(chr));
+
+/**
+ * Split a string into an array of keywords based on separatorChars and whitespaces
+ * Also convert to lowercase
+ *
+ * @param {string} s a string to split into an array of keywords
+ * @param {string} sep a string of all separatorChars
+ * @returns {Array.<string>}
+ */
+const splitIntoKeywords = (s, sep = SEPARATOR_CHARS) => {
+    const pattern = new RegExp(
+        // the regex itself needs some escaping. also adding whitespace support
+        `[\\s${sep.replace(/[-\\\]^[]/g, '\\$&')}]+`,
+    );
+    return s
+        .trim()
+        .split(pattern)
+        .map((word) => word.toLowerCase());
+};
+
 module.exports = {
     castBoolean,
     castRangeInt,
     checkStandardOptions,
     displayQuery,
     getQueryableProps,
+    hasSeparatorChars,
+    splitIntoKeywords,
 };

test/db_integration/queries.test.js

@@ -112,7 +112,11 @@ describeWithAuth('query builder', () => {
         });
     });
 
-    describe('selectByKeyword', () => {
+    // KBDEV-1426. Skipping keyword queryType tests for OrientDB != 3.0
+    (process.env.ORIENTDB_VERSION === '3.0' || process.env.ORIENTDB_VERSION === undefined
+        ? describe
+        : describe.skip
+    )('selectByKeyword', () => {
         test('get from related variant reference', async () => {
             const query = parse({ keyword: 'kras', queryType: 'keyword', target: 'Statement' });
             const result = await select(session, query);
@@ -133,6 +137,24 @@ describeWithAuth('query builder', () => {
                 ),
             ).toHaveProperty('length', 1);
         });
+
+        test('short terms are discarded without errors when long terms are present', async () => {
+            expect(
+                await select(
+                    session,
+                    parse({ keyword: 'kras gain of function', queryType: 'keyword', target: 'Statement' }),
+                ),
+            ).toHaveProperty('length', 1);
+        });
+
+        test('short terms throw error when long terms are not present', async () => {
+            await expect(
+                select(
+                    session,
+                    parse({ keyword: 'of', queryType: 'keyword', target: 'Statement' }),
+                ),
+            ).rejects.toThrow(ValidationError);
+        });
     });
 
     test('custom projection', async () => {

test/db_integration/query_routes.test.js

@@ -111,7 +111,11 @@ describeWithAuth('api read-only routes', () => {
         });
     });
 
-    describe('/query search statements by keyword', () => {
+    // KBDEV-1426. Skipping keyword queryType tests for OrientDB != 3.0
+    (process.env.ORIENTDB_VERSION === '3.0' || process.env.ORIENTDB_VERSION === undefined
+        ? describe
+        : describe.skip
+    )('/query search statements by keyword', () => {
         test('count ignores limit', async () => {
             const response = await request({
                 body: {
@@ -146,10 +150,10 @@ describeWithAuth('api read-only routes', () => {
             expect(response.body.result).toHaveProperty('length', 2);
         });
 
-        test('multiple keywords are co-required', async () => {
+        test('multiple keywords are co-required; without match', async () => {
             const response = await request({
                 body: {
-                    keyword: 'kras,resistance',
+                    keyword: 'kras resistance',
                     queryType: 'keyword',
                     target: 'Statement',
                 },
@@ -162,6 +166,22 @@ describeWithAuth('api read-only routes', () => {
             expect(response.body.result).toHaveProperty('length', 0);
         });
 
+        test('multiple keywords are co-required; with match', async () => {
+            const response = await request({
+                body: {
+                    keyword: 'kras gain of function',
+                    queryType: 'keyword',
+                    target: 'Statement',
+                },
+                headers: { Authorization: mockToken },
+                method: 'POST',
+                uri,
+            });
+            expect(response.statusCode).toBe(HTTP_STATUS.OK);
+            expect(response.body).toHaveProperty('result');
+            expect(response.body.result).toHaveProperty('length', 1);
+        });
+
         test('error on no body', async () => {
             try {
                 await request({

test/repo/query_builder/fragment.test.js

@@ -0,0 +1,51 @@
+const { ValidationError } = require('@bcgsc-pori/graphkb-schema');
+
+const { Comparison } = require('../../../src/repo/query_builder/fragment');
+
+describe('Comparison', () => {
+    // Ontology
+    const OntologyComparison = new Comparison({
+        name: 'Ontology',
+        operator: 'CONTAINSTEXT',
+        prop: 'name',
+        value: 'abcdef',
+    });
+
+    test('not to throw error on valid CONTAINSTEXT value', () => {
+        expect(() => OntologyComparison.validate()).not.toThrow(ValidationError);
+    });
+
+    test('throw error on CONTAINSTEXT value containing whitespaces', () => {
+        OntologyComparison.value = 'abc def';
+        expect(() => OntologyComparison.validate()).toThrow(ValidationError);
+    });
+
+    test('throw error on CONTAINSTEXT value containing separator chars', () => {
+        OntologyComparison.value = 'abc:def';
+        expect(() => OntologyComparison.validate()).toThrow(ValidationError);
+    });
+
+    // Variant
+    const VariantComparison = new Comparison({
+        name: 'Variant',
+        operator: 'CONTAINSTEXT',
+        prop: 'displayName',
+        value: 'abc:def',
+    });
+
+    test('Variant special case: not to throw error', () => {
+        expect(() => VariantComparison.validate()).not.toThrow(ValidationError);
+    });
+
+    test('CategoryVariant special case: not to throw error', () => {
+        VariantComparison.name = 'CategoryVariant';
+        VariantComparison.value = 'gene1:gene2 fusion';
+        expect(() => VariantComparison.validate()).not.toThrow(ValidationError);
+    });
+
+    test('PositionalVariant special case: not to throw error', () => {
+        VariantComparison.name = 'PositionalVariant';
+        VariantComparison.value = 'gene:p.123del';
+        expect(() => VariantComparison.validate()).not.toThrow(ValidationError);
+    });
+});

test/repo/query_builder/util.test.js

@@ -0,0 +1,60 @@
+const { hasSeparatorChars, splitIntoKeywords } = require('../../../src/repo/query_builder/util');
+
+describe('hasSeparatorChars', () => {
+    test.each([
+        ['abc def', false], // control
+        ['abc:def', true],
+        ['abc;def', true],
+        ['abc,def', true],
+        ['abc.def', true],
+        ['abc|def', true],
+        ['abc+def', true],
+        ['abc*def', true],
+        ['abc/def', true],
+        ['abc=def', true],
+        ['abc!def', true],
+        ['abc?def', true],
+        ['abc[def', true],
+        ['abc]def', true],
+        ['abc(def', true],
+        ['abc)def', true],
+        ['abc\u005Cdef', true],
+        ['abc\\def', true],
+    ])('hasSeparatorChars(%s) === %s', (kw, expected) => {
+        expect(hasSeparatorChars(kw)).toBe(expected);
+    });
+});
+
+describe('splitIntoKeywords', () => {
+    test.each([
+        ['abcdef', 1], // control
+        ['abc def', 2],
+        ['abc:def', 2],
+        ['abc;def', 2],
+        ['abc,def', 2],
+        ['abc.def', 2],
+        ['abc|def', 2],
+        ['abc+def', 2],
+        ['abc*def', 2],
+        ['abc/def', 2],
+        ['abc=def', 2],
+        ['abc!def', 2],
+        ['abc?def', 2],
+        ['abc[def', 2],
+        ['abc]def', 2],
+        ['abc(def', 2],
+        ['abc)def', 2],
+        ['abc\u005Cdef', 2],
+        ['abc\\def', 2],
+    ])('splitIntoKeywords(%s) === %s', (kw, expected) => {
+        expect(splitIntoKeywords(kw).length).toBe(expected);
+    });
+
+    test.each([
+        ['Abc', ['abc']], // lowercase
+        [' abc ', ['abc']], // trimmed
+    ])('splitIntoKeywords(%s) === %s', (kw, expected) => {
+        expect(splitIntoKeywords(kw).length).toBe(1);
+        expect(splitIntoKeywords(kw)[0]).toBe(expected[0]);
+    });
+});