diff --git a/src/did/+did/+datastructures/fieldsearch.m b/src/did/+did/+datastructures/fieldsearch.m index 270467eb..f29779b6 100644 --- a/src/did/+did/+datastructures/fieldsearch.m +++ b/src/did/+did/+datastructures/fieldsearch.m @@ -25,6 +25,10 @@ % | 'greaterthaneq' - is the field value greater than or equal to 'param1' (and comparable size) % | 'hassize' - does the field value have the size as indicated in 'param1' [x y z ...] % | 'hasmember' - does the field value have the member indicated in 'param1' ? + % | Text is a member of text that is the whole value or an item of its + % | comma-separated list; anything is a member of a cell array that holds + % | an equal element; a number is a member of a numeric array that holds + % | it. Text is never a member of a number, nor a number of text. % | 'hasfield' - is the field present? (no role for 'param1' or 'param2') % | 'partial_struct' - is the field value a structure that has all the fields of 'param1' with the same values? % | (note that it may have additional fields not found in the structure in param1) @@ -148,9 +152,7 @@ end case 'hasmember' if isthere - try - b = ismember(searchstruct.param1,value); - end + b = hasMember(value, searchstruct.param1); end case 'hasfield' b = isthere; @@ -218,3 +220,34 @@ if negation b = ~b; end + +function tf = hasMember(value, member) + % HASMEMBER - is MEMBER a member of VALUE, for 'hasmember'? + % + % Text is a member of text that is the whole value or an item of its + % comma-separated list (white space after a comma, and before one, is + % ignored); anything is a member of a cell array that holds an equal + % element; a number is a member of a numeric or logical array that + % holds it. Text is never a member of a number, nor a number of text. + % This is what did.database's SQL search matches, and DID-python's + % field_search. (ismember compared text character by character.) + tf = false; + if isstring(member) && isscalar(member), member = char(member); end + if isstring(value) && isscalar(value), value = char(value); end + isNumber = @(x) (isnumeric(x) || islogical(x)) && isreal(x); + if ischar(member) + if ischar(value) && isempty(value) + tf = isempty(member); + elseif ischar(value) + pattern = ['(^|,\s*)' regexptranslate('escape', member) '\s*(,|$)']; + tf = ~isempty(regexp(value, pattern, 'once', 'start')); + elseif iscell(value) + tf = any(cellfun(@(v) ischar(v) && strcmp(v, member), value(:))); + end + elseif isNumber(member) && isscalar(member) + if isNumber(value) + tf = any(value(:) == member); + elseif iscell(value) + tf = any(cellfun(@(v) isNumber(v) && isscalar(v) && v == member, value(:))); + end + end diff --git a/src/did/+did/database.m b/src/did/+did/database.m index 32417376..7e193350 100644 --- a/src/did/+did/database.m +++ b/src/did/+did/database.m @@ -1093,13 +1093,10 @@ function close_doc(database_obj, file_obj) case 'hassize' %TODO error('DID:Database:SQL','Query operation "%s" is not yet implemented',op); case 'hasmember' - fieldNameLike = regexprep(field,{'\\','\*','_'},{'\\\\','%','\\_'}); - field_check = ['(' field_check ' OR fields.field_name like "' fieldNameLike '.%" ESCAPE "\")']; - %value_check= ['(doc_data.value like "%' param1 ',%"' ... - value_check = ['(regex(doc_data.value,"^(.*,\s*)*' did.database.sqlEscapeLiteral(param1RegexEsc) '\s*(,.*)*$") NOT NULL' ... - ' OR doc_data.value=' param1StrEsc ... - ' OR doc_data.value="' param1StrEsc '")']; - sql_str = [field_check ' AND ' notStr value_check]; + % Searched by brute force in search_doc_ids (see + % isBruteForceTerm), so never gets here + error('DID:Database:SQL', ['hasmember is searched by brute force, ' ... + 'not in SQL']); case 'hasfield' fieldNameLike = regexprep(field,{'\\','\*','_'},{'\\\\','%','\\_'}); sql_str = [field_check ' OR fields.field_name like "' fieldNameLike '.%" ESCAPE "\"']; @@ -1249,8 +1246,8 @@ function close_doc(database_obj, file_obj) termOp(termOp=='~') = ''; if did.database.baseIdLookupValues(query_struct(i)) rank(i) = 1; - elseif any(strcmp(termOp, {'regexp','contains_string','hasmember'})) ... - || did.database.isBruteForceDependsOn(query_struct(i)) + elseif any(strcmp(termOp, {'regexp','contains_string'})) ... + || did.database.isBruteForceTerm(query_struct(i)) rank(i) = 4; elseif strcmp(char(query_struct(i).field), 'base.session_id') rank(i) = 3; @@ -1316,7 +1313,7 @@ function close_doc(database_obj, file_obj) if size(doc_ids,1)==1 && size(doc_ids,2)>1 doc_ids = doc_ids'; % ensure column vector end - elseif did.database.isBruteForceDependsOn(query_struct) + elseif did.database.isBruteForceTerm(query_struct) doc_ids = brute_force_search(sqlitedb_obj, query_struct, branch_id, candidate_ids); else % leaf scalar query query_str = get_sql_query_str(sqlitedb_obj, query_struct, branch_id, candidate_ids); @@ -1368,9 +1365,9 @@ function close_doc(database_obj, file_obj) % ORed base.id lookups among the operands are searched together % by search_base_ids, as for an OR of nothing but base.id % lookups. Operands that are not single SQL terms - ANDs (struct - % arrays), negated ORs and the depends_on queries that are - % brute-forced (see isBruteForceDependsOn) - are searched by - % search_doc_ids, one at a time, as before. + % arrays), negated ORs and the terms that are brute-forced + % (see isBruteForceTerm) - are searched by search_doc_ids, one + % at a time, as before. batchSize = sqlitedb_obj.orBatchTerms(); numOperands = numel(operands); @@ -1389,7 +1386,7 @@ function close_doc(database_obj, file_obj) if strcmp(op, 'or') continue % a negated OR (a plain one was flattened) end - if did.database.isBruteForceDependsOn(operand) + if did.database.isBruteForceTerm(operand) continue % no SQL form: brute_force_search end [isId, id] = did.database.baseIdLookupValues(operand); @@ -1445,7 +1442,7 @@ function close_doc(database_obj, file_obj) % the ones that did.datastructures.fieldsearch matches against the % search structure of QUERY_STRUCT (see did.query/to_searchstructure). % Used for the queries that SQL cannot match exactly (see - % isBruteForceDependsOn), as DID-python does. + % isBruteForceTerm), as DID-python does. if nargin > 3 && iscell(candidate_ids) ids = candidate_ids; else @@ -2374,6 +2371,28 @@ function validate_field_type_and_value(database_obj, doc_name, field_name, value isempty(did.database.dependsOnSqlMatch(query_struct.param1, query_struct.param2)); end % isBruteForceDependsOn() + function tf = isBruteForceTerm(query_struct) + % isBruteForceTerm - is a search term tested by brute force? + % + % TF = isBruteForceTerm(QUERY_STRUCT) + % + % True if QUERY_STRUCT (a single search structure) is searched + % by brute_force_search - its documents tested one by one with + % did.datastructures.fieldsearch - rather than in SQL, as + % DID-python searches it: + % - hasmember and ~hasmember, always. Their members are often + % those of an array or cell, which doc2sql stores only as a + % summary of its size and class ('[1 3] double'), so SQL + % cannot see them (issue #236). + % - the depends_on queries SQL cannot match exactly (see + % isBruteForceDependsOn). + % search_doc_ids searches these last in an AND, so they test + % only the documents the other terms leave. + op = strtrim(lower(char(query_struct.operation))); + op(op=='~') = ''; + tf = strcmp(op, 'hasmember') || did.database.isBruteForceDependsOn(query_struct); + end % isBruteForceTerm() + function [tf, ids] = baseIdLookupValues(query_struct) % baseIdLookupValues - is a search structure a lookup by base.id? % diff --git a/tests/+did/+unittest/TestBatchedOr.m b/tests/+did/+unittest/TestBatchedOr.m index 3a58636b..14b0d46c 100644 --- a/tests/+did/+unittest/TestBatchedOr.m +++ b/tests/+did/+unittest/TestBatchedOr.m @@ -78,14 +78,13 @@ function restoreDefaults(testCase) did.query('demoA.value', 'lessthaneq', 3) did.query('demoB.value', 'greaterthan', 138) did.query('demoB.value', 'greaterthaneq', 138) - did.query('demoC.value', 'hasmember', 205) did.query('demoB', 'hasfield') did.query('', 'depends_on', 'item1', testCase.idsA{4}) did.query('base.name', 'regexp', '^c2') did.query('', 'isa', 'demoB') did.query('', 'isa', 'demoC') }; - negatable = terms([1:9 11:14]); + negatable = terms([1:8 10:13]); for k = 1:numel(negatable) s = negatable{k}.searchstructure; s.operation = ['~' s.operation]; @@ -255,6 +254,22 @@ function testBruteForceDependsOnInAnOr(testCase) end end + function testBruteForceHasMemberInAnOr(testCase) + % hasmember is always searched by brute_force_search (see + % did.database/isBruteForceTerm), on its own, not in a batch + member = did.query('demoC.value', 'hasmember', 205); + notMember = member.searchstructure; + notMember.operation = '~hasmember'; + for term = {member, notMember} + s = testCase.toStruct(term{1}); + q = testCase.orOf({term{1}, did.query('base.name', 'exact_string', 'n3'), ... + did.query('', 'isa', 'demoB')}); + testCase.verifyBatchedMatchesPerOperand(q, [s.operation ' in an OR']); + q2 = [testCase.toStruct(did.query('', 'isa', 'demoC')), q]; + testCase.verifyBatchedMatchesPerOperand(q2, [s.operation ' in an OR inside an AND']); + end + end + function testWithoutSearchIndexes(testCase) % Without the indexes there are no candidates and no batched % id lookups; the batch is a UNION over the whole branch diff --git a/tests/+did/+unittest/TestHasMemberQuoting.m b/tests/+did/+unittest/TestHasMemberQuoting.m new file mode 100644 index 00000000..8f0ddefb --- /dev/null +++ b/tests/+did/+unittest/TestHasMemberQuoting.m @@ -0,0 +1,318 @@ +classdef TestHasMemberQuoting < matlab.unittest.TestCase + % Test 'hasmember': one rule, searched by brute force, never as SQL + % + % did.database/query_struct_to_sql_str put a text 'hasmember' value + % into the SQL unquoted, so 'name17' was read as a column (an error), + % 'value' as the column doc_data.value (every document matched) and + % '1 OR 1=1' as SQL (SQL injection). And each implementation had its + % own rule: did.datastructures.fieldsearch used ismember, which + % compares two texts character by character; the SQL matched an item + % of a comma-separated list, and could not see the members of arrays + % and cells at all (doc2sql stores them as '[1 3] double'); DID-python + % tested for a substring. Now hasmember has one rule (see fieldsearch): + % - text is a member of text that is the whole value or an item of + % its comma-separated list; + % - anything is a member of a cell array that holds an equal element, + % and a number of a numeric array that holds it; + % - text is never a member of a number, nor a number of text; + % and the SQLite search tests it by brute force with fieldsearch (see + % did.database/isBruteForceTerm), as DID-python does, so no hasmember + % value goes into SQL. + % + % testRule checks fieldsearch against a table of cases (the same table + % as DID-python's tests/test_hasmember_quoting.py). The other tests + % compare db.search with fieldsearch for text values, numbers, array + % and cell fields and '~hasmember', on their own, inside an AND (only + % the candidates are tested) and inside an OR. See issue #236. + + properties (Constant) + db_filename = 'test_db_hasmember.sqlite' + names = {'value', '1 OR 1=1', 'say "hi"', 'x, value', 'alpha', 'n7', 'O''Brien', '5'} + end + + properties + db % did.test.helper.QueryLogSqlitedb + docs % the did.documents added, in order + end + + properties (TestParameter) + textValue = {'value', '1 OR 1=1', 'say "hi"', 'O''Brien', 'x, value', 'name17', 'zzz', '5', ''} + numberValue = {3, 5, 99} + end + + methods (TestClassSetup) + function setupClass(testCase) + testCase.applyFixture(matlab.unittest.fixtures.WorkingFolderFixture); + testCase.applyFixture(did.test.fixture.PathConstantFixture); + + testCase.db = did.test.helper.QueryLogSqlitedb(testCase.db_filename); + testCase.db.add_branch('a'); + testCase.docs = cell(1, numel(testCase.names)); + for i = 1:numel(testCase.names) + testCase.docs{i} = did.document('demoA', 'base.name', testCase.names{i}, ... + 'demoA.value', i); + end + testCase.db.add_docs(testCase.docs, 'a', 'Validate', false); + end + end + + methods + function ids = expectedIds(testCase, s) + % The ids of the documents that fieldsearch matches with S + ids = {}; + for k = 1:numel(testCase.docs) + if did.datastructures.fieldsearch(testCase.docs{k}.document_properties, s) + ids{end+1} = testCase.docs{k}.id(); %#ok + end + end + ids = sort(ids(:)); + end + + function verifySearch(testCase, s, msg) + found = testCase.db.search(s); + testCase.verifyClass(found, 'cell', msg); + testCase.verifyEqual(sort(found(:)), testCase.expectedIds(s), msg); + end + + function verifyIds(testCase, s, docIndexes, msg) + % The search and fieldsearch must both find the documents + % DOCINDEXES, no more + expected = cellfun(@(d) d.id(), testCase.docs(docIndexes), 'UniformOutput', false); + testCase.verifyEqual(testCase.expectedIds(s), sort(expected(:)), [msg ': fieldsearch']); + testCase.verifySearch(s, msg); + end + + function s = hasMember(~, field, value, negate) + s = did.query(field, 'hasmember', value).searchstructure; + if negate + s.operation = '~hasmember'; + end + end + + function s = orOf(~, terms) + % Left-nested OR of search structures, as q = q | ... builds it + s = terms{1}; + for k = 2:numel(terms) + s = struct('field', '', 'operation', 'or', 'param1', s, 'param2', terms{k}); + end + end + + function s = allDocs(~) + % A term that matches every document, and that search_doc_ids + % searches before hasmember (fieldsearch has no 'isa') + s = did.query('demoA.value', 'lessthan', 100).searchstructure; + end + + function numQueries = countDocQueries(testCase) + % The SQL searches run since the query log was cleared + entries = testCase.db.queryLog; + numQueries = nnz(startsWith({entries.query}, 'SELECT DISTINCT docs.doc_id')); + end + + function verifyLoadedOnly(testCase, numDocs, msg) + % Since the query log was cleared, NUMDOCS documents were + % loaded, and the documents of the branch were not listed + entries = testCase.db.queryLog; + queries = {entries.query}; + testCase.verifyFalse(any(startsWith(queries, 'SELECT docs.doc_id FROM docs')), ... + [msg ': the branch must not be listed']); + testCase.verifyEqual(nnz(startsWith(queries, 'SELECT json_code FROM docs')), numDocs, ... + [msg ': documents loaded']); + end + end + + methods (Test) + function testRule(testCase) + % {value, member, is a member}; the same table as DID-python's + cases = { + 'value', 'value', true + 'x, value', 'value', true + 'x,value ,y', 'value', true + 'valuex', 'value', false + 'value', 'x, value', false + 'eulav', 'value', false % ismember matched this + 'a.b', '.', false % the member is not a pattern + '', '', true + 5, 5, true + [1 2 3], 2, true + [1 2 3], 4, false + true, 1, true + {'a', 'b'}, 'b', true + {'a', 'b'}, 'a,b', false + {1, 'a'}, 1, true + '5', 5, false + 5, '5', false + 'a', 97, false % ismember matched this + struct('a',1), 'a', false + }; + for k = 1:size(cases, 1) + [value, member, isMember] = cases{k, :}; + A = struct('f', {value}); + s = struct('field', 'f', 'operation', 'hasmember', 'param1', member, 'param2', ''); + testCase.verifyEqual(logical(did.datastructures.fieldsearch(A, s)), isMember, ... + sprintf('case %d: hasmember', k)); + s.operation = '~hasmember'; + testCase.verifyEqual(logical(did.datastructures.fieldsearch(A, s)), ~isMember, ... + sprintf('case %d: ~hasmember', k)); + end + end + + function testText(testCase, textValue) + for negate = [false true] + s = testCase.hasMember('base.name', textValue, negate); + testCase.verifySearch(s, sprintf('%s ''%s''', s.operation, textValue)); + end + end + + function testNumber(testCase, numberValue) + % doc2sql stores demoA.value as a number + for negate = [false true] + s = testCase.hasMember('demoA.value', numberValue, negate); + testCase.verifySearch(s, sprintf('%s %g', s.operation, numberValue)); + end + s = testCase.hasMember('demoA.value', numberValue, false); + testCase.verifyNumElements(testCase.db.search(s), ... + double(numberValue <= numel(testCase.names)), ... + sprintf('hasmember %g must match the one document holding it', numberValue)); + end + + function testExpectedDocuments(testCase) + testCase.verifyIds(testCase.hasMember('base.name', '1 OR 1=1', false), 2, ... + 'hasmember ''1 OR 1=1'''); + testCase.verifyIds(testCase.hasMember('base.name', 'value', false), [1 4], ... + 'hasmember ''value'' (the name and the list holding it)'); + testCase.verifyIds(testCase.hasMember('base.name', 'value', true), [2 3 5 6 7 8], ... + '~hasmember ''value'''); + testCase.verifyIds(testCase.hasMember('base.name', 'O''Brien', false), 7, ... + 'hasmember ''O''''Brien'''); + end + + function testTextAndNumbersDoNotMix(testCase) + % base.name '5' is text; demoA.value 3 is a number + testCase.verifyIds(testCase.hasMember('base.name', 5, false), [], ... + 'hasmember 5 in the text ''5'''); + testCase.verifyIds(testCase.hasMember('base.name', '5', false), 8, ... + 'hasmember ''5'' in the text ''5'''); + testCase.verifyIds(testCase.hasMember('demoA.value', '3', false), [], ... + 'hasmember ''3'' in the number 3'); + testCase.verifyIds(testCase.hasMember('demoA.value', 3, false), 3, ... + 'hasmember 3 in the number 3'); + end + + function testNumbersNoValueHolds(testCase) + % Inf is written as 9e999 and NaN is a member of nothing; no + % document holds either, so ~hasmember matches every one + for member = {Inf, -Inf, NaN} + msg = sprintf('hasmember %g', member{1}); + testCase.verifyIds(testCase.hasMember('demoA.value', member{1}, false), [], msg); + testCase.verifyIds(testCase.hasMember('demoA.value', member{1}, true), ... + 1:numel(testCase.names), ['~' msg]); + end + end + + function testMissingField(testCase) + % No document has demoB.value: none holds a member of it, so + % ~hasmember matches every one (as in fieldsearch) + testCase.verifyIds(testCase.hasMember('demoB.value', 1, false), [], ... + 'hasmember in a missing field'); + testCase.verifyIds(testCase.hasMember('demoB.value', 1, true), ... + 1:numel(testCase.names), '~hasmember in a missing field'); + end + + function testArrayAndCellFields(testCase) + % doc2sql stores these as a summary ('[1 3] double', '[1 2] + % cell'), which SQL could not search; brute force can + testCase.applyFixture(matlab.unittest.fixtures.WorkingFolderFixture); + db = did.implementations.sqlitedb('list_fields.sqlite'); + db.add_branch('a'); + docs = {did.document('demoA', 'base.name', 'numbers', 'demoA.value', [1 2 3]), ... + did.document('demoA', 'base.name', 'other', 'demoA.value', [4 5]), ... + did.document('demoA', 'base.name', 'names', 'demoA.value', {'alpha', 'beta'})}; + db.add_docs(docs, 'a', 'Validate', false); + ids = cellfun(@(d) d.id(), docs, 'UniformOutput', false); + stored = db.get_docs(ids); % the documents as the search sees them + cases = { + 'demoA.value', 2, 1 + 'demoA.value', 5, 2 + 'demoA.value', 6, [] + 'demoA.value', 'beta', 3 + 'demoA.value', 'gamma', [] + 'base.name', 'other', 2 + }; + for k = 1:size(cases, 1) + [field, member, matching] = cases{k, :}; + for negate = [false true] + s = testCase.hasMember(field, member, negate); + if negate + expected = ids(setdiff(1:3, matching)); + else + expected = ids(matching); + end + expected = sort(expected(:)); + msg = sprintf('%s %s in %s', s.operation, num2str(member), field); + found = db.search(s); + testCase.verifyEqual(sort(found(:)), expected, msg); + fieldsearchIds = cell(0, 1); + for j = 1:numel(stored) + if did.datastructures.fieldsearch(stored(j).document_properties, s) + fieldsearchIds{end+1, 1} = stored(j).id(); %#ok + end + end + testCase.verifyEqual(sort(fieldsearchIds), expected, [msg ': fieldsearch']); + end + end + end + + function testStructField(testCase) + % A struct (base) has no members: its fields are not searched + testCase.verifyIds(testCase.hasMember('base', 'value', false), [], ... + 'hasmember ''value'' in the struct base'); + end + + function testInAnAnd(testCase, textValue) + % 'demoA.value < 4' is searched first and leaves 3 candidates, + % so hasmember loads and tests those 3 documents only + firstThree = did.query('demoA.value', 'lessthan', 4).searchstructure; + for negate = [false true] + s = [firstThree, testCase.hasMember('base.name', textValue, negate)]; + msg = sprintf('demoA.value < 4 AND %s ''%s''', s(2).operation, textValue); + testCase.db.clearQueryLog(); + testCase.verifySearch(s, msg); + testCase.verifyLoadedOnly(3, msg); + end + for negate = [false true] + s = [firstThree, testCase.hasMember('demoA.value', 2, negate)]; + testCase.verifySearch(s, sprintf('demoA.value < 4 AND %s 2', s(2).operation)); + end + end + + function testInAnOr(testCase) + terms = {testCase.hasMember('base.name', 'value', false), ... + testCase.hasMember('base.name', '1 OR 1=1', false), ... + testCase.hasMember('base.name', 'say "hi"', false), ... + testCase.hasMember('base.name', 'O''Brien', true), ... + testCase.hasMember('demoA.value', 6, false), ... + testCase.hasMember('demoA.value', 2, true)}; + for k = 1:numel(terms) + others = terms([1:k-1 k+1:end]); + s = testCase.orOf([terms(k), {did.query('base.name', 'exact_string', 'alpha').searchstructure}]); + testCase.verifySearch(s, sprintf('%s ''%s'' OR ...', terms{k}.operation, ... + num2str(terms{k}.param1))); + s = testCase.orOf([terms(k) others(1)]); + testCase.verifySearch(s, sprintf('%s ''%s'' OR another hasmember', ... + terms{k}.operation, num2str(terms{k}.param1))); + end + + % every operand is brute-forced, so no SQL search is run + s = testCase.orOf(terms(1:3)); + testCase.db.clearQueryLog(); + testCase.verifySearch(s, 'an OR of text hasmembers'); + testCase.verifyEqual(testCase.countDocQueries(), 0, ... + 'hasmember must not be searched in SQL'); + + % ...and restricted to candidates, inside an AND + s = [testCase.allDocs(), testCase.orOf(terms)]; + testCase.verifySearch(s, 'demoA.value < 100 AND an OR of hasmembers'); + end + end +end