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.

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

#Refocus get /subjects filtering by tag working #68

Merged
merged 3 commits into from
Oct 24, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions api/v1/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ module.exports = {
SEQ_DEFAULT_SCOPE: 'defaultScope',
SEQ_DESC: 'DESC',
SEQ_LIKE: '$iLike',
SEQ_CONTAINS: '$contains',
SEQ_OR: '$or',
SEQ_WILDCARD: '%',
SLASH: '/',
Expand Down
1 change: 1 addition & 0 deletions api/v1/helpers/nouns/subjects.js
Original file line number Diff line number Diff line change
Expand Up @@ -304,4 +304,5 @@ module.exports = {
fieldsWithJsonArrayType,
fieldsWithArrayType,
loggingEnabled,
tagFilterName: 'tags',
}; // exports
70 changes: 52 additions & 18 deletions api/v1/helpers/verbs/findUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@
const u = require('./utils');
const constants = require('../../constants');
const defaults = require('../../../../config').api.defaults;
const conf = require('../../../../config');

const env = conf.environment[conf.nodeEnv];
const filterSubjByTags = env.filterSubjByTags;

/**
* Escapes all percent literals so they're not treated as wildcards.
Expand Down Expand Up @@ -52,10 +56,17 @@ function toSequelizeWildcards(val) {
* case-insensitive string matching
*
* @param {String} val - The value to transform into a Sequelize where clause
* @param {Object} props - The helpers/nouns module for the given DB model.
* @returns {Object} a Sequelize where clause using "$ilike" for
* case-insensitive string matching
*/
function toWhereClause(val) {
function toWhereClause(val, props) {
if (filterSubjByTags && Array.isArray(val) && props.tagFilterName) {
const containsClause = {};
containsClause[constants.SEQ_CONTAINS] = val;
return containsClause;
}

// TODO handle non-string data types like dates and numbers
if (typeof val !== 'string') {
return val;
Expand All @@ -72,9 +83,10 @@ function toWhereClause(val) {
* a Sequelize "where" object.
*
* @param {Object} filter
* @param {Object} props - The helpers/nouns module for the given DB model.
* @returns {Object} a Sequelize "where" object
*/
function toSequelizeWhere(filter) {
function toSequelizeWhere(filter, props) {
const where = {};
const keys = Object.keys(filter);
for (let i = 0; i < keys.length; i++) {
Expand All @@ -85,23 +97,34 @@ function toSequelizeWhere(filter) {
}

const values = [];
for (let j = 0; j < filter[key].length; j++) {
const v = filter[key][j];
if (typeof v === 'boolean') {
values.push(v);
} else if (typeof v === 'string') {
const arr = v.split(constants.COMMA);
for (let k = 0; k < arr.length; k++) {
values.push(toWhereClause(arr[k]));

// if tag filter is enabled and key is "tags", then create a contains
// clause and add it to where clause,
// like, { where : { '$contains': ['tag1', 'tag2'] } }
if (filterSubjByTags && props.tagFilterName &&
key === props.tagFilterName) {
const tagArr = filter[key];
values.push(toWhereClause(tagArr, props));
where[key] = values[0];
} else {
for (let j = 0; j < filter[key].length; j++) {
const v = filter[key][j];
if (typeof v === 'boolean') {
values.push(v);
} else if (typeof v === 'string') {
const arr = v.split(constants.COMMA);
for (let k = 0; k < arr.length; k++) {
values.push(toWhereClause(arr[k]));
}
}
}
}

if (values.length === 1) {
where[key] = values[0];
} else if (values.length > 1) {
where[key] = {};
where[key][constants.SEQ_OR] = values;
if (values.length === 1) {
where[key] = values[0];
} else if (values.length > 1) {
where[key] = {};
where[key][constants.SEQ_OR] = values;
}
}
}
}
Expand Down Expand Up @@ -165,14 +188,25 @@ function options(params, props) {
const keys = Object.keys(params);
for (let i = 0; i < keys.length; i++) {
const key = keys[i];
const isFilterField = constants.NOT_FILTER_FIELDS.indexOf(key) < 0;

let isFilterField;
if (filterSubjByTags) {
isFilterField = constants.NOT_FILTER_FIELDS.indexOf(key) < 0;
} else {
// if filterSubjByTags is disabled and we do v1/subjects?tags=mytag, we
// get error because isFilterField resolves to true becuase of swagger
// changes, and I cannot apply feature flag to swagger.
isFilterField = constants.NOT_FILTER_FIELDS.indexOf(key) < 0 &&
key !== props.tagFilterName;
}

if (isFilterField && params[key].value !== undefined) {
filter[key] = params[key].value;
}
}

if (filter) {
opts.where = toSequelizeWhere(filter);
opts.where = toSequelizeWhere(filter, props);
}

return opts;
Expand Down
11 changes: 11 additions & 0 deletions api/v1/swagger.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3094,6 +3094,17 @@ paths:
Filter by parentAbsolutePath; asterisk (*) wildcards ok.
required: false
type: string
-
name: tags
in: query
items:
type: string
maxLength: 60
Copy link
Contributor

Choose a reason for hiding this comment

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

Why 60 here? (Might need to allow for longer since you could build up a list of includes or excludes.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Each tag length is max 60 char long, as restricted by model.
Also, the changes in this PR only handles includes at sql level like v1/subjects?tags=mytag.
Do we need excludes here as well? I assumed the story was about includes only to get trust data working. I can create another PR if we need excludes as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok we can get this through as is, but i think we want to follow up with story to allow v1/subjects?tags=tag1,tag2,tag3 or v1/subjects?tags=-tag10,tag11 for parity with the analogous functionality on the /hierarchy endpoint

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, let me create a story then.

Copy link
Contributor

Choose a reason for hiding this comment

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

the /hierarchy endpoint filters are case insensitive too.

Copy link
Contributor Author

@pallavi2209 pallavi2209 Oct 24, 2016

Choose a reason for hiding this comment

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

@shriramshankar Just checked, we can add different case tags to a subject, like I can have

 tags = [
    "mytag",
    "Cali",
    "mytaG",
    "MYTAG"
  ]

Is this expected behavior?
Also, if we need to filter case insensitive, there are 2 ways:

  1. Filter on API layer instead of query level, like hierarchy endpoint. Disadvantage: get all subjects first and then extra processing to filter.
  2. store tags in lowercase in db. This is more efficient way to do, if we agree.

I can add this feature to the new story to filter by excludes as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes you can have mixed cases for tags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, if we can have mixed tags, then option 2 won't work and we will have to do filtering on API layer? What is the use case of having mixed tags?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since filtering for the hierarchy end point was done in the API layer, it made sense to store the tags the way it was created by the user.

pattern: ^[0-9A-Za-z_][0-9A-Za-z_\\-]{1,59}$
description: >-
Filter by tags.
required: false
type: array
responses:
200:
description: >-
Expand Down
27 changes: 26 additions & 1 deletion config.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ const optimizeUpsert = pe.OPTIMIZE_UPSERT === 'true' ||
// env variable to enable caching for /GET /v1/perspectives/{key}
const enableCachePerspective = pe.ENABLE_CACHE_PERSPECTIVE || false;

const filterSubjByTags = pe.FILTER_SUBJ_BY_TAGS === 'true' ||
pe.FILTER_SUBJ_BY_TAGS === true || false;

module.exports = {

api: {
Expand Down Expand Up @@ -111,6 +114,7 @@ module.exports = {
useAccessToken: pe.USE_ACCESS_TOKEN || false,
tokenSecret:
'7265666f637573726f636b7377697468677265656e6f776c7373616e6672616e',
filterSubjByTags,
},
development: {
checkTimeoutIntervalMillis: pe.CHECK_TIMEOUT_INTERVAL_MILLIS ||
Expand All @@ -130,6 +134,7 @@ module.exports = {
useAccessToken: pe.USE_ACCESS_TOKEN || false,
tokenSecret:
'7265666f637573726f636b7377697468677265656e6f776c7373616e6672616e',
filterSubjByTags,
},
production: {
checkTimeoutIntervalMillis: pe.CHECK_TIMEOUT_INTERVAL_MILLIS ||
Expand All @@ -148,6 +153,7 @@ module.exports = {
useAccessToken: pe.USE_ACCESS_TOKEN || false,
tokenSecret: pe.SECRET_TOKEN ||
'7265666f637573726f636b7377697468677265656e6f776c7373616e6672616e',
filterSubjByTags,
},
test: {
checkTimeoutIntervalMillis: pe.CHECK_TIMEOUT_INTERVAL_MILLIS ||
Expand All @@ -166,6 +172,7 @@ module.exports = {
useAccessToken: pe.USE_ACCESS_TOKEN || false,
tokenSecret: pe.SECRET_TOKEN ||
'7265666f637573726f636b7377697468677265656e6f776c7373616e6672616e',
filterSubjByTags,
},
testDisableHttp: {
checkTimeoutIntervalMillis: pe.CHECK_TIMEOUT_INTERVAL_MILLIS ||
Expand All @@ -179,6 +186,7 @@ module.exports = {
useAccessToken: 'true',
tokenSecret:
'7265666f637573726f636b7377697468677265656e6f776c7373616e6672616e',
filterSubjByTags,
},
testWhitelistLocalhost: {
checkTimeoutIntervalMillis: pe.CHECK_TIMEOUT_INTERVAL_MILLIS ||
Expand All @@ -192,6 +200,7 @@ module.exports = {
ipWhitelist: iplist,
tokenSecret:
'7265666f637573726f636b7377697468677265656e6f776c7373616e6672616e',
filterSubjByTags,
},
testBlockAllhosts: {
checkTimeoutIntervalMillis: pe.CHECK_TIMEOUT_INTERVAL_MILLIS ||
Expand All @@ -205,6 +214,7 @@ module.exports = {
ipWhitelist: [''],
tokenSecret:
'7265666f637573726f636b7377697468677265656e6f776c7373616e6672616e',
filterSubjByTags,
},
testTokenReq: {
checkTimeoutIntervalMillis: pe.CHECK_TIMEOUT_INTERVAL_MILLIS ||
Expand All @@ -218,6 +228,7 @@ module.exports = {
useAccessToken: 'true',
tokenSecret:
'7265666f637573726f636b7377697468677265656e6f776c7373616e6672616e',
filterSubjByTags,
},
testTokenNotReq: {
checkTimeoutIntervalMillis: pe.CHECK_TIMEOUT_INTERVAL_MILLIS ||
Expand All @@ -231,8 +242,22 @@ module.exports = {
useAccessToken: false,
tokenSecret:
'7265666f637573726f636b7377697468677265656e6f776c7373616e6672616e',
filterSubjByTags,
},

testSubjTagFilter: {
checkTimeoutIntervalMillis: pe.CHECK_TIMEOUT_INTERVAL_MILLIS ||
DEFAULT_CHECK_TIMEOUT_INTERVAL_MILLIS,
dbLogging: false, // console.log | false | ...
dbUrl: defaultDbUrl,
disableHttp,
redisUrl: '//127.0.0.1:6379',
defaultNodePort: defaultPort,
host: '127.0.0.1',
useAccessToken: false,
tokenSecret:
'7265666f637573726f636b7377697468677265656e6f776c7373616e6672616e',
filterSubjByTags: true,
}
},

nodeEnv,
Expand Down
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,9 @@
"test-token-notreq": "NODE_ENV=testTokenNotReq mocha -R dot --recursive tests/tokenNotReq",
"test-db": "npm run resetdb && mocha -R dot --recursive tests/db",
"test-realtime": "mocha -R dot --recursive tests/realtime",
"test-subj-tag-filter": "NODE_ENV=testSubjTagFilter mocha -R dot --recursive tests/subjectTagFilter",
"test-view": "NODE_ENV=test mocha -R dot --recursive --compilers js:babel-core/register --require ./tests/view/setup.js tests/view",
"test": "istanbul cover ./node_modules/mocha/bin/_mocha --report lcovonly -- -R dot --recursive tests/api tests/db && cat ./coverage/lcov.info | ./node_modules/coveralls/bin/coveralls.js && rm -rf ./coverage && npm run test-view && npm run test-realtime && npm run test-token-req && npm run test-token-notreq && npm run test-disablehttp",
"test": "istanbul cover ./node_modules/mocha/bin/_mocha --report lcovonly -- -R dot --recursive tests/api tests/db && cat ./coverage/lcov.info | ./node_modules/coveralls/bin/coveralls.js && rm -rf ./coverage && npm run test-view && npm run test-realtime && npm run test-token-req && npm run test-token-notreq && npm run test-disablehttp && npm run test-subj-tag-filter",
"postinstall": "NODE_ENV=production gulp browserifyViews && gulp movecss && gulp movesocket && gulp movelensutil && npm run checkdb",
"prestart": "npm run checkprivatedb"
},
Expand Down
2 changes: 2 additions & 0 deletions tests/api/v1/subjects/get.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,12 @@ describe(`api: GET ${path}`, () => {
const us = {
name: `${tu.namePrefix}UnitedStates`,
description: 'country',
tags: ['US'],
};
const vt = {
name: `${tu.namePrefix}Vermont`,
description: 'state',
tags: ['US', 'NE'],
};

before((done) => {
Expand Down
104 changes: 104 additions & 0 deletions tests/subjectTagFilter/subjectTagFilter.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
/**
* Copyright (c) 2016, salesforce.com, inc.
* All rights reserved.
* Licensed under the BSD 3-Clause license.
* For full license text, see LICENSE.txt file in the repo root or
* https://opensource.org/licenses/BSD-3-Clause
*/

/**
* tests/subjectTagFilter/subjectTagFilter.js
*/
'use strict';
const supertest = require('supertest');

const api = supertest(require('../../index').app);
const constants = require('../../api/v1/constants');
const tu = require('../testUtils');
const u = require('./utils');
const Subject = tu.db.Subject;
const path = '/v1/subjects';
const expect = require('chai').expect;

describe(`api: GET ${path}`, () => {
let token;

const na = {
name: `${tu.namePrefix}NorthAmerica`,
description: 'continent',
};
const us = {
name: `${tu.namePrefix}UnitedStates`,
description: 'country',
tags: ['US'],
};
const vt = {
name: `${tu.namePrefix}Vermont`,
description: 'state',
tags: ['US', 'NE'],
};

before((done) => {
tu.createToken()
.then((returnedToken) => {
token = returnedToken;
done();
})
.catch((err) => done(err));
});

before((done) => {
Subject.create(na)
.then((createdNa) => {
na.id = createdNa.id;
us.parentId = na.id;
return Subject.create(us);
})
.then((createdUs) => {
us.id = createdUs.id;
vt.parentId = us.id;
return Subject.create(vt);
})
.then((createdVt) => {
vt.id = createdVt.id;
done();
})
.catch((err) => done(err));
});

after(u.forceDelete);
after(tu.forceDeleteUser);

it('GET with tag filter :: one tag', (done) => {
api.get(`${path}?tags=US`)
.set('Authorization', token)
.expect(constants.httpStatus.OK)
.end((err, res) => {
if (err) {
return done(err);
}

expect(res.body.length).to.equal(2);
expect(res.body[0].tags).to.eql(['US']);
expect(res.body[1].tags).to.eql(['US', 'NE']);

done();
});
});

it('GET with tag filter :: multiple tags', (done) => {
api.get(`${path}?tags=NE,US`)
.set('Authorization', token)
.expect(constants.httpStatus.OK)
.end((err, res) => {
if (err) {
return done(err);
}

expect(res.body.length).to.equal(1);
expect(res.body[0].tags).to.eql(['US', 'NE']);

done();
});
});
});
Loading