Skip to content

Multiple filter w/ include fix + internal routing fix + more #233

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

Merged
merged 8 commits into from
Feb 6, 2017
Merged

Multiple filter w/ include fix + internal routing fix + more #233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
15873aa
allows objects to pass graphiql
st3xupery Dec 24, 2016
88e92c5
supress graphiql if config sets false; record originalUrl value in re…
st3xupery Dec 26, 2016
4a4a180
Fixed bug where comma delineated filters would upset the internal rou…
st3xupery Dec 27, 2016
62c00d9
Added one more test to check non-comma filter standard
st3xupery Dec 27, 2016
a4b97fb
Merge remote-tracking branch 'real-origin/master' into HEAD
st3xupery Feb 5, 2017
8bb500f
replaced if condition with regex; added coverage folder to gitignore
st3xupery Feb 5, 2017
c68857a
adjusted regex to look for forward slash from end of statement
st3xupery Feb 5, 2017
ca1623a
typo in swagger basepath generate
st3xupery Feb 6, 2017
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: 1 addition & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
node_modules
coverage.html
coverage
complexity
npm-debug.log
jsonapi-server.cpuprofile
12 changes: 12 additions & 0 deletions example/resources/photos.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,18 @@ jsonApi.define({
width: 350,
tags: ['black', 'green'],
photographer: { type: 'people', id: 'ad3aa89e-9c5b-4ac9-a652-6670f9f27587' }
},
{
id: 'ed45eba1-15fe-41c7-93da-1df3dfa5289f',
type: 'photos',
title: 'Sunset Horizon',
url: 'http://www.example.com/sunset',
height: 450,
width: 1050,
raw: true,
tags: ['orange', 'sky', 'sun'],
photographer: { type: 'people', id: 'cc5cca2e-0dd8-4b95-8cfc-a11230e73116' }
}

]
})
4 changes: 2 additions & 2 deletions lib/filter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,6 @@ filter._parseScalarFilterElement = (attributeConfig, scalarElement) => {
filter._parseFilterElementHelper = (attributeConfig, filterElement) => {
if (!filterElement) return { error: 'invalid or empty filter element' }

if (typeof filterElement === 'string') filterElement = filterElement.split(FILTER_SEPERATOR)

const parsedElements = [].concat(filterElement).map(scalarElement => filter._parseScalarFilterElement(attributeConfig, scalarElement))

if (parsedElements.length === 1) return parsedElements[0]
Expand Down Expand Up @@ -117,6 +115,8 @@ filter.parseAndValidate = request => {
for (const key in request.params.filter) {
filterElement = request.params.filter[key]

if (typeof filterElement === 'string') request.params.filter[key] = filterElement = filterElement.split(FILTER_SEPERATOR)
Copy link
Contributor

@pmcnr-hx pmcnr-hx Jan 24, 2017

Choose a reason for hiding this comment

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

This is a pragmatic solution, but I'm not too thrilled about mutating request.params.filter[key]. I'm going to have a look at the internal routing and check if we can used the processed filter there also.


if (!Array.isArray(filterElement) && filterElement instanceof Object) continue // skip deep filters

error = filter._resourceDoesNotHaveProperty(resourceConfig, key)
Expand Down
10 changes: 6 additions & 4 deletions lib/graphQl/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,12 @@ const writeTypes = require('./writeTypes.js')
jsonApiGraphQL.with = app => {
const config = jsonApi._apiConfig

app.use(new RegExp(`${config.base}$`), graphqlHTTP({
schema: jsonApiGraphQL.generate(jsonApi._resources),
graphiql: !!config.graphiql
}))
if (config.graphiql !== false) {
Copy link
Contributor

@pmcnr-hx pmcnr-hx Jan 24, 2017

Choose a reason for hiding this comment

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

Shall we introduce the strict configuration property that @theninj4 suggests to turn off the GraphQL endpoint? Abusing the graphiql configuration property for this end is not the best option.

Copy link
Contributor Author

@st3xupery st3xupery Feb 6, 2017

Choose a reason for hiding this comment

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

Not sure what you mean, is !== false not a strict equivalency?

Copy link
Contributor

@pmcnr-hx pmcnr-hx Feb 6, 2017

Choose a reason for hiding this comment

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

What we were suggesting was a new configuration property named strict, but we can get round to doing that in a later PR.

app.use(new RegExp(`${config.base}$`), graphqlHTTP({
schema: jsonApiGraphQL.generate(jsonApi._resources),
graphiql: !!config.graphiql
}))
}
}

jsonApiGraphQL.generate = allResourceConfig => {
Expand Down
8 changes: 5 additions & 3 deletions lib/postProcessing/include.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ const debug = require('../debugging.js')

includePP.action = (request, response, callback) => {
let includes = request.params.include
const filters = request.params.filter || { }
const filters = request.params.filter
if (!includes) return callback()
includes = (`${includes}`).split(',')

Expand Down Expand Up @@ -47,6 +47,7 @@ includePP._arrayToTree = (request, includes, filters, callback) => {
const parts = text.split('.')
const first = parts.shift()
const rest = parts.join('.')
if (!filter) filter = {}

let resourceAttribute = node._resourceConfig.map(resourceConfig => {
return resourceConfig.attributes[first]
Expand All @@ -70,6 +71,7 @@ includePP._arrayToTree = (request, includes, filters, callback) => {
}

filter = filter[first] || { }

if (filter instanceof Array) {
filter = filter.filter(i => i instanceof Object).pop()
}
Expand Down Expand Up @@ -159,7 +161,7 @@ includePP._fillIncludeTree = (includeTree, request, callback) => {
ids += `&${includeTree[parts[1]]._filter.join('&')}`
}
resourcesToFetch.push({
url: `${jsonApi._apiConfig.pathPrefix + parts[0]}/?${ids}`,
url: `${jsonApi._apiConfig.base + parts[0]}/?${ids}`,
as: relation
})
})
Expand All @@ -173,7 +175,7 @@ includePP._fillIncludeTree = (includeTree, request, callback) => {
ids += `&${includeTree[parts[2]]._filter.join('&')}`
}
resourcesToFetch.push({
url: `${jsonApi._apiConfig.pathPrefix + parts[1]}/?${ids}`,
url: `${jsonApi._apiConfig.base + parts[1]}/?${ids}`,
as: relation
})
})
Expand Down
1 change: 1 addition & 0 deletions lib/rerouter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ rerouter.route = (newRequest, callback) => {

const req = {
url: newRequest.uri,
originalUrl: newRequest.originalRequest.originalUrl,
headers: newRequest.originalRequest.headers,
cookies: newRequest.originalRequest.cookies,
params: rerouter._mergeParams(url.parse(newRequest.uri.split('?')[1] || { }), newRequest.params)
Expand Down
1 change: 1 addition & 0 deletions lib/router.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ router._getParams = req => {
headers: req.headers,
safeHeaders: _.omit(req.headers, headersToRemove),
cookies: req.cookies,
originalUrl: req.originalUrl,
route: {
verb: req.method,
host: req.headers.host,
Expand Down
2 changes: 1 addition & 1 deletion lib/swagger/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ swagger._getSwaggerBase = () => {
let basePath, host, protocol
if (jsonApi._apiConfig.urlPrefixAlias) {
const urlObj = url.parse(jsonApi._apiConfig.urlPrefixAlias)
basePath = urlObj.pathname.replace(/\/$/, '')
basePath = urlObj.pathname.replace(/(?!^\/)\/$/, '')
Copy link
Contributor

@pmcnr-hx pmcnr-hx Jan 24, 2017

Choose a reason for hiding this comment

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

If we use the (?<!^)\/$ regexp (with a look behind to assert we're not at the beginning of the string) above we can avoid the if.

Copy link
Contributor Author

@st3xupery st3xupery Feb 6, 2017

Choose a reason for hiding this comment

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

Had to use something slightly different as javascript does not have a look behind in it's regex capabilities. The alternative works to the same effect: (?!^/)/$

host = urlObj.host
protocol = urlObj.protocol.replace(/:$/, '')
} else {
Expand Down
2 changes: 1 addition & 1 deletion test/_preResourceValidationCheck.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ describe('Testing jsonapi-server', () => {
[ { name: 'articles', count: 4 },
{ name: 'comments', count: 3 },
{ name: 'people', count: 4 },
{ name: 'photos', count: 3 },
{ name: 'photos', count: 4 },
{ name: 'tags', count: 5 }
].forEach(resource => {
describe(`Searching for ${resource.name}`, () => {
Expand Down
54 changes: 49 additions & 5 deletions test/get-resource.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ describe('Testing jsonapi-server', () => {

assert.equal(res.statusCode, '200', 'Expecting 200 OK')
const photoTypes = json.data.map(i => i.attributes.raw)
assert.deepEqual(photoTypes, [ true ], 'expected matching resources')
assert.deepEqual(photoTypes, [ true, true ], 'expected matching resources')

done()
})
Expand Down Expand Up @@ -530,13 +530,13 @@ describe('Testing jsonapi-server', () => {
json = helpers.validateJson(json)

assert.equal(res.statusCode, '200', 'Expecting 200 OK')
assert.equal(json.included.length, 7, 'Should be 7 included resources')
assert.equal(json.included.length, 8, 'Should be 8 included resources')

const people = json.included.filter(resource => resource.type === 'people')
assert.equal(people.length, 4, 'Should be 4 included people resources')

const photos = json.included.filter(resource => resource.type === 'photos')
assert.equal(photos.length, 3, 'Should be 3 included photos resources')
assert.equal(photos.length, 4, 'Should be 4 included photos resources')

done()
})
Expand All @@ -552,13 +552,13 @@ describe('Testing jsonapi-server', () => {
json = helpers.validateJson(json)

assert.equal(res.statusCode, '200', 'Expecting 200 OK')
assert.equal(json.included.length, 7, 'Should be 7 included resources')
assert.equal(json.included.length, 8, 'Should be 8 included resources')

const people = json.included.filter(resource => resource.type === 'people')
assert.equal(people.length, 4, 'Should be 4 included people resources')

const photos = json.included.filter(resource => resource.type === 'photos')
assert.equal(photos.length, 3, 'Should be 3 included photos resources')
assert.equal(photos.length, 4, 'Should be 4 included photos resources')

done()
})
Expand All @@ -585,6 +585,50 @@ describe('Testing jsonapi-server', () => {
done()
})
})

it('include author.photos with multiple filters', done => {
const url = 'http://localhost:16006/rest/articles?include=author.photos&filter[author]=ad3aa89e-9c5b-4ac9-a652-6670f9f27587&filter[author]=cc5cca2e-0dd8-4b95-8cfc-a11230e73116'
helpers.request({
method: 'GET',
url
}, (err, res, json) => {
assert.equal(err, null)
json = helpers.validateJson(json)

assert.equal(res.statusCode, '200', 'Expecting 200 OK')
assert.equal(json.included.length, 5, 'Should be 2 included resources')

const people = json.included.filter(resource => resource.type === 'people')
assert.equal(people.length, 2, 'Should be 2 included people resource')

const photos = json.included.filter(resource => resource.type === 'photos')
assert.equal(photos.length, 3, 'Should be 2 included photos resource')

done()
})
})

it('include author.photos with multiple filters comma delineated', done => {
const url = 'http://localhost:16006/rest/articles?include=author.photos&filter[author][firstname]=Mark,Oli'
helpers.request({
method: 'GET',
url
}, (err, res, json) => {
assert.equal(err, null)
json = helpers.validateJson(json)

assert.equal(res.statusCode, '200', 'Expecting 200 OK')
assert.equal(json.included.length, 4, 'Should be 2 included resources')

const people = json.included.filter(resource => resource.type === 'people')
assert.equal(people.length, 2, 'Should be 2 included people resource')

const photos = json.included.filter(resource => resource.type === 'photos')
assert.equal(photos.length, 2, 'Should be 2 included photos resource')

done()
})
})
})

describe('by foreign key', () => {
Expand Down
2 changes: 1 addition & 1 deletion test/zzPostResourceValidationCheck.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ describe('Testing jsonapi-server', () => {
[ { name: 'articles', count: 4 },
{ name: 'comments', count: 2 },
{ name: 'people', count: 4 },
{ name: 'photos', count: 4 },
{ name: 'photos', count: 5 },
{ name: 'tags', count: 5 }
].forEach(resource => {
describe(`Searching for ${resource.name}`, () => {
Expand Down