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

Make LineUp v4 RegExp filter serialization backwards compatible #345

Merged
merged 26 commits into from
Apr 6, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
7235fa3
Export serialize and restore RegExp function
thinkh Mar 30, 2020
c92f0e9
Add jest tests for serialize/restoreRegExp
thinkh Mar 30, 2020
ed4e0ca
Refactor `serializeLineUpFilter` for LineUp v4
thinkh Mar 30, 2020
fcd5d7d
Ignore number columns filters
oltionchampari Mar 31, 2020
a5a1a9c
Serialize and restore only regex filters
oltionchampari Mar 31, 2020
103e909
Add ILineUpStringFilterValue type
oltionchampari Mar 31, 2020
502eace
Check if filter is a
oltionchampari Mar 31, 2020
a6b1777
Adapt restoreRegexp to pass
oltionchampari Mar 31, 2020
942e1d2
Add addittional tests
oltionchampari Mar 31, 2020
9f7b022
Add comment
oltionchampari Mar 31, 2020
ab6b164
Rename function
oltionchampari Apr 1, 2020
a4c34f2
Restore filter only when cmd is filter
oltionchampari Apr 1, 2020
e508584
Refactoring
oltionchampari Apr 1, 2020
77f5def
Include case filter being null
oltionchampari Apr 1, 2020
0aa6348
Adapt tests
oltionchampari Apr 1, 2020
d4280ef
Remove comments
oltionchampari Apr 1, 2020
e117ac9
Minor change
oltionchampari Apr 1, 2020
23c9e03
Merge branch lineupjs4
oltionchampari Apr 2, 2020
ddb0f56
Use enum to get filter value
thinkh Apr 2, 2020
f9b7e43
Throw error when restoring unknown filter values
thinkh Apr 2, 2020
5f5bae1
Extract cmd filter impl to own file + move test
thinkh Apr 2, 2020
1d72e89
Test restore for regex filter values
oltionchampari Apr 3, 2020
09c65c5
Adapt implementation to account for edge case
oltionchampari Apr 3, 2020
2f16945
Comment
oltionchampari Apr 3, 2020
a4bf429
Improve type guards and refactor code
thinkh Apr 3, 2020
304e51f
Add type guard filter + refactore restore regexp
thinkh Apr 3, 2020
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
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@
"ts-loader": "4.0.1",
"tslib": "~1.11.0",
"tslint": "5.9.1",
"ts-jest": "^25.2.1",
"ts-jest": "25.2.1",
"typedoc": "~0.16.10",
"typescript": "~3.8.2",
"url-loader": "0.5.8",
Expand Down
10 changes: 5 additions & 5 deletions src/lineup/internal/OverviewColumn.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,12 @@ export default class OverviewColumn extends BooleanColumn {

constructor(id: string, desc: IBooleanColumnDesc) {
super(id, Object.assign(desc, {
label: i18n.t('tdp:core.lineup.OverviewColumn.overviewSelection')
label: i18n.t('tdp:core.lineup.OverviewColumn.overviewSelection'),
renderer: 'boolean',
groupRenderer: 'boolean',
summaryRenderer: 'categorical'
}));
(<OverviewColumn>this).setDefaultRenderer('boolean');
(<OverviewColumn>this).setDefaultGroupRenderer('boolean');
(<OverviewColumn>this).setDefaultSummaryRenderer('categorical');
(<OverviewColumn>this).setWidthImpl(0); // hide
this.setWidthImpl(0); // hide
}

getValue(row: IDataRow) {
Expand Down
70 changes: 13 additions & 57 deletions src/lineup/internal/cmds.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {IObjectRef, action, meta, cat, op, ProvenanceGraph, ICmdResult} from 'ph
import {NumberColumn, LocalDataProvider, StackColumn, ScriptColumn, OrdinalColumn, CompositeColumn, Ranking, ISortCriteria, Column, isMapAbleColumn, mappingFunctions} from 'lineupjs';
import {resolveImmediately} from 'phovea_core/src';
import i18n from 'phovea_core/src/i18n';
import {isSerializedFilter, restoreLineUpFilter, serializeLineUpFilter, isLineUpStringFilter} from './cmds/filter';


// used for function calls in the context of tracking or untracking actions in the provenance graph in order to get a consistent defintion of the used strings
Expand Down Expand Up @@ -220,9 +221,15 @@ export async function setColumnImpl(inputs: IObjectRef<any>[], parameter: any) {
bak = source[`getRenderer`]();
source[`setRenderer`].call(source, parameter.value);
break;
case LineUpTrackAndUntrackActions.filter:
bak = source[`get${prop}`]();
// restore serialized regular expression before passing to LineUp
const value = isSerializedFilter(parameter.value) ? restoreLineUpFilter(parameter.value) : parameter.value;
source[`set${prop}`].call(source, value);
break;
default:
bak = source[`get${prop}`]();
source[`set${prop}`].call(source, restoreRegExp(parameter.value)); // restore serialized regular expression before passing to LineUp
source[`set${prop}`].call(source, parameter.value);
break;
}
}
Expand Down Expand Up @@ -349,80 +356,29 @@ function recordPropertyChange(source: Column | Ranking, provider: LocalDataProvi
return;
}

const newSerializedValue = serializeRegExp(newValue); // serialize possible RegExp object to be properly stored as provenance graph
if (property === LineUpTrackAndUntrackActions.filter) {
newValue = isLineUpStringFilter(newValue) ? serializeLineUpFilter(newValue) : newValue; // serialize possible RegExp object to be properly stored as provenance graph
}

if (source instanceof Column) {
// assert ALineUpView and update the stats
lineupViewWrapper.value.getInstance().updateLineUpStats();

const rid = rankingId(provider, source.findMyRanker());
const path = source.fqpath;
graph.pushWithResult(setColumn(lineupViewWrapper, rid, path, property, newSerializedValue), {
graph.pushWithResult(setColumn(lineupViewWrapper, rid, path, property, newValue), {
inverse: setColumn(lineupViewWrapper, rid, path, property, old)
});
} else if (source instanceof Ranking) {
const rid = rankingId(provider, source);
graph.pushWithResult(setColumn(lineupViewWrapper, rid, null, property, newSerializedValue), {
graph.pushWithResult(setColumn(lineupViewWrapper, rid, null, property, newValue), {
inverse: setColumn(lineupViewWrapper, rid, null, property, old)
});
}
};
source.on(`${property}Changed.track`, delayed > 0 ? delayedCall(f, delayed) : f);
}

/**
* Serialize RegExp objects from LineUp string columns as plain object
* that can be stored in the provenance graph
*/
interface IRegExpFilter {
/**
* RegExp as string
*/
value: string;
/**
* Flag to indicate the value should be restored as RegExp
*/
isRegExp: boolean;
}

/**
* Serializes RegExp objects to an IRegexFilter object, which can be stored in the provenance graph.
* In case a string is passed to this function no serialization is applied.
*
* Background information:
* The serialization step is necessary, because RegExp objects are converted into an empty object `{}` on `JSON.stringify`.
* ```
* JSON.stringify(/^123$/gm); // result: {}
* ```
*
* @param value Input string or RegExp object
* @returns {string | IRegExpFilter} Returns the input string or a plain `IRegExpFilter` object
*/
function serializeRegExp(value: string | RegExp): string | IRegExpFilter {
if (!(value instanceof RegExp)) {
return value;
}
return {value: value.toString(), isRegExp: true};
}

/**
* Restores a RegExp object from a given IRegExpFilter object.
* In case a string is passed to this function no deserialization is applied.
*
* @param filter Filter as string or plain object matching the IRegExpFilter
* @returns {string | RegExp| null} Returns the input string or the restored RegExp object
*/
function restoreRegExp(filter: string | IRegExpFilter): string | RegExp {
if (filter === null || !(<IRegExpFilter>filter).isRegExp) {
return <string | null>filter;
}

const serializedRegexParser = /^\/(.+)\/(\w+)?$/; // from https://gist.github.com/tenbits/ec7f0155b57b2d61a6cc90ef3d5f8b49
const matches = serializedRegexParser.exec((<IRegExpFilter>filter).value);
const [_full, regexString, regexFlags] = matches;
return new RegExp(regexString, regexFlags);
}

function trackColumn(provider: LocalDataProvider, lineup: IObjectRef<IViewProvider>, graph: ProvenanceGraph, col: Column) {
recordPropertyChange(col, provider, lineup, graph, LineUpTrackAndUntrackActions.metaData);
recordPropertyChange(col, provider, lineup, graph, LineUpTrackAndUntrackActions.filter);
Expand Down
175 changes: 175 additions & 0 deletions src/lineup/internal/cmds/filter.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,175 @@
/**
* Basic LineUp string filter values
*/
type LineUpStringFilterValue = string[] | string | null;

/**
* This type guard checks if `filter` parameter matches the `LineUpStringFilterValue` type
* @param filter Any filterable value that should be checked
* @returns Returns true if filter applies to the `LineUpStringFilterValue`
*/
function isLineUpStringFilterValue(filter: any): filter is LineUpStringFilterValue {
return filter === null || typeof filter === 'string' || Array.isArray(filter);
}

/**
* Serialize RegExp objects from LineUp string columns as plain object
* that can be stored in the provenance graph
*/
interface IRegExpFilter {
/**
* RegExp as string
*/
value: LineUpStringFilterValue;
/**
* Flag to indicate the value should be restored as RegExp
*/
isRegExp: boolean;
}

/**
* This type guard checks if `filter` parameter matches the `IRegExpFilter` type.
* @param filter Any filterable value that should be checked
* @returns Returns true if filter applies to the `IRegExpFilter`
*/
function isIRegExpFilter(filter: any): filter is IRegExpFilter {
return filter && filter.hasOwnProperty('value') && isLineUpStringFilterValue(filter.value) && filter.hasOwnProperty('isRegExp');
}

/**
* This interface combines the `IStringFilter` from `StringColumn`
* and `ICategoricalFilter` from `ICategoricalColumn`.
*/
interface ILineUpStringFilter {
/**
* Filter value
*/
filter: LineUpStringFilterValue | RegExp;

/**
* Filter for missing values
*/
filterMissing: boolean;
}

/**
* This type guard checks if the `filter` parameter matches the `isLineUpStringFilter` type.
*
* @internal
* @param filter Any value that could be a filter
* @returns Returns true if filter should be serialized/restored or false if not.
*/
export function isLineUpStringFilter(filter: any): filter is ILineUpStringFilter {
return filter && filter.hasOwnProperty('filter') && (isLineUpStringFilterValue(filter.filter) || filter.filter instanceof RegExp) && filter.hasOwnProperty('filterMissing');
}


/**
* Similar to the `ILineUpStringFilter`, but the RegExp is replaced with `IRegExpFilter`
*/
interface ISerializableLineUpFilter {
/**
* Filter value
* Note that the RegExp is replaced with IRegExpFilter (compared to the `ILineUpStringFilter` interface)
*/
filter: LineUpStringFilterValue | IRegExpFilter;

/**
* Filter for missing values
*/
filterMissing: boolean;
}

/**
* This type guard checks if the `filter` parameter matches the `ISerializableLineUpFilter` type.
* Necessary since number columns filter has properties `min`, `max` and no filter property.
*
* @internal
* @param filter Any value that could be a filter
thinkh marked this conversation as resolved.
Show resolved Hide resolved
* @returns Returns true if filter should be serialized/restored or false if not.
*/
export function isSerializedFilter(filter: any): filter is ISerializableLineUpFilter {
return filter && filter.hasOwnProperty('filter') && (isLineUpStringFilterValue(filter.filter) || isIRegExpFilter(filter.filter)) && filter.hasOwnProperty('filterMissing');
}

/**
* Serializes LineUp string filter, which can contain RegExp objects to an IRegexFilter object.
* The return value of this function can be passed to `JSON.stringify()` and stored in the provenance graph.
*
* Background information:
* The serialization step is necessary, because RegExp objects are converted into an empty object `{}` on `JSON.stringify`.
* ```
* JSON.stringify(/^123$/gm); // result: {}
* ```
*
* @internal
*
* @param filter LineUp filter object
* @returns Returns the `ISerializableLineUpFilter` object
*/
export function serializeLineUpFilter(filter: ILineUpStringFilter): ISerializableLineUpFilter {
const value = filter.filter;
const isRegExp = value instanceof RegExp;
return {
filter: {
value: isRegExp ? value.toString() : value as LineUpStringFilterValue,
isRegExp
},
filterMissing: filter.filterMissing
};
}

/**
* Coverts a RegExp string to a RegExp instance
*
* @internal
* @param value RegExp string
* @returns The RegExp instance
*/
export function restoreRegExp(value: string): RegExp {
const serializedRegexParser = /^\/(.+)\/(\w+)?$/; // from https://gist.github.com/tenbits/ec7f0155b57b2d61a6cc90ef3d5f8b49
const matches = serializedRegexParser.exec(value);

if(matches === null) {
throw new Error('Unable to parse regular expression from string. The string does not seem to be a valid RegExp.');
}

const [_full, regexString, regexFlags] = matches;
return new RegExp(regexString, regexFlags);
}

/**
* Restores filter values from the provenance graph and returns an `ILineUpStringFilter`
* which can be passed to the LineUp instance (using LineUp > 4.0.0).
*
* Valid seralized filter values are:
* - `LineUpStringFilterValue` used with LineUp < 4.0.0 and tdp_core < 9.0.0
* - `IRegExpFilter` used with LineUp < 4.0.0 and tdp_core >= 9.0.0
* - `ISerializableLineUpFilter` used with LineUp > 4.0.0 and tdp_core > 9.1.0
*
* @interal
* @param filter Filter with one of the types described above
* @param filterMissing The flag indicates if missing values should be filtered (default = `false`)
* @returns Returns an `ILineUpStringFilter` which can be passed to LineUp
*/
export function restoreLineUpFilter(filter: LineUpStringFilterValue | IRegExpFilter | ISerializableLineUpFilter, filterMissing = false): ILineUpStringFilter {
if (isLineUpStringFilterValue(filter)) {
return {filter, filterMissing};

} else if (isIRegExpFilter(filter) && filter.isRegExp === true) {
if(typeof filter.value === 'string') {
return {filter: restoreRegExp(filter.value), filterMissing};
}

throw new Error('Wrong type of filter value. Unable to restore RegExp instance from the given filter value.');

} else if (isIRegExpFilter(filter) && filter.isRegExp === false) {
return restoreLineUpFilter(filter.value, filterMissing);

} else if (isSerializedFilter(filter)) {
return restoreLineUpFilter(filter.filter, filter.filterMissing);

}

throw new Error('Unknown LineUp filter format. Unable to restore the given filter.');
Copy link
Member

Choose a reason for hiding this comment

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

I added this error message to prevent cases where the return value of this function is void. After playing around with this implementation I got an error with the regexp string:

ordino-lineup-unknown-filter-format

This is the error in the console:

filter.ts:125 Uncaught (in promise) Error: Unknown LineUp filter format. Unable to restore the given filter.
    at restoreLineUpFilter (filter.ts:125)
    at restoreLineUpFilter (filter.ts:121)
    at ActionNode.<anonymous> (cmds.ts:227)
    at Generator.next (<anonymous>)
    at fulfilled (tslib.es6.js:71)

@oltionchampari Can you check why calling it recursively (starting from line 121) and not returning void (without the error) and now we run into the error? In guess is that we still have an uncovered case where the three typeguards above are not working correctly.

My expectation would be that we do not get the error message and one of the typeguards catches the recursive call and returns a valid filter object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue was that when you go back in the provenance graph the restoreLineUpFilter()
function gets called with the old value which gets derived from the Ranking.Thus it hasn't been serialized:

const fromGraphFilter = {filter: /abc/, filterMissing: false};
  restoreLineUpFilter(fromGraphFilter)

I added a test to cover this case and returned the filter when it's already serialized.

}
Loading