-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Relative time queries #4289
Changes from 27 commits
5d9b227
9b88e13
205fc35
712c495
f3fce46
b837e2a
f4723a6
477bbab
f28f3fd
ced46cb
b1e2594
db94a73
5e503d1
6a09f17
f8ce556
68641c4
f82fff2
6a0e2ce
50dc4ba
5f7d26b
af0f831
dc61f91
1995052
91149e9
a46dadd
6aa7442
08d0743
314bac5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a thought, but we could support shorthand interval values like |
||
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 | ||
|
@@ -565,9 +668,33 @@ function transformConstraint(constraint, field) { | |
case '$gte': | ||
case '$exists': | ||
case '$ne': | ||
case '$eq': | ||
answer[key] = transformer(constraint[key]); | ||
case '$eq': { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we may enter on some weird expectations here: There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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': { | ||
|
@@ -1196,4 +1323,6 @@ module.exports = { | |
transformUpdate, | ||
transformWhere, | ||
mongoObjectToParseObject, | ||
relativeTimeToDate, | ||
transformConstraint, | ||
}; |
There was a problem hiding this comment.
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.