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.

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

Jump to bottom

UI-8742 - More robust MDX cleanup #115

Merged
merged 8 commits into from
Sep 26, 2023
Merged

UI-8742 - More robust MDX cleanup #115

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3f04f98
UI-8742 - More robust MDX cleanup
antoinetissier Sep 20, 2023
66f86d6
Bump to latest versions of the SDK
antoinetissier Sep 25, 2023
c2b436b
add robustness
antoinetissier Sep 25, 2023
90ca332
format test snapshot
antoinetissier Sep 25, 2023
30f7167
Merge branch 'main' into personal/a.tissier/UI-8742-more-robust-mdx-c…
antoinetissier Sep 25, 2023
f747c99
PR feedback
antoinetissier Sep 26, 2023
cb3e7c4
Update src/4.3_to_5.0/_cleanupDrilldownLevel.ts
antoinetissier Sep 26, 2023
605b821
Update src/4.3_to_5.0/_cleanupDrilldownLevel.ts
antoinetissier Sep 26, 2023
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
4 changes: 4 additions & 0 deletions __mocks__/clipboard-polyfill/text.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
// This module is a transitive dependency of @activeviam packages.
// It is used in the browser in Atoti UI, but it is not necessary for atoti-ui-migration.
// See https://github.com/activeviam/atoti-ui-migration/blob/c06e91517d4898bf54e919944e49fae6e270036c/jest.config.js#L7-L8
module.exports = {};
18 changes: 9 additions & 9 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,15 @@
"trailingComma": "all"
},
"devDependencies": {
"@activeviam/activeui-sdk-5.0": "npm:@activeviam/[email protected]20230915115910-fc34c05",
"@activeviam/activeui-sdk-5.1": "npm:@activeviam/[email protected].8",
"@activeviam/activeui-sdk-scripts": "5.0.29-20230915115910-fc34c05",
"@activeviam/content-server-initialization-5.0": "npm:@activeviam/[email protected]20230915115910-fc34c05",
"@activeviam/dashboard-base-5.1": "npm:@activeviam/[email protected].8",
"@activeviam/data-model-5.0": "npm:@activeviam/[email protected]20230915115910-fc34c05",
"@activeviam/data-model-5.1": "npm:@activeviam/[email protected].8",
"@activeviam/mdx-5.0": "npm:@activeviam/[email protected]20230915115910-fc34c05",
"@activeviam/mdx-5.1": "npm:@activeviam/[email protected].8",
"@activeviam/activeui-sdk-5.0": "npm:@activeviam/[email protected]20230925104351-05a6f39",
"@activeviam/activeui-sdk-5.1": "npm:@activeviam/[email protected].9-20230925084242-888ae49",
"@activeviam/activeui-sdk-scripts": "5.0.29-20230925104351-05a6f39",
"@activeviam/content-server-initialization-5.0": "npm:@activeviam/[email protected]20230925104351-05a6f39",
"@activeviam/dashboard-base-5.1": "npm:@activeviam/[email protected].9-20230925084242-888ae49",
"@activeviam/data-model-5.0": "npm:@activeviam/[email protected]20230925104351-05a6f39",
"@activeviam/data-model-5.1": "npm:@activeviam/[email protected].9-20230925084242-888ae49",
"@activeviam/mdx-5.0": "npm:@activeviam/[email protected]20230925104351-05a6f39",
"@activeviam/mdx-5.1": "npm:@activeviam/[email protected].9-20230925084242-888ae49",
"@swc/core": "^1.3.31",
"@swc/jest": "^0.2.24",
"@types/cli-progress": "^3.11.0",
Expand Down
4 changes: 2 additions & 2 deletions src/4.3_to_5.0/_cleanupDescendants.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ describe("_cleanupDescendants", () => {
NON EMPTY Hierarchize(
Descendants(
{
[Booking].[Desk].[ALL].[AllMember].[LegalEntityA]
[Booking].[Desk].[ALL].[AllMember].[LegalEntityA].[BusinessUnitA].[DeskA].[0]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We were actually checking something invalid with the previous input:

Descendants(
              {
                [Booking].[Desk].[ALL].[AllMember].[LegalEntityA]
               },
               1
)

should NOT be replaced by just LegalEntityA.
Indeed the level at distance 1 from LegalEntityA is BusinessUnit.

However in the new test input:

Descendants(
              {
                [Booking].[Desk].[ALL].[AllMember].[LegalEntityA].[BusinessUnitA].[DeskA].[0]
               },
               1
)

the Descendants node should be replaced by the 0 BookId, since it doesn't have descendants anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we also keep a test for the LegalEntityA case which seems to have been broken but that you fixed in this PR (right?)

},
1
)
Expand All @@ -71,7 +71,7 @@ describe("_cleanupDescendants", () => {
"SELECT
NON EMPTY Hierarchize(
{
[Booking].[Desk].[ALL].[AllMember].[LegalEntityA]
[Booking].[Desk].[ALL].[AllMember].[LegalEntityA].[BusinessUnitA].[DeskA].[0]
}
) ON ROWS
FROM [EquityDerivativesCube]"
Expand Down
61 changes: 18 additions & 43 deletions src/4.3_to_5.0/_cleanupDescendants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,11 @@
import { MdxDrillthrough, MdxSelect, Cube } from "@activeviam/activeui-sdk-5.0";
import { getHierarchy, getLevelIndex } from "@activeviam/data-model-5.0";
import { Mdx, Cube } from "@activeviam/activeui-sdk-5.0";
import { getLevelIndex } from "@activeviam/data-model-5.0";
import {
MdxFunction,
findDescendant,
getHierarchies,
getLevels,
isMdxCompoundIdentifier,
isMdxFunction,
isMdxLiteral,
stringify,
getIndexOfDeepestLevelExpressedInDescendantsNode,
} from "@activeviam/mdx-5.0";
import { produce } from "immer";
import _set from "lodash/set";
Expand All @@ -20,50 +17,31 @@ const canDescendantsBeReplacedByItsFirstArgument = (
descendantsNode: MdxFunction,
cube: Cube,
) => {
const [set, downToLevel] = descendantsNode.arguments;
const set = descendantsNode.arguments[0];
const levelsInSet = getLevels(set, { cube });

// If no levels are found in the input set, for instance when it consists only in the `CurrentMember` of a hierarchy (which could be on any level), the `Descendants` function should not be removed.
if (levelsInSet.length === 0) {
// If no levels are found in the input set, for instance when it consists only in the `CurrentMember` of a hierarchy (which could be on any level), the `Descendants` function should not be removed.
return false;
}

const shallowestLevelIndexInSet = Math.min(
...levelsInSet.map((levelCoordinates) =>
getLevelIndex({ cube, ...levelCoordinates }),
),
);
const { dimensionName, hierarchyName } = levelsInSet[0];

if (!downToLevel) {
// Only the set as argument: check whether it holds only leaf members.
const hierarchyCoordinates = getHierarchies(set, { cube })[0];
const hierarchy = getHierarchy(hierarchyCoordinates, cube);
return shallowestLevelIndexInSet === hierarchy.levels.length - 1;
}
const levelIndexesInSet = levelsInSet.map((levelCoordinates) =>
getLevelIndex({ cube, ...levelCoordinates }),
);
const shallowestLevelIndexInSet = Math.min(...levelIndexesInSet);
const deepestLevelIndexInSet = Math.max(...levelIndexesInSet);

let downToLevelIndex;
if (isMdxLiteral(downToLevel)) {
downToLevelIndex = parseInt(downToLevel.value, 10);
} else {
if (!isMdxCompoundIdentifier(downToLevel)) {
throw new Error(
`Invalid second argument of Descendants. Expected a level index or a level unique name, but got: ${stringify(
downToLevel,
{ indent: true },
)}`,
);
}
const [dimensionName, hierarchyName, levelName] =
downToLevel.identifiers.map((identifier) => identifier.value);
downToLevelIndex = getLevelIndex({
cube,
const deepestLevelExpressedInDescendantsNode =
getIndexOfDeepestLevelExpressedInDescendantsNode({
descendantsNode,
dimensionName,
hierarchyName,
levelName,
indexOfDeepestLevelExpressedInInputSet: deepestLevelIndexInSet,
cube,
});
}

return shallowestLevelIndexInSet >= downToLevelIndex;
return shallowestLevelIndexInSet >= deepestLevelExpressedInDescendantsNode;
};

/**
Expand All @@ -73,10 +51,7 @@ const canDescendantsBeReplacedByItsFirstArgument = (
* Descendants([Currency].[Currency].[ALL].[AllMember].[EUR], [Currency].[Currency].[Currency]) => [Currency].[Currency].[ALL].[AllMember].[EUR]
*
*/
export function _cleanupDescendants<T extends MdxSelect | MdxDrillthrough>(
mdx: T,
cube: Cube,
): T {
export function _cleanupDescendants<T extends Mdx>(mdx: T, cube: Cube): T {
return produce(mdx, (draft) => {
let nextNodeToCleanup;
while (
Expand Down
25 changes: 20 additions & 5 deletions src/4.3_to_5.0/_cleanupDrilldownLevel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
import { Mdx, Cube } from "@activeviam/activeui-sdk-5.0";
import { getHierarchy, getLevelIndex } from "@activeviam/data-model-5.0";
import {
getHierarchy,
getLevelIndex,
areHierarchiesEqual,
} from "@activeviam/data-model-5.0";
import {
MdxFunction,
findDescendant,
Expand All @@ -17,15 +21,26 @@ const canDrilldownLevelBeReplacedByItsFirstArgument = (
drilldownLevelNode: MdxFunction,
cube: Cube,
) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function seems to have been improved in some cases. Don't clean up drilldownlevel if:

  • it has several arguments
  • something about the levels expressed in the set that are not on the same hierarchy as the first hierarchy found in the set (not sure what)

Can you add unit tests in order to illustrate and test these improvements?

Copy link
Collaborator Author

@antoinetissier antoinetissier Sep 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Replace if both these conditions are met:

  • the function has a single argument
  • in the input set, the deepest level expressed in the first hierarchy (which is the one being implicitly drilled) is a leaf level

I'll add a comment.

const set = drilldownLevelNode.arguments[0];
const levelsInSet = getLevels(set, { cube });
const [set, ...otherArguments] = drilldownLevelNode.arguments;
if (otherArguments.length > 0) {
// Do not attempt to cleanup the more complex DrilldownLevel expressions.
return false;
}

const hierarchyCoordinates = getHierarchies(set, { cube })[0];
const hierarchy = getHierarchy(hierarchyCoordinates, cube);

const levelsInSet = getLevels(set, {
cube,
}).filter((levelCorodinates) =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
}).filter((levelCorodinates) =>
}).filter((levelCoordinates) =>

areHierarchiesEqual(levelCorodinates, hierarchyCoordinates),
);
const deepestLevelIndexInSet = Math.max(
...levelsInSet.map((levelCoordinates) =>
getLevelIndex({ cube, ...levelCoordinates }),
),
);
const hierarchyCoordinates = getHierarchies(set, { cube })[0];
const hierarchy = getHierarchy(hierarchyCoordinates, cube);

return deepestLevelIndexInSet === hierarchy.levels.length - 1;
};

Expand Down
11 changes: 5 additions & 6 deletions src/4.3_to_5.0/_fixErroneousExpansionMdx.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,15 @@ import {
* But the returned mdx does not have the typical useless (and dangerous) parts of queries created by collapsing then re-expanding a member in ActiveUI 4.
* See https://support.activeviam.com/jira/browse/UI-6692
*/
export function _fixErroneousExpansionMdx(
mdx: MdxSelect | MdxDrillthrough,
cube: Cube,
): MdxSelect | MdxDrillthrough {
export function _fixErroneousExpansionMdx<
T extends MdxSelect | MdxDrillthrough,
>(mdx: T, cube: Cube): T {
if (isMdxDrillthrough(mdx)) {
return mdx;
}

return produce<MdxSelect>(mdx, (draft) => {
draft.axes.forEach((axis) => {
return produce<T>(mdx, (draft) => {
(draft as MdxSelect).axes.forEach((axis) => {
const levelsOnAxis = getLevels(axis, { cube });

let erroneousUnionNode:
Expand Down
24 changes: 13 additions & 11 deletions src/4.3_to_5.0/_migrateQuery.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,19 +77,21 @@ export const _migrateQuery = <T extends MdxSelect | MdxDrillthrough>({
}

const parsedMdx = parse<T>(mdx);
const mdxWithoutObsoleteDescendants = _cleanupDescendants(parsedMdx, cube);
const mdxWithoutObsoleteDrilldownLevel = _cleanupDrilldownLevel(
mdxWithoutObsoleteDescendants,
cube,
);
const fixedMdx = _fixErroneousExpansionMdx(
mdxWithoutObsoleteDrilldownLevel,
cube,
);
const filters = getFilters(fixedMdx, { cube }).map(

let improvedMdx = parsedMdx;
try {
improvedMdx = _cleanupDescendants(improvedMdx, cube);
improvedMdx = _cleanupDrilldownLevel(improvedMdx, cube);
improvedMdx = _fixErroneousExpansionMdx(improvedMdx, cube);
} catch (e) {
// The MDX may be suboptimal, but it is expected to be valid from the perspective of the MDX specification.
// So attempt to improve it, but do not block the migration if the process fails.
}

const filters = getFilters(improvedMdx, { cube }).map(
({ mdx: filterMdx }) => filterMdx,
);
const mdxWithoutFilters = setFilters(fixedMdx, { filters: [], cube });
const mdxWithoutFilters = setFilters(improvedMdx, { filters: [], cube });

// TODO UI-5036 Migrate query ranges.
return [
Expand Down
1 change: 1 addition & 0 deletions webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ module.exports = {
// This is required because of the "@activeviam/activeui-sdk-5.0" dependency.
antd: false,
"@ant-design/icons": false,
"clipboard-polyfill": false,
},
},
mode: "none",
Expand Down
Loading
Loading