Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#919710] Ensure only published makes are returned from general search, ... #151

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+170 −133
Open

[#919710] Ensure only published makes are returned from general search, ... #151

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e7bd0e2
[#919710] Ensure only published makes are returned from general searc…
ScottDowne Sep 24, 2013
ed2adec
[#919710] Updating tests.
ScottDowne Sep 27, 2013
df118fc
[#919710] Removed authenticated search and going to do that in anothe…
ScottDowne Sep 30, 2013
fe94780
review
ScottDowne Sep 30, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/models/make.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand Down
266 changes: 141 additions & 125 deletions lib/queryBuilder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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" ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check that customFilter exists, or default it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you're defaulting it below when it's referenced for baseQuery - that should be fine, but perhaps you could shorthand it so that buildQuery( query, callback ) works alongside the three argument version. Worth it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree with this.

Why burden the calls that don't care about filters to have to worry about filters.

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 );
}
};
};
1 change: 1 addition & 0 deletions routes/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
19 changes: 19 additions & 0 deletions routes/make.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to use the hawkError function so that the server signs it's response with Hawk.

return hawkError( req, res, "Malformed Request", 400, "search" );

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the one below it, line 173 is fine? nvm, looks like you saw that a bit later.

}

queryBuilder.authenticatedSearch( req.query, function( err, dsl ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure to use HawkModule.respond( ... )

example: https://github.com/ScottDowne/MakeAPI/blob/ed2adeca1b418e78db76206c667ee4e121f18772/routes/make.js#L137-L138

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this applies to lines 171.

Line 173 should use hawkError

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 );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to figure out how to get doSearch to sign the response with hawk, without breaking regular search.

});
},
searchTest: function( req, res ) {
res.render( "search.html" );
},
Expand Down
1 change: 1 addition & 0 deletions server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
Expand Down
19 changes: 15 additions & 4 deletions test/queryBuilder/core.unit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
]
}
}
},
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove these.

assert.deepEqual( query, baseQuery );
});
});
Expand Down
17 changes: 13 additions & 4 deletions test/queryBuilder/sort.unit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
]
}
}
},
Expand Down