From e7bd0e2899039dfba43b42198cdcc65067d7a19b Mon Sep 17 00:00:00 2001 From: scottdowne Date: Tue, 24 Sep 2013 17:46:48 -0400 Subject: [PATCH 1/4] [#919710] Ensure only published makes are returned from general search, and add an authenticated search for authenticated apps. --- lib/models/make.js | 2 + lib/queryBuilder.js | 266 +++++++++++++++++++++++--------------------- routes/index.js | 1 + routes/make.js | 19 ++++ server.js | 1 + 5 files changed, 164 insertions(+), 125 deletions(-) diff --git a/lib/models/make.js b/lib/models/make.js index 68a8a8a..27e4322 100644 --- a/lib/models/make.js +++ b/lib/models/make.js @@ -92,6 +92,8 @@ module.exports = function( environment, mongoInstance ) { published: { type: Boolean, "default": true, + es_type: "boolean", + es_indexed: true, es_index: "not_analyzed" }, tags: { diff --git a/lib/queryBuilder.js b/lib/queryBuilder.js index 58cad18..00ed106 100644 --- a/lib/queryBuilder.js +++ b/lib/queryBuilder.js @@ -136,147 +136,163 @@ module.exports = function( loginApi ) { } }; - // capture valid query generator keys - GENERATOR_KEYS = Object.keys( generators ); - - return { - build: function( query, callback ) { - if ( !( query && query.constructor === Object ) || !( callback && typeof callback === "function" ) ) { - throw new Error( "Check your arguments." ); - } + function buildQuery( query, customFilter, callback ) { + if ( !( query && query.constructor === Object ) || !( callback && typeof callback === "function" ) ) { + throw new Error( "Check your arguments." ); + } - query.limit = +query.limit; - query.page = +query.page; - - // baseQuery is the most basic query we can make. - // advancedQuery is used if we need to generate filters, and wraps around baseQuery. - var baseQuery = { - query: { - filtered: { - query: { - match_all: {} - }, - filter: { - missing: { - field: "deletedAt", - null_value: true - } - } - } - } - }, - advancedQuery = { - query: { - filtered: { - filter: { - bool: { - must: [], - should: [] - } - }, - query: baseQuery.query - } + query.limit = +query.limit; + query.page = +query.page; + + // baseQuery is the most basic query we can make. + // advancedQuery is used if we need to generate filters, and wraps around baseQuery. + var baseQuery = { + query: { + filtered: { + query: { + match_all: {} + }, + filter: customFilter || {} } - }, - searchQuery = {}, - size = query.limit && isFinite( query.limit ) ? query.limit : DEFAULT_SEARCH_SIZE, - page = query.page && isFinite( query.page ) ? query.page : 1, - user = query.user, - sort = query.sortByField, - filterOccurence = query.or ? "should" : "must", - sortObj, - notRegexMatch; - - // If the request contains any of the filter generating keys, or defines a user search, use the advancedQuery object - if ( Object.keys( query ).some( hasGeneratorKey ) || user ) { - searchQuery = advancedQuery; - Object.keys( query ).forEach(function( key ){ - value = query[ key ]; - if ( generators[ key ] ) { - notRegexMatch = NOT_REGEX.exec( value ); - if ( notRegexMatch ) { - searchQuery.query.filtered.filter.bool[ filterOccurence ].push( generators[ key ]( notRegexMatch[ 1 ], true ) ); - } else { - searchQuery.query.filtered.filter.bool[ filterOccurence ].push( generators[ key ]( value ) ); + } + }, + advancedQuery = { + query: { + filtered: { + filter: { + bool: { + must: [], + should: [] + } + }, + query: baseQuery.query } } - }); - } else { - searchQuery = baseQuery; - } + }, + searchQuery = {}, + size = query.limit && isFinite( query.limit ) ? query.limit : DEFAULT_SEARCH_SIZE, + page = query.page && isFinite( query.page ) ? query.page : 1, + user = query.user, + sort = query.sortByField, + filterOccurence = query.or ? "should" : "must", + sortObj, + notRegexMatch; + + // If the request contains any of the filter generating keys, or defines a user search, use the advancedQuery object + if ( Object.keys( query ).some( hasGeneratorKey ) || user ) { + searchQuery = advancedQuery; + Object.keys( query ).forEach(function( key ){ + value = query[ key ]; + if ( generators[ key ] ) { + notRegexMatch = NOT_REGEX.exec( value ); + if ( notRegexMatch ) { + searchQuery.query.filtered.filter.bool[ filterOccurence ].push( generators[ key ]( notRegexMatch[ 1 ], true ) ); + } else { + searchQuery.query.filtered.filter.bool[ filterOccurence ].push( generators[ key ]( value ) ); + } + } + }); + } else { + searchQuery = baseQuery; + } - // set size and from and sort - if ( size > MAX_SEARCH_SIZE ) { - size = MAX_SEARCH_SIZE; - } else if ( size < 1 ) { - size = 1; - } - searchQuery.size = size; + // set size and from and sort + if ( size > MAX_SEARCH_SIZE ) { + size = MAX_SEARCH_SIZE; + } else if ( size < 1 ) { + size = 1; + } + searchQuery.size = size; - if ( page < 1 ) { - page = 1; - } - searchQuery.from = ( page - 1 ) * size; + if ( page < 1 ) { + page = 1; + } + searchQuery.from = ( page - 1 ) * size; - if ( sort ) { - sort = ( Array.isArray( sort ) ? sort : [ sort ] ).filter(function( pair ) { - return typeof pair === "string" && pair.length && VALID_SORT_FIELDS.indexOf( pair.split( "," )[ 0 ] ) !== -1; - }); + if ( sort ) { + sort = ( Array.isArray( sort ) ? sort : [ sort ] ).filter(function( pair ) { + return typeof pair === "string" && pair.length && VALID_SORT_FIELDS.indexOf( pair.split( "," )[ 0 ] ) !== -1; + }); - if ( sort.length ) { - searchQuery.sort = []; - sort.forEach(function( pair ){ - pair = pair.split( "," ); - sortObj = {}; - if ( pair[ 0 ] === "likes" ) { - sortObj._script = { - lang: "js", - order: pair[ 1 ], - script: "doc['likes.userId'].values.length", - type: "number" - }; - } else { - sortObj[ pair[ 0 ] ] = pair[ 1 ] || "desc"; - } - searchQuery.sort.push( sortObj ); - }); - } + if ( sort.length ) { + searchQuery.sort = []; + sort.forEach(function( pair ){ + pair = pair.split( "," ); + sortObj = {}; + if ( pair[ 0 ] === "likes" ) { + sortObj._script = { + lang: "js", + order: pair[ 1 ], + script: "doc['likes.userId'].values.length", + type: "number" + }; + } else { + sortObj[ pair[ 0 ] ] = pair[ 1 ] || "desc"; + } + searchQuery.sort.push( sortObj ); + }); } + } - if ( user ) { - notRegexMatch = NOT_REGEX.exec( user ); - if ( notRegexMatch ) { - user = notRegexMatch[ 1 ]; + if ( user ) { + notRegexMatch = NOT_REGEX.exec( user ); + if ( notRegexMatch ) { + user = notRegexMatch[ 1 ]; + } + loginApi.getUser( user, function( err, userData ) { + if ( err ) { + callback({ + error: err, + code: 500 + }); + return; } - loginApi.getUser( user, function( err, userData ) { - if ( err ) { - callback({ - error: err, - code: 500 - }); - return; - } - if ( !userData ) { - if ( searchQuery.query.filtered.filter.bool.should.length ) { - // If this is an OR filtered query, ignore the undefined user - callback( null, searchQuery ); - } else { - callback( { code: 404 } ); - } - return; + if ( !userData ) { + if ( searchQuery.query.filtered.filter.bool.should.length ) { + // If this is an OR filtered query, ignore the undefined user + callback( null, searchQuery ); + } else { + callback( { code: 404 } ); } + return; + } - var filter = generateFilter( "term", { - email: userData.email - }, !!notRegexMatch ); + var filter = generateFilter( "term", { + email: userData.email + }, !!notRegexMatch ); - searchQuery.query.filtered.filter.bool[ filterOccurence ].push( filter ); - callback( null, searchQuery ); - }); - } else { + searchQuery.query.filtered.filter.bool[ filterOccurence ].push( filter ); callback( null, searchQuery ); - } + }); + } else { + callback( null, searchQuery ); + } + } + + // capture valid query generator keys + GENERATOR_KEYS = Object.keys( generators ); + + return { + build: function( query, callback ) { + buildQuery( query, { + and: [ + { + missing: { + field: "deletedAt", + null_value: true + } + }, + { + term: { + published: true + } + } + ] + }, callback ); + }, + authenticatedSearch: function( query, callback ) { + buildQuery( query, {}, callback ); } }; }; diff --git a/routes/index.js b/routes/index.js index 38da47d..a08762d 100755 --- a/routes/index.js +++ b/routes/index.js @@ -11,6 +11,7 @@ module.exports = function routesCtor( makeModel, apiUserModel, env ) { res.render( "index.html" ); }, search: makeRoutes.search, + authenticatedSearch: makeRoutes.authenticatedSearch, create: makeRoutes.create, update: makeRoutes.update, remove: makeRoutes.remove, diff --git a/routes/make.js b/routes/make.js index 980c30b..e035f04 100644 --- a/routes/make.js +++ b/routes/make.js @@ -157,6 +157,25 @@ module.exports = function( makeModel, env ) { doSearch( req, res, dsl ); }); }, + authenticatedSearch: function( req, res ) { + + if ( !req.query ) { + return searchError( res, "Malformed Request", 400 ); + } + + queryBuilder.authenticatedSearch( req.query, function( err, dsl ) { + if ( err ) { + if ( err.code === 404 ) { + // No user was found, no makes to search. + metrics.increment( "make.search.success" ); + return res.json( { makes: [], total: 0 } ); + } else { + return searchError( res, err, err.code ); + } + } + doSearch( req, res, dsl ); + }); + }, searchTest: function( req, res ) { res.render( "search.html" ); }, diff --git a/server.js b/server.js index b8b19f8..048b421 100755 --- a/server.js +++ b/server.js @@ -95,6 +95,7 @@ app.put( "/api/20130724/make/like/:id", middleware.hawkAuth, Mongo.isDbOnline, m app.put( "/api/20130724/make/unlike/:id", middleware.hawkAuth, Mongo.isDbOnline, middleware.getMake, middleware.unlike, routes.update ); app.del( "/api/20130724/make/:id", middleware.hawkAuth, Mongo.isDbOnline, middleware.getMake, routes.remove ); app.get( "/api/20130724/make/search", Mongo.isDbOnline, middleware.crossOrigin, routes.search ); +app.get( "/api/20130724/make/authenticatedSearch", middleware.hawkAuth, Mongo.isDbOnline, middleware.crossOrigin, routes.authenticatedSearch ); // 20130724 Admin API routes app.put( "/admin/api/20130724/make/:id", csrfMiddleware, middleware.collabAuth, middleware.fieldFilter, Mongo.isDbOnline, middleware.getMake, routes.update ); From ed2adeca1b418e78db76206c667ee4e121f18772 Mon Sep 17 00:00:00 2001 From: scottdowne Date: Fri, 27 Sep 2013 11:44:28 -0400 Subject: [PATCH 2/4] [#919710] Updating tests. --- test/queryBuilder/core.unit.js | 19 +++++++++++++++---- test/queryBuilder/sort.unit.js | 17 +++++++++++++---- 2 files changed, 28 insertions(+), 8 deletions(-) diff --git a/test/queryBuilder/core.unit.js b/test/queryBuilder/core.unit.js index 6bb0509..1d33d22 100644 --- a/test/queryBuilder/core.unit.js +++ b/test/queryBuilder/core.unit.js @@ -7,10 +7,19 @@ module.exports = function( qb ){ match_all: {} }, filter: { - missing: { - field: "deletedAt", - null_value: true - } + and: [ + { + missing: { + field: "deletedAt", + null_value: true + } + }, + { + term: { + published: true + } + } + ] } } }, @@ -62,6 +71,8 @@ module.exports = function( qb ){ assert.strictEqual( err, null ); }); it( "built query should match the defined base query", function() { +console.log("query", query); +console.log("baseQuery", baseQuery); assert.deepEqual( query, baseQuery ); }); }); diff --git a/test/queryBuilder/sort.unit.js b/test/queryBuilder/sort.unit.js index 9db965d..0f10b3e 100644 --- a/test/queryBuilder/sort.unit.js +++ b/test/queryBuilder/sort.unit.js @@ -7,10 +7,19 @@ module.exports = function( qb ) { match_all: {} }, filter: { - missing: { - field: "deletedAt", - null_value: true - } + and: [ + { + missing: { + field: "deletedAt", + null_value: true + } + }, + { + term: { + published: true + } + } + ] } } }, From df118fcb7debdaef69c0b303ab819c975f0f66be Mon Sep 17 00:00:00 2001 From: scottdowne Date: Mon, 30 Sep 2013 07:47:46 -0400 Subject: [PATCH 3/4] [#919710] Removed authenticated search and going to do that in another ticket. --- lib/queryBuilder.js | 3 --- routes/index.js | 1 - routes/make.js | 19 ------------------- server.js | 1 - test/queryBuilder/core.unit.js | 2 -- 5 files changed, 26 deletions(-) diff --git a/lib/queryBuilder.js b/lib/queryBuilder.js index 00ed106..227fa73 100644 --- a/lib/queryBuilder.js +++ b/lib/queryBuilder.js @@ -290,9 +290,6 @@ module.exports = function( loginApi ) { } ] }, callback ); - }, - authenticatedSearch: function( query, callback ) { - buildQuery( query, {}, callback ); } }; }; diff --git a/routes/index.js b/routes/index.js index a08762d..38da47d 100755 --- a/routes/index.js +++ b/routes/index.js @@ -11,7 +11,6 @@ module.exports = function routesCtor( makeModel, apiUserModel, env ) { res.render( "index.html" ); }, search: makeRoutes.search, - authenticatedSearch: makeRoutes.authenticatedSearch, create: makeRoutes.create, update: makeRoutes.update, remove: makeRoutes.remove, diff --git a/routes/make.js b/routes/make.js index e035f04..980c30b 100644 --- a/routes/make.js +++ b/routes/make.js @@ -157,25 +157,6 @@ module.exports = function( makeModel, env ) { doSearch( req, res, dsl ); }); }, - authenticatedSearch: function( req, res ) { - - if ( !req.query ) { - return searchError( res, "Malformed Request", 400 ); - } - - queryBuilder.authenticatedSearch( req.query, function( err, dsl ) { - if ( err ) { - if ( err.code === 404 ) { - // No user was found, no makes to search. - metrics.increment( "make.search.success" ); - return res.json( { makes: [], total: 0 } ); - } else { - return searchError( res, err, err.code ); - } - } - doSearch( req, res, dsl ); - }); - }, searchTest: function( req, res ) { res.render( "search.html" ); }, diff --git a/server.js b/server.js index 048b421..b8b19f8 100755 --- a/server.js +++ b/server.js @@ -95,7 +95,6 @@ app.put( "/api/20130724/make/like/:id", middleware.hawkAuth, Mongo.isDbOnline, m app.put( "/api/20130724/make/unlike/:id", middleware.hawkAuth, Mongo.isDbOnline, middleware.getMake, middleware.unlike, routes.update ); app.del( "/api/20130724/make/:id", middleware.hawkAuth, Mongo.isDbOnline, middleware.getMake, routes.remove ); app.get( "/api/20130724/make/search", Mongo.isDbOnline, middleware.crossOrigin, routes.search ); -app.get( "/api/20130724/make/authenticatedSearch", middleware.hawkAuth, Mongo.isDbOnline, middleware.crossOrigin, routes.authenticatedSearch ); // 20130724 Admin API routes app.put( "/admin/api/20130724/make/:id", csrfMiddleware, middleware.collabAuth, middleware.fieldFilter, Mongo.isDbOnline, middleware.getMake, routes.update ); diff --git a/test/queryBuilder/core.unit.js b/test/queryBuilder/core.unit.js index 1d33d22..7df7e5e 100644 --- a/test/queryBuilder/core.unit.js +++ b/test/queryBuilder/core.unit.js @@ -71,8 +71,6 @@ module.exports = function( qb ){ assert.strictEqual( err, null ); }); it( "built query should match the defined base query", function() { -console.log("query", query); -console.log("baseQuery", baseQuery); assert.deepEqual( query, baseQuery ); }); }); From fe9478019e245ecba9caa067922fd9ddde26edee Mon Sep 17 00:00:00 2001 From: scottdowne Date: Mon, 30 Sep 2013 13:33:43 -0400 Subject: [PATCH 4/4] review --- lib/queryBuilder.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/queryBuilder.js b/lib/queryBuilder.js index 227fa73..2a39165 100644 --- a/lib/queryBuilder.js +++ b/lib/queryBuilder.js @@ -137,6 +137,10 @@ module.exports = function( loginApi ) { }; function buildQuery( query, customFilter, callback ) { + if ( typeof customFilter === "function" ) { + callback = customFilter; + customFilter = {}; + } if ( !( query && query.constructor === Object ) || !( callback && typeof callback === "function" ) ) { throw new Error( "Check your arguments." ); } @@ -152,7 +156,7 @@ module.exports = function( loginApi ) { query: { match_all: {} }, - filter: customFilter || {} + filter: customFilter } } },