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

Conversation

marvelm
Copy link
Contributor

@marvelm marvelm commented Oct 23, 2017

Context

I would like to be able to create queries where the current time is injected by Parse.

For example:

const recentInactiveQ = new Parse.Query(Parse.Installation);
recentInactiveQ.greaterThan('createdAt', { $relativeTime: '2 weeks ago' });
recentInactiveQ.lessThan('updatedAt', { $relativeTime: '3 days ago' });

Parse.Push.send({
  where: recentInactiveQ,
  data: {
    alert: "Here's what you missed this week...",
    url: "https://example.org/this-weeks-content"
  },
});

The future

This type of query can be serialized and re-executed at any time. I plan on adding support for recurring push notifications and relative time queries are essential for this feature.
It would look something like this (#4105):

Parse.Push.createCampaign({
  where: recentInactiveQ,
  data: {
    alert: "Here's what you missed this week...",
    url: "https://example.org/this-weeks-content"
  },
  interval: 'daily',
  sendTime: '17:30',
});

@flovilmart
Copy link
Contributor

I've added all of you as @marvelm is working with me @AmpMe on those features.

})
.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.

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

// strip the 'ago' or 'in'
if (future) {
parts = parts.slice(1);
} else if (past) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to do a check for past? Unless there's something else we're planning to add you could just use else.

ttl: new Date(now - 2 * 24 * 60 * 60 * 1000), // 2 days ago
});

dropDatabase()
Copy link
Contributor

Choose a reason for hiding this comment

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

you don't need that, the DB is dropped between each test

spec/helper.js Outdated
@@ -451,3 +451,26 @@ jasmine.restoreLibrary = function(library, name) {
}
require(library)[name] = libraryCache[library][name];
}

const { MongoClient } = require('mongodb');
Copy link
Contributor

Choose a reason for hiding this comment

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

not needed.

switch(interval) {
case 'day':
case 'days':
seconds += val * 24 * 60 * 60;
Copy link
Contributor

@montymxb montymxb Oct 24, 2017

Choose a reason for hiding this comment

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

We can just use 2880 (3600, oops..., wow 86400...) rather than performing the multiplication each time, but you could leave a comment to detail it if needed.


case 'hour':
case 'hours':
seconds += val * 60 * 60;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above with 3600

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.

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.

Copy link
Contributor

@montymxb montymxb left a comment

Choose a reason for hiding this comment

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

Some recommendations and ideas for moving this along. The coverage is kind of low too, before this goes in we'll probably have addressed that however.

Overall this is super cool! The logic is pretty clean for converting from relative time textually on top of it, nice job.

@@ -565,9 +663,22 @@ function transformConstraint(constraint, field) {
case '$gte':
case '$exists':
case '$ne':
case '$eq':
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.

@marvelm
Copy link
Contributor Author

marvelm commented Oct 25, 2017

Thanks for reviewing.

I had to rebase against master because of a failing test. I've addressed the issues raised in the review in "'Add integration test for multiple results' f8ce556" and the following commits.

@flovilmart
Copy link
Contributor

@montymxb this all look good to me!

@flovilmart flovilmart mentioned this pull request Oct 26, 2017
@montymxb
Copy link
Contributor

Changes look good, but the CI looks out of date. I'm going to bring this up to date with the master to retrigger build/coverage checking. I'm expecting it to come up as a ✓ (or close) but I want to see for certain before I give this an approval.

@parse-community parse-community deleted a comment from codecov bot Oct 26, 2017
@codecov
Copy link

codecov bot commented Oct 26, 2017

Codecov Report

Merging #4289 into master will decrease coverage by 0.03%.
The diff coverage is 86.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4289      +/-   ##
==========================================
- Coverage   92.53%   92.49%   -0.04%     
==========================================
  Files         118      118              
  Lines        8196     8246      +50     
==========================================
+ Hits         7584     7627      +43     
- Misses        612      619       +7
Impacted Files Coverage Δ
src/Adapters/Storage/Mongo/MongoTransform.js 85.22% <86.27%> (+0.24%) ⬆️
src/RestWrite.js 93.21% <0%> (-0.19%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1dd58b7...314bac5. Read the comment docs.

Copy link
Contributor

@montymxb montymxb left a comment

Choose a reason for hiding this comment

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

Alright, looks good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants