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

Relative time queries #4289

Merged
merged 28 commits into from
Oct 26, 2017
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
5d9b227
Add relative time queries
Sep 26, 2017
9b88e13
Encode successful result
Oct 2, 2017
205fc35
Add integration test
Oct 11, 2017
712c495
Add more error cases
Oct 11, 2017
f3fce46
Remove unnecessary new Date
Oct 11, 2017
b837e2a
Error when time has both 'in' and 'ago'
Oct 12, 2017
f4723a6
naturalTimeToDate -> relativeTimeToDate
Oct 13, 2017
477bbab
Add $relativeTime operator
Oct 24, 2017
f28f3fd
Throw error if $relativeTime is invalid
Oct 24, 2017
ced46cb
Add integration test for invalid relative time
Oct 24, 2017
b1e2594
Exclude $exists query
Oct 24, 2017
db94a73
Only run integration tests on MongoDB
Oct 24, 2017
5e503d1
Add it_only_db test helper
Oct 24, 2017
6a09f17
Handle where val might be null or undefined
Oct 24, 2017
f8ce556
Add integration test for multiple results
Oct 25, 2017
68641c4
Lowercase text before processing
Oct 25, 2017
f82fff2
Always past if not future
Oct 25, 2017
6a0e2ce
Precompute seconds multiplication
Oct 25, 2017
50dc4ba
Add shorthand for interval
Oct 25, 2017
5f7d26b
Throw error if $relativeTime is used with $exists, $ne, and $eq
Oct 25, 2017
af0f831
Improve coverage for relativeTimeToDate
Oct 25, 2017
dc61f91
Add test for erroring on floating point units
Oct 25, 2017
1995052
Remove unnecessary dropDatabase function
Oct 25, 2017
91149e9
Unit test $ne, $exists, $eq
Oct 25, 2017
a46dadd
Verify field type
Oct 25, 2017
6aa7442
Fix unit test for $exists
Oct 25, 2017
08d0743
Merge branch 'master' into relative-time-queries
marvelm Oct 25, 2017
314bac5
Merge branch 'master' into relative-time-queries
montymxb Oct 26, 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
1 change: 1 addition & 0 deletions spec/.eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
"Container": true,
"equal": true,
"notEqual": true,
"it_only_db": true,
"it_exclude_dbs": true,
"describe_only_db": true,
"describe_only": true,
Expand Down
115 changes: 115 additions & 0 deletions spec/MongoTransform.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -347,3 +347,118 @@ describe('transformUpdate', () => {
done();
});
});

describe('transformConstraint', () => {
describe('$relativeTime', () => {
it('should error on $eq, $ne, and $exists', () => {
expect(() => {
transform.transformConstraint({
$eq: {
ttl: {
$relativeTime: '12 days ago',
}
}
});
}).toThrow();

expect(() => {
transform.transformConstraint({
$ne: {
ttl: {
$relativeTime: '12 days ago',
}
}
});
}).toThrow();

expect(() => {
transform.transformConstraint({
$exists: {
$relativeTime: '12 days ago',
}
});
}).toThrow();
});
})
});

describe('relativeTimeToDate', () => {
const now = new Date('2017-09-26T13:28:16.617Z');

describe('In the future', () => {
it('should parse valid natural time', () => {
const text = 'in 12 days 10 hours 24 minutes 30 seconds';
const { result, status, info } = transform.relativeTimeToDate(text, now);
expect(result.toISOString()).toBe('2017-10-08T23:52:46.617Z');
expect(status).toBe('success');
expect(info).toBe('future');
});
});

describe('In the past', () => {
it('should parse valid natural time', () => {
const text = '2 days 12 hours 1 minute 12 seconds ago';
const { result, status, info } = transform.relativeTimeToDate(text, now);
expect(result.toISOString()).toBe('2017-09-24T01:27:04.617Z');
expect(status).toBe('success');
expect(info).toBe('past');
});
});

describe('Error cases', () => {
it('should error if string is completely gibberish', () => {
expect(transform.relativeTimeToDate('gibberishasdnklasdnjklasndkl123j123')).toEqual({
status: 'error',
info: "Time should either start with 'in' or end with 'ago'",
});
});

it('should error if string contains neither `ago` nor `in`', () => {
expect(transform.relativeTimeToDate('12 hours 1 minute')).toEqual({
status: 'error',
info: "Time should either start with 'in' or end with 'ago'",
});
});

it('should error if there are missing units or numbers', () => {
expect(transform.relativeTimeToDate('in 12 hours 1')).toEqual({
status: 'error',
info: 'Invalid time string. Dangling unit or number.',
});

expect(transform.relativeTimeToDate('12 hours minute ago')).toEqual({
status: 'error',
info: 'Invalid time string. Dangling unit or number.',
});
});

it('should error on floating point numbers', () => {
expect(transform.relativeTimeToDate('in 12.3 hours')).toEqual({
status: 'error',
info: "'12.3' is not an integer.",
});
});

it('should error if numbers are invalid', () => {
expect(transform.relativeTimeToDate('12 hours 123a minute ago')).toEqual({
status: 'error',
info: "'123a' is not an integer.",
});
});

it('should error on invalid interval units', () => {
expect(transform.relativeTimeToDate('4 score 7 years ago')).toEqual({
status: 'error',
info: "Invalid interval: 'score'",
});
});

it("should error when string contains 'ago' and 'in'", () => {
expect(transform.relativeTimeToDate('in 1 day 2 minutes ago')).toEqual({
status: 'error',
info: "Time cannot have both 'in' and 'ago'",
});
});
});
});

77 changes: 75 additions & 2 deletions spec/ParseQuery.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -3105,7 +3105,80 @@ describe('Parse.Query testing', () => {
equal(result.has('testPointerField'), result.get('shouldBe'));
});
done();
}
).catch(done.fail);
}).catch(done.fail);
});

it_only_db('mongo')('should handle relative times correctly', function(done) {
const now = Date.now();
const obj1 = new Parse.Object('MyCustomObject', {
name: 'obj1',
ttl: new Date(now + 2 * 24 * 60 * 60 * 1000), // 2 days from now
});
const obj2 = new Parse.Object('MyCustomObject', {
name: 'obj2',
ttl: new Date(now - 2 * 24 * 60 * 60 * 1000), // 2 days ago
});

Parse.Object.saveAll([obj1, obj2])
.then(() => {
const q = new Parse.Query('MyCustomObject');
q.greaterThan('ttl', { $relativeTime: 'in 1 day' });
return q.find({ useMasterKey: true });
})
.then((results) => {
expect(results.length).toBe(1);
})
.then(() => {
const q = new Parse.Query('MyCustomObject');
q.greaterThan('ttl', { $relativeTime: '1 day ago' });
return q.find({ useMasterKey: true });
})
.then((results) => {
expect(results.length).toBe(1);
})
.then(() => {
const q = new Parse.Query('MyCustomObject');
q.lessThan('ttl', { $relativeTime: '5 days ago' });
return q.find({ useMasterKey: true });
})
.then((results) => {
expect(results.length).toBe(0);
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Considering that we have a test for getting at least one of each object, and one for neither, can we add one that returns both objects? Something using q.greaterThan('ttl', { $relativeTime: '3 days ago' }); is what comes to mind.

.then(() => {
const q = new Parse.Query('MyCustomObject');
q.greaterThan('ttl', { $relativeTime: '3 days ago' });
return q.find({ useMasterKey: true });
})
.then((results) => {
expect(results.length).toBe(2);
})
.then(done, done.fail);
});

it_only_db('mongo')('should error on invalid relative time', function(done) {
const obj1 = new Parse.Object('MyCustomObject', {
name: 'obj1',
ttl: new Date(Date.now() + 2 * 24 * 60 * 60 * 1000), // 2 days from now
});

const q = new Parse.Query('MyCustomObject');
q.greaterThan('ttl', { $relativeTime: '-12 bananas ago' });
obj1.save({ useMasterKey: true })
.then(() => q.find({ useMasterKey: true }))
.then(done.fail, done);
});

it_only_db('mongo')('should error when using $relativeTime on non-Date field', function(done) {
const obj1 = new Parse.Object('MyCustomObject', {
name: 'obj1',
nonDateField: 'abcd',
ttl: new Date(Date.now() + 2 * 24 * 60 * 60 * 1000), // 2 days from now
});

const q = new Parse.Query('MyCustomObject');
q.greaterThan('nonDateField', { $relativeTime: '1 day ago' });
obj1.save({ useMasterKey: true })
.then(() => q.find({ useMasterKey: true }))
.then(done.fail, done);
});
});
8 changes: 8 additions & 0 deletions spec/helper.js
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,14 @@ global.it_exclude_dbs = excluded => {
}
}

global.it_only_db = db => {
if (process.env.PARSE_SERVER_TEST_DB === db) {
return it;
} else {
return xit;
}
};

global.fit_exclude_dbs = excluded => {
if (excluded.indexOf(process.env.PARSE_SERVER_TEST_DB) >= 0) {
return xit;
Expand Down
133 changes: 131 additions & 2 deletions src/Adapters/Storage/Mongo/MongoTransform.js
Original file line number Diff line number Diff line change
Expand Up @@ -533,6 +533,109 @@ function transformTopLevelAtom(atom, field) {
}
}

function relativeTimeToDate(text, now = new Date()) {
text = text.toLowerCase();

let parts = text.split(' ');

// Filter out whitespace
parts = parts.filter((part) => part !== '');

const future = parts[0] === 'in';
const past = parts[parts.length - 1] === 'ago';
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, wondering if these should be case insensitive. We could lowercase the entire string in advance before processing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool


if (!future && !past) {
return { status: 'error', info: "Time should either start with 'in' or end with 'ago'" };
}

if (future && past) {
return {
status: 'error',
info: "Time cannot have both 'in' and 'ago'",
};
}

// strip the 'ago' or 'in'
if (future) {
parts = parts.slice(1);
} else { // past
parts = parts.slice(0, parts.length - 1);
}

if (parts.length % 2 !== 0) {
return {
status: 'error',
info: 'Invalid time string. Dangling unit or number.',
};
}

const pairs = [];
while(parts.length) {
pairs.push([ parts.shift(), parts.shift() ]);
}

let seconds = 0;
for (const [num, interval] of pairs) {
const val = Number(num);
if (!Number.isInteger(val)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

With only whole integer values allowed perhaps we should add a test that expects to fail when someone tries something like 2.5 days.

Copy link
Contributor

Choose a reason for hiding this comment

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

From the code, as interpolated to seconds, it seems that it should work with non Integer values no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense it would work, but we would want to test for it if we decide to allow it (or disallow it as currently).

Copy link
Contributor

Choose a reason for hiding this comment

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

let's stick with integers for now.

return {
status: 'error',
info: `'${num}' is not an integer.`,
};
}

switch(interval) {
case 'day':
case 'days':
seconds += val * 86400; // 24 * 60 * 60
break;

case 'hr':
case 'hrs':
case 'hour':
case 'hours':
seconds += val * 3600; // 60 * 60
break;

case 'min':
case 'mins':
case 'minute':
case 'minutes':
seconds += val * 60;
break;

case 'sec':
case 'secs':
case 'second':
case 'seconds':
seconds += val;
break;

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a thought, but we could support shorthand interval values like sec, min, hr and their plurals. Not necessarily needed however.

default:
return {
status: 'error',
info: `Invalid interval: '${interval}'`,
};
}
}

const milliseconds = seconds * 1000;
if (future) {
return {
status: 'success',
info: 'future',
result: new Date(now.valueOf() + milliseconds)
};
}
if (past) {
return {
status: 'success',
info: 'past',
result: new Date(now.valueOf() - milliseconds)
};
}
}

// Transforms a query constraint from REST API format to Mongo format.
// A constraint is something with fields like $lt.
// If it is not a valid constraint but it could be a valid something
Expand Down Expand Up @@ -565,9 +668,33 @@ function transformConstraint(constraint, field) {
case '$gte':
case '$exists':
case '$ne':
case '$eq':
answer[key] = transformer(constraint[key]);
case '$eq': {
Copy link
Contributor

Choose a reason for hiding this comment

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

we may enter on some weird expectations here: $ne { $relativeTime: "1 day ago" } would you expect all object that were created the full day before to be excluded or just the one that matches perfectly the current time?

Copy link
Contributor

Choose a reason for hiding this comment

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

@montymxb what do you think? Should we limit to $lt/$lte/$gt/$gte to be safe?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, it would make the most sense with the query methods you've mentioned above, but not sure if we should simply rule out $ne/$eq. Although they may not be the most effective query methods with relative date matching they could still work, especially if we allow setting the now date in the future (but still not that great).

I would just say exclude those, but if someone expects it to work what would we return? Just an empty response like the query matched nothing, or provide some sort of error clarifying that equal/notEqual ops are not allowed using this param, or another approach perhaps.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or throw an error, that relativeDates operators don’t work with those operators

Copy link
Contributor

Choose a reason for hiding this comment

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

That would work for me 👍 . Simplifying the initial implementation may be best, we can consider it later if someone actually needs to use it.

const val = constraint[key];
if (val && typeof val === 'object' && val.$relativeTime) {
if (field && field.type !== 'Date') {
throw new Parse.Error(Parse.Error.INVALID_JSON, '$relativeTime can only be used with Date field');
}

switch (key) {
case '$exists':
case '$ne':
case '$eq':
throw new Parse.Error(Parse.Error.INVALID_JSON, '$relativeTime can only be used with the $lt, $lte, $gt, and $gte operators');
}

const parserResult = relativeTimeToDate(val.$relativeTime);
if (parserResult.status === 'success') {
answer[key] = parserResult.result;
break;
}

log.info('Error while parsing relative date', parserResult);
throw new Parse.Error(Parse.Error.INVALID_JSON, `bad $relativeTime (${key}) value. ${parserResult.info}`);
}

answer[key] = transformer(val);
break;
}

case '$in':
case '$nin': {
Expand Down Expand Up @@ -1196,4 +1323,6 @@ module.exports = {
transformUpdate,
transformWhere,
mongoObjectToParseObject,
relativeTimeToDate,
transformConstraint,
};