From 055989f4b531fcf58e9f11f04c665b758490e7f9 Mon Sep 17 00:00:00 2001 From: Aref Shafaei Date: Fri, 20 Sep 2024 16:46:29 -0700 Subject: [PATCH] fix parser issues related to multi-layered facets issues: - fix typos in ermrestCompactPath and _createParsedJoinFromStr - make sure Location constructor uses the correct join. the function was assuming that the first portion of the url always has a facet, but that might not be true. --- js/parser.js | 43 ++++++++++++++++++++-------- test/specs/parser/tests/01.parser.js | 16 ++++++++++- 2 files changed, 46 insertions(+), 13 deletions(-) diff --git a/js/parser.js b/js/parser.js index c7e0d587..efb9e560 100644 --- a/js/parser.js +++ b/js/parser.js @@ -268,14 +268,13 @@ } // pathParts: - var aliasJoinRegExp = /\$.*/, - joinRegExp = /(?:left|right|full|^)\((.*)\)=\((.*:.*:.*)\)/, + var joinRegExp = /(?:left|right|full|^)\((.*)\)=\((.*:.*:.*)\)/, facetsRegExp = /\*::facets::(.+)/, customFacetsRegExp = /\*::cfacets::(.+)/; - var schemaTable = parts[0], table = this._rootTableName, schema = this._rootSchemaName; - var self = this, pathParts = [], alias, match, prevJoin = false; - var facets, cfacets, filter, filtersString, searchTerm, join, joins = []; + var table = this._rootTableName, schema = this._rootSchemaName; + var self = this, pathParts = [], alias, match, prevJoin = false, startWithT1 = false, aliasNumber; + var facets, cfacets, filter, filtersString, join, joins = []; // go through each of the parts parts.forEach(function (part, index) { @@ -291,9 +290,23 @@ // there wasn't any join before, so this is the start of new path, // so we should create an object for the previous one. if (!prevJoin && index !== 1) { - // we're creating this for the previous section, so we should use the previous index - // Alias will be T, T1, T2, ... (notice we don't have T0) - alias = module._parserAliases.JOIN_TABLE_PREFIX + (pathParts.length > 0 ? pathParts.length : ""); + /** + * we're creating this alias for the previous section, so we should use the previous index + * Alias will be T, T1, T2, ... (notice we don't have T0) + */ + aliasNumber = pathParts.length; + + /** + * T is always preserved for the initial schema:table. if the first pathPart doesn't have any joins + * and has facet/filters, then it's for the schema:table and so we should start with T. + * but if has join, then it means that the schema:table didn't have any filter/facets. so we have to start from T1, + * and the rest of the parts should just add one more + */ + if (pathParts.length === 0 && joins.length > 0) { + startWithT1 = true; + } + if (startWithT1) aliasNumber++; + alias = module._parserAliases.JOIN_TABLE_PREFIX + (aliasNumber > 0 ? aliasNumber : ""); pathParts.push(new PathPart(alias, joins, schema, table, facets, cfacets, filter, filtersString)); filter = undefined; filtersString = undefined; cfacets = undefined; facets = undefined; join = undefined; joins = []; } @@ -672,7 +685,7 @@ }; }); - // no rightJoin: s:t/ + // no rightJoin: s:t/ if (rightJoinIndex === -1) { uri = self.rootTableAlias + ":="; if (self.rootSchemaName) { @@ -693,14 +706,14 @@ if (self.pathParts[i].joins.length > 0) { // since we're reversing, we have to make sure we're using the // alias of the previous pathpart - alias = i > 0 ? self.pathPart[i-1].alias : self.rootTableAlias; + alias = i > 0 ? self.pathParts[i-1].alias : self.rootTableAlias; uri += "/" + joinsStr(self.pathParts[i], alias, i, true); } } // if there was pathParts before facet with null, change back to the facet with null if (self.pathParts[rightJoinIndex].joins.length > 0) { - uri += "/$" + self.pathParts[i].alias; + uri += "/$" + self.pathParts[rightJoinIndex].alias; } } @@ -1403,6 +1416,10 @@ * * when there aren't any joins, this could be the schema:table, so we have to start with T. * but if there are joins, we can start with T1, as T alias is always the first schema:table. + * + * this is how we're using the alias property: + * - if it doesn't have any joins, it's the alias of the previous path or root (so facet/) + * - if it has join, it's the alias that we're using to name the projected table that the join represents. */ if (lastPart) { var aliasNumber = ""; @@ -1449,6 +1466,8 @@ * Container for the path part, It will have the following attributes: * - joins: an array of ParsedJoin objects. * - alias: The alias that the facets should refer to + * - if there aren't any joins, it's the alias of the previous path or root. + * - otherwise, it's the alias that we're using to name the projected table that the join represents. * - schema: The schema that the join ends up with * - table: the table that the joins end up with * - facets: the facets object @@ -1717,7 +1736,7 @@ * @returns {ParsedJoin} */ function _createParsedJoinFromStr (linking, table, schema) { - var fromSchemaTable = schema ? [table,schema].join(":") : table; + var fromSchemaTable = schema ? [schema,table].join(":") : table; var fromCols = linking[1].split(","); var toParts = linking[2].match(/([^:]*):([^:]*):([^\)]*)/); var toCols = toParts[3].split(","); diff --git a/test/specs/parser/tests/01.parser.js b/test/specs/parser/tests/01.parser.js index 59ed911d..7fd85386 100644 --- a/test/specs/parser/tests/01.parser.js +++ b/test/specs/parser/tests/01.parser.js @@ -691,11 +691,24 @@ exports.execute = function(options) { "ermrestCompactPath missmatch" ); }); + + it ("parser should handle not having any filter on the first layer.", function () { + uri = baseUri + `/(id)=(s:table2:id)/!(RID=%3A%3Anull%3A%3A&RCB=%3A%3Anull%3A%3A)/(id3)=(s:table3:id)/*::facets::${validBlob2}/(id4)=(s:table4:id)`; + location = options.ermRest.parse(uri); + expect(location).toBeDefined("location is not defined"); + expect(location.uri).toEqual(uri, "uri missmatch"); + expect(location.ermrestCompactPath).toEqual( + "T:=parse_schema:parse_table/T1:=(id)=(s:table2:id)/!(RID=%3A%3Anull%3A%3A&RCB=%3A%3Anull%3A%3A)/T2:=(id3)=(s:table3:id)/accession=2/$T2/some-other-column::ciregexp::test2/$T2/M:=(id4)=(s:table4:id)", + "ermrestCompactPath missmatch" + ); + }); }); - // relies on the previous test describe("for changing facets in location, ", function () { it("Location.facets setter should be able to change the facet and update other APIs.", function() { + uri = baseUri + "/*::facets::" + validBlob + "/some_col=v1/(id)=(s:otherTable:id)/(id2)=(s:otherTable2:id)" + + "/*::facets::" + validBlob2 + "/some_col2=v2/(id)=(s:yetAnotherTable:id)" + "/*::facets::" + validBlob3; + location = options.ermRest.parse(uri); location.facets = facetObj; uri = baseUri + "/*::facets::" + validBlob + "/some_col=v1/(id)=(s:otherTable:id)/(id2)=(s:otherTable2:id)" + @@ -1536,6 +1549,7 @@ exports.execute = function(options) { // {"and": [ {"source": "column"} ]} expectError("N4IghgdgJiBcDaoDOB7ArgJwMYFM4ixQBs0BbCEAXwF1Kg"); }); + }); });