#4678: Converting strings to Date when schema.type is Date within agg… #4743

cjbland · 2018-04-30T23:17:36Z

I added a check to see if the schema field is a Date. If so, convert the string into a Date object which the mongodb driver will accept for the aggregate function (it will not accept strings). I also added checks for the createdAt and updatedAt attributes and converted them to the actual column names.

…regate function

flovilmart

The code look good, can you add tests to ensure we cover all those 3 code paths and don’t introduce any regression in the future?

codecov · 2018-04-30T23:46:05Z

Codecov Report

Merging #4743 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4743      +/-   ##
==========================================
+ Coverage   92.77%   92.81%   +0.04%     
==========================================
  Files         119      119              
  Lines        8760     8813      +53     
==========================================
+ Hits         8127     8180      +53     
  Misses        633      633

Impacted Files	Coverage Δ
src/Adapters/Storage/Mongo/MongoStorageAdapter.js	`92.03% <100%> (+1.2%)`	⬆️

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 3ace26d...f8d7826. Read the comment docs.

cjbland · 2018-04-30T23:59:58Z

I need to keep working on this, I had forgotten about the other operators such as $gt, $gte, etc.

flovilmart · 2018-05-01T00:49:28Z

@cjbland no worry! Thanks for your contributions!

…ry values to Date objects

flovilmart · 2018-05-01T22:54:33Z

src/Adapters/Storage/Mongo/MongoStorageAdapter.js

-            stage.$match = transformMatch;
-          }
-        }
+        this._parseAggregateArgs(schema, stage.$match);


Returned value seem unused.

Thanks for picking up on that...I was trying to hurry and pushed the repo by accident (muscle memory from my day job, haha). I just pushed the fix.

…to fix/4678

flovilmart

Thanks for the PR. I did a first pass at the review, so if we can get the style + a bit of comments on the hard parts that would be very appreciated ;)

flovilmart · 2018-05-02T00:10:37Z

src/Adapters/Storage/Mongo/MongoStorageAdapter.js

+  _parseAggregateArgs(schema: any, pipeline: any): any {
+    var rtnval = {};
+    if (Array.isArray(pipeline)) {
+      rtnval = [];


can we transform in

return pipeline.map((value) => this._parseAggregateArgs(schema, value));

flovilmart · 2018-05-02T00:10:53Z

src/Adapters/Storage/Mongo/MongoStorageAdapter.js

@@ -608,6 +597,55 @@ export class MongoStorageAdapter implements StorageAdapter {
      .catch(err => this.handleError(err));
  }

+  _parseAggregateArgs(schema: any, pipeline: any): any {
+    var rtnval = {};


use const, and actually remove :)

flovilmart · 2018-05-02T00:11:21Z

src/Adapters/Storage/Mongo/MongoStorageAdapter.js

+      }
+    }
+    else if (typeof pipeline === 'object') {
+      for (const field in pipeline) {


declare const returnValue = {} here and update all fields please

flovilmart · 2018-05-02T00:12:13Z

src/Adapters/Storage/Mongo/MongoStorageAdapter.js

+      for (const field in pipeline) {
+        if (schema.fields[field] && schema.fields[field].type === 'Pointer') {
+          rtnval[`_p_${field}`] = `${schema.fields[field].targetClass}$${pipeline[field]}`;
+        }


throughout the codebase we always write

if { } else if ... { } else if ... { }

Can we keep the style please?

flovilmart · 2018-05-02T00:13:49Z

src/Adapters/Storage/Mongo/MongoStorageAdapter.js

+    }
+
+    var rtnval = {}
+    for (const field in value) {


can we document what we expect the value to look like, I have trouble understanding what it does, and why we need the iterator, and recursion.

Sorry for the newbie question...how do we take care of these? I've updated my branch to include your suggestions. Thanks.

This function may work wrong way when "value" is
{ $exists: false }

flovilmart · 2018-05-02T00:15:56Z

@cjbland also it seems that many tests are failing :) You can run them locally with npm test spec/ParseQuery.Aggregate.spec.js so only the aggregate suite is run!

cjbland · 2018-05-02T00:17:09Z

Thanks for the tip. I didn't realize the tests were failing but I see them now. I'll also fix the styling and take care of the other comments you've made. Thanks for taking your time to review this and sorry I'm not making it a little easier for you.

dplewis · 2018-05-02T00:22:20Z

@cjbland Thanks for getting this PR in. I started something like this a while ago and maybe we can collaborate on this. You can reuse whats in the MongoTransform.js for most of the transform keys.

Here is my branch.
https://github.com/dplewis/parse-server/tree/aggregate-recursive

flovilmart · 2018-05-02T13:55:33Z

@dplewis do you want to take-over with @cjbland on this PR? Perhaps @cjbland , you could get back on the aggregate-recursive branch and see if your use case is solved. What were you planing @dplewis ? I know you've been busy lately with other things.

dplewis · 2018-05-02T14:48:32Z

@flovilmart I noticed 3-4 open issues and a lot of users requested transforming keys like pointers, default fields, and dates. I was trying to handle all use cases but I kinda lack complex aggregate queries to ensure it works.

The current transform code is getting to be too much to handle considering there are ~40 mongo aggregate stages and could use some refactoring.

The stages could be broken down as follows.

// { limit: 1 }
{ stageName: value}

// value can be any type or $field
// { group : { objectId: '' }
// { group: { objectId: '$name' }
{ stageName: { field: value }

// { match: { pointer: { $exist: true } } }
{ stageName: { field: { op: value } }

// group: { total: { $sum: '$score' } },
{ stageName: { newField: { op: value } }

// match: { $or: [{ score: { $gt: 15, $lt: 25 } }, { views: { $gt: 750, $lt: 850 } }]}
{ stageName: { op: value }

There maybe more that I don't know of. I can submit a PR and worry about complex queries as they come along.

Here's an example of the most complex aggregate I've used. #1908 (comment)

cjbland · 2018-05-03T00:01:51Z

I'm definitely open to collaborating. The recursive aggregate function I wrote seems to work, my issue is the test case... I don't have any experience writing these tests, but I feel like the previous tests' TestObjects are being returned when I wasn't expecting them. Any pointers on how best to debug something like that?

dplewis · 2018-05-03T00:16:29Z

@cjbland For sure, checkout the Contributing.MD for running test cases.
https://github.com/parse-community/parse-server/blob/master/CONTRIBUTING.md

There is a function loadTestData() that gets called before each test.

https://github.com/parse-community/parse-server/blob/master/spec/ParseQuery.Aggregate.spec.js#L20

Thats where your TestObjects are coming from. Keep in mind that those objects exists

…were failing

cjbland · 2018-05-04T15:28:46Z

I added some comments to hopefully document my approach here. Personally, I'm not a fan of recursion, but this seems like the perfect candidate for it. The goal was to treat the query like a tree and traverse the tree to get to the leaf nodes. Once at the leaf node determine whether or not the value needed to be converted (based on the schema). I understand the structure isn't exactly a tree, but that's the best way for me to describe my intent. This should also handle any of the aggregation cases (since we aren't looking specifically for pipeline or comparison operators).

dplewis · 2018-05-16T01:26:15Z

@cjbland I like your approach better. Can you copy the tests from my branch to see if they run?

cjbland · 2018-05-17T01:37:26Z

Absolutely. Give me a few days, I have a lot going on this week.

cjbland

I've addressed these changes in the most recent commits.

…to fix/4678

cjbland · 2018-06-20T21:55:51Z

I made the change that @dplewis suggested and added more tests for better code coverage. Hopefully this gets us a little closer.

cjbland · 2018-06-20T22:54:37Z

Is there documentation on which of these aggregate functions are supported by PostgreSQL? That seems to be why the latest tests failed for test 6417.3 - it didn't know how to handle the $exists operator.

dplewis · 2018-06-20T22:57:40Z

@cjbland that’s the reason why that test was excluded. Postgres only supports basic aggregates aka 3 stages no operators

cjbland · 2018-06-22T12:57:36Z

I'm happier with this now. Let me know what the next steps are. Thanks guys!

dplewis · 2018-06-26T00:53:06Z

@flovilmart I did some linter clean up can you do a final review?

flovilmart

Let's go!

dplewis · 2018-06-26T01:02:25Z

@cjbland Thanks for the PR

cjbland · 2018-06-26T01:04:26Z

No problem, thanks for your patience. I’d like to contribute more if needed.

flovilmart · 2018-06-26T01:06:19Z

@cjbland, thanks! Contributions can take many forms, and we always encourage people who want to start contributing a bit more to respond on some issues. If you feel you can address a few of the open ones, i’ll Gladly walk you through it and give the appropriate accesses so you can close / label them!

EricNetsch · 2018-08-12T00:30:52Z

@dplewis @flovilmart
Looks like this was released into 2.8.3 but not seeing the new code on my side when updating/reinstalling. Really strange since I even see the new code when I download the new 2.8.3 zip.

Any ideas on what's going on?
Thanks!

flovilmart · 2018-08-12T00:37:14Z

I wasn't released part of 2.8.3 as this version is a hotfix for compatibility with the JS SDK.

2.8.2...2.8.3

EricNetsch · 2018-08-12T00:49:59Z

@flovilmart Got it. Saw it in the release commits so assumed it was part of 2.8.3 .
Any idea on when we can release this fix?

Thanks for clarifying,

flovilmart · 2018-08-12T00:51:35Z

We’re working on the 3.0 at the moment, and it’s likely gonna be the next release. Perhaps there will be a 2.8.4 before we cut the 3.0

EricNetsch · 2018-08-12T00:53:08Z

Got it thanks.
Ahh I see I was looking at master commits since 2.8.3 lol. Confusing UX on github releases.
Appreciate it.

flovilmart · 2018-08-12T00:54:04Z

It’s always unlikely a release is ahead of master :)

@dplewis

#4743) * #4678: Converting strings to Date when schema.type is Date within aggregate function * Added test cases to test new date match aggregate query * Added function to parse match aggregate arguments and convert necessary values to Date objects * Added missing return value * Improved code quality based on suggestions and figured out why tests were failing * Added tests from @dplewis * Supporting project aggregation as well as exists operator * Excluding exists match for postgres * Handling the $group operator similar to $match and $project * Added more tests for better code coverage * Excluding certain tests from being run on postgres * Excluding one more test from postgres * clean up

@dplewis

#4743) * #4678: Converting strings to Date when schema.type is Date within aggregate function * Added test cases to test new date match aggregate query * Added function to parse match aggregate arguments and convert necessary values to Date objects * Added missing return value * Improved code quality based on suggestions and figured out why tests were failing * Added tests from @dplewis * Supporting project aggregation as well as exists operator * Excluding exists match for postgres * Handling the $group operator similar to $match and $project * Added more tests for better code coverage * Excluding certain tests from being run on postgres * Excluding one more test from postgres * clean up

@dplewis

…Date within agg… (parse-community#4743) * parse-community#4678: Converting strings to Date when schema.type is Date within aggregate function * Added test cases to test new date match aggregate query * Added function to parse match aggregate arguments and convert necessary values to Date objects * Added missing return value * Improved code quality based on suggestions and figured out why tests were failing * Added tests from @dplewis * Supporting project aggregation as well as exists operator * Excluding exists match for postgres * Handling the $group operator similar to $match and $project * Added more tests for better code coverage * Excluding certain tests from being run on postgres * Excluding one more test from postgres * clean up

parseplatformorg · 2021-11-01T16:37:15Z

🎉 This change has been released in version 5.0.0-beta.1

parseplatformorg · 2022-03-14T13:09:20Z

🎉 This change has been released in version 5.0.0

#4678: Converting strings to Date when schema.type is Date within agg…

fa8c902

…regate function

flovilmart suggested changes Apr 30, 2018

View reviewed changes

cjbland mentioned this pull request Apr 30, 2018

Aggregation match doesn't seem to work with date value #4678

Closed

Added test cases to test new date match aggregate query

9a12b7c

cjbland added 2 commits May 1, 2018 18:38

Added function to parse match aggregate arguments and convert necessa…

044453a

…ry values to Date objects

Merge branch 'master' into fix/4678

90b43ba

flovilmart reviewed May 1, 2018

View reviewed changes

cjbland added 2 commits May 1, 2018 20:04

Added missing return value

1714711

Merge branch 'fix/4678' of https://github.com/cjbland/parse-server in…

867863e

…to fix/4678

flovilmart suggested changes May 2, 2018

View reviewed changes

Improved code quality based on suggestions and figured out why tests …

d922a7a

…were failing

cjbland added 3 commits May 19, 2018 14:25

Added tests from @dplewis

7261ba0

Supporting project aggregation as well as exists operator

beb54bb

Merge branch 'master' into fix/4678

403e81b

cjbland commented May 19, 2018

View reviewed changes

cjbland added 2 commits May 19, 2018 14:33

Merge branch 'master' into fix/4678

9526c65

Merge branch 'fix/4678' of https://github.com/cjbland/parse-server in…

7993f45

…to fix/4678

cjbland added 2 commits June 20, 2018 19:39

Excluding certain tests from being run on postgres

cc25079

Excluding one more test from postgres

1a51a82

clean up

af531c1

flovilmart approved these changes Jun 26, 2018

View reviewed changes

Merge branch 'master' into fix/4678

6b8fded

Merge branch 'master' into fix/4678

f8d7826

dplewis approved these changes Jun 26, 2018

View reviewed changes

dplewis merged commit 5cdbde2 into parse-community:master Jun 26, 2018

snyk-bot mentioned this pull request Apr 1, 2021

[Snyk] Upgrade apollo-server-express from 2.21.0 to 2.21.1 #7308

Merged

parseplatformorg added the state:released-beta Released as beta version label Nov 1, 2021

parseplatformorg added the state:released Released as stable version label Mar 14, 2022

#4678: Converting strings to Date when schema.type is Date within agg… #4743

#4678: Converting strings to Date when schema.type is Date within agg… #4743

Conversation

cjbland commented Apr 30, 2018

flovilmart left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 30, 2018 • edited Loading

Codecov Report

cjbland commented Apr 30, 2018

flovilmart commented May 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flovilmart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flovilmart commented May 2, 2018

cjbland commented May 2, 2018

dplewis commented May 2, 2018 • edited Loading

flovilmart commented May 2, 2018

dplewis commented May 2, 2018

cjbland commented May 3, 2018

dplewis commented May 3, 2018

cjbland commented May 4, 2018 • edited Loading

dplewis commented May 16, 2018

cjbland commented May 17, 2018

cjbland left a comment

Choose a reason for hiding this comment

cjbland commented Jun 20, 2018

cjbland commented Jun 20, 2018

dplewis commented Jun 20, 2018

cjbland commented Jun 22, 2018

dplewis commented Jun 26, 2018

flovilmart left a comment

Choose a reason for hiding this comment

dplewis commented Jun 26, 2018

cjbland commented Jun 26, 2018

flovilmart commented Jun 26, 2018

EricNetsch commented Aug 12, 2018 • edited Loading

flovilmart commented Aug 12, 2018

EricNetsch commented Aug 12, 2018

flovilmart commented Aug 12, 2018

EricNetsch commented Aug 12, 2018

flovilmart commented Aug 12, 2018

parseplatformorg commented Nov 1, 2021

parseplatformorg commented Mar 14, 2022

codecov bot commented Apr 30, 2018 •

edited

Loading

dplewis commented May 2, 2018 •

edited

Loading

cjbland commented May 4, 2018 •

edited

Loading

EricNetsch commented Aug 12, 2018 •

edited

Loading