Skip to content

fix: filter out {} stages from the view pipeline (COMPASS-4977) #2408

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Aug 20, 2021
Merged

fix: filter out {} stages from the view pipeline (COMPASS-4977) #2408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 15 additions & 1 deletion packages/compass-aggregations/src/modules/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ const debug = require('debug')('mongodb-aggregations:modules:index');

import { combineReducers } from 'redux';
import { ObjectId } from 'bson';
import isEmpty from 'lodash.isempty';

import dataService, { INITIAL_STATE as DS_INITIAL_STATE } from './data-service';
import fields, { INITIAL_STATE as FIELDS_INITIAL_STATE } from './fields';
Expand Down Expand Up @@ -675,6 +676,19 @@ export const getPipelineFromIndexedDB = (pipelineId) => {
};
};


/**
* Make view pipeline.
*
* @returns {Array} The mapped/filtered view pipeline.
*/
export const makeViewPipeline = (unfilteredPipeline) => {
return unfilteredPipeline
.map((p) => (p.executor || generateStage(p)))
// generateStage can return {} under various conditions
.filter((stage) => !isEmpty(stage));
Comment on lines +688 to +689
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can change generateStage in a way that returns something that is easier to identify as an empty stage (like null or something) and then we don't need this dependency on isEmpty to do the check (lodash is used a lot across the codebase, but we are trying to use it less recently and remove it where appropriate), wdyt?

Copy link
Contributor Author

@lerouxb lerouxb Aug 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I considered that and had a look at the other places (outside of tests) that call generateStage():

We'd have to change those to turn null back into {} because that seems to be what they expect. So I'm not sure - it seems debatable either way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense, thanks for the context!

};

/**
* Open create view.
*
Expand All @@ -686,7 +700,7 @@ export const openCreateView = () => {
return (dispatch, getState) => {
const state = getState();
const sourceNs = state.namespace;
const sourcePipeline = state.pipeline.map((p) => (p.executor || generateStage(p)));
const sourcePipeline = makeViewPipeline(state.pipeline);

const meta = {
source: sourceNs,
Expand Down
18 changes: 18 additions & 0 deletions packages/compass-aggregations/src/modules/index.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import reducer, {
clonePipeline,
newPipeline,
modifySource,
makeViewPipeline,
RESET,
CLEAR_PIPELINE,
RESTORE_PIPELINE,
Expand Down Expand Up @@ -256,4 +257,21 @@ describe('root [ module ]', () => {
});
});
});

describe('#makeViewPipeline', () => {
it('filters out empty stages', () => {
const pipeline = [
// executor preferred
{ executor: { a: 1 } },

// falling back to generateStage()
{ isEnabled: false}, // !isEnabled
{}, // no stageOperator
{ stage: '' } // stage === ''
// leaving out non-blank generated ones for generateStage()'s own unit tests
];

expect(makeViewPipeline(pipeline)).to.deep.equal([{ a: 1 }]);
});
});
});