Add pipeline key to Aggregate #4959
Conversation
Why isn't this possible? The is redundant and having to support both in the future is kinda messy. |
|
https://github.com/parse-community/Parse-SDK-JS/blob/master/src/RESTController.js#L163 This line here, currently I would need to pass in an array for data. As you can see that won't work. Maybe I'm missing something. |
|
This is very annoying to realize that at this point in time... Because maintaining both implementation is quite a mess and not documented, I'm thinking about potential issues and explainting why the traces in JS don't match the traces in REST. OR we make it a breaking change and we stop supporting without the PIPELINE operator .Which defeats the purpose of the aggregate router as we now have a differentiating key (pipeline). The whole thing seems to be rushed out, and it isn't like the aggregation API hasn't been a source of issues in the past. So my feeling at that point is that we should come back with a proper design for the aggregate API, discuss it before, and look at the impact in the current SDK's. As at this point, it's utterly broken. |
I agree, the Aggregate API has had many unforeseen issues.
I agree with making it a breaking change. Allowing for stages with the same name is what I'm going for right now so the PHP and JS SDK will need an update as well. The aggregate router is there because we aren't returning Parse Objects. It also has a distinct operator as well. |
src/Routers/AggregateRouter.js
Outdated
| pipeline = body.map((stage) => { | ||
| if (Array.isArray(data)) { | ||
| pipeline = data.map((stage) => { | ||
| const stageName = Object.keys(stage)[0]; |
There was a problem hiding this comment.
why is it always the 1st key of the stage? What if the stage object has many keys? What is the shape of that object
There was a problem hiding this comment.
The 1st key is always the operator like $group, $match, $project.
src/Routers/AggregateRouter.js
Outdated
|
|
||
| if (Array.isArray(body)) { | ||
| pipeline = body.map((stage) => { | ||
| if (Array.isArray(data)) { |
There was a problem hiding this comment.
the names are very confusing here.
We extract pipeline from body, but then it's data, and it's mapped back as an array etc...
Also the this.transformStage(stageName, data) should only be called at one site.
My recommendation,
if it's an object map into an array
then transform that array into the proper pipeline option.
let pipeline = body.pipeline || body;
if (!Array.isArray(pipeline)) {
pipeline = Object.keys(pipeline).map((key) => { [key]: pipeline[key] })
}
options.pipeline = pipeline.map((stage) => {
const keys = Object.keys(state);
if (keys.length != 1) { throw new Error(`Pipeline stages should only have one key found ${keys.join(' ')}`) }
return this.transformStage(keys[0], stage);
});
src/Routers/AggregateRouter.js
Outdated
| handleFind(req) { | ||
| const body = Object.assign(req.body, ClassesRouter.JSONFromQuery(req.query)); | ||
| const options = {}; | ||
| const data = body.pipeline || body; |
There was a problem hiding this comment.
Add a note here that originally the body could be passed as a single object, and now we support many options and please detail the options, and add a test for each possiblity.
Perhaps extract a single function that maps the request body to a pipeline like
getPipeline(body) {
//
}
transformStage(stage) {}
Consider making them static if needed as they don't need 'this'
|
@dplewis so the new API should aloow only for: ?? |
|
Yes that is correct |
Codecov Report
@@ Coverage Diff @@
## master #4959 +/- ##
==========================================
- Coverage 94.28% 94.26% -0.02%
==========================================
Files 120 120
Lines 8744 8743 -1
==========================================
- Hits 8244 8242 -2
- Misses 500 501 +1
Continue to review full report at Codecov.
|
|
How does this look? |
|
Thanks for the help! |
* Add pipeline key to Aggregate * clean up * unit tests
Fix for parse-community/Parse-SDK-JS#637
In JS SDK I can't pass an array into the payload. So I added a key to allow for this.