Performance issue, readability and extension proposals toOdataString

[hidegh]

PROPOSALS

Up to now, despite all changes the modified toOdataStringEx did not include any breaking changes to existing code! Would consider some extension point via the ODataSettingEx, so that we'd avoid the changes inside implementation.

Performance, readability & extensibility issues

When working with KENDO Angular and OData, I've found the original implementation quiet limiting, so when I was reading the source found some issues:

1. When multiple properties (filter/sort/skip/take) exists in the Status object, each call will trigger the computation of all the values, thus i changed the original rule constant to ruleFn (see solution in the text I've added to this ticket)
2. Generic readability & unnecessary over-complication (see: serializeKey, serializeFilter) - it makes for non JavaScript gurus the code quiet hard to read
3. Miss the possibility to extend
   -- had to change some of the original transformation functions to add extra parameter
   -- and/or had to change the original transformation function to avoid parameter deconstruction and to avoid the use of immutables (this needed from me to do create a copy of State object before doing any calculation, also fixing the multiple execution - see rule vs ruleFn -, but afterwards enriching objects inside State and use those extra parameters inside my own toOdataStringEx was possible)
4. Btw. would also check the UTC date string, it should not be generated via JSON.stringify as that might be overriden (we did that to keep original time-zone values, but of course that valid change within ODATA query expression was not correct) - this dependency should be removed.

OData serializeKey simplification

NEW:
const serializeKey = (keys, value) => isPresent(value) ? keys[0] + value : '';

ORIGINAL:

const either = (predicate, right, left) => value => predicate(value) ? right(value) : left(value);
const isPresent = (value: any): boolean => value !== null && value !== undefined;
const constant = x => () => x;
const emptyString = constant(undefined);
const concat = a => b => a + b;
const serializeKey = (strings, val) => either(isPresent, concat(strings[0]), emptyString)(val);

let key = serializeKey`$skip=${state.skip}`;

COMMENT:
No, it's not a violation of DRY, it's about readability.

IMPORTANT:
An internal refactor and an Fn suffix for those parameters that are expected to be functions, like:
const eitherFn = (predicateFn, rightFn, leftFn) => value => predicateFn(value) ? rightFn(value) : leftFn(value);
...could HELP A BIT with readability, even if interfaces when CURRYING is used is not as helpful as seeing the implementation itself (in our case the: predicateFn(value) ? rightFn(value) : leftFn(value);
...so either(isPresent, concat(strings[0]), emptyString)(val) is not saying much, without that!
Interface for either: (predicate, right, left): (value: any) => any

OData serializeFilter - simplification around serializeAll

COMMENT:
- It's just removing one extra function
- NOT USING CURRYING UNNECESSARILY inside map and join
- making it READABLE
- adding some type-safety checks

NEW:

const filterSerializationFn = (filter: FilterDescriptor) => filterOperators[<string>filter.operator](filter);

const compositeFilterSerializationFn = (filter: CompositeFilterDescriptor): string => {
    let filterExpression =
        filter
            .filters
            // NOTE: either(...) is a fnc that passes the supplied caller arg. (f) as an argument for it's argument (which are) functions!
            .map(f => either(isCompositeFilterDescriptor, compositeFilterSerializationFn, filterSerializationFn)(f))
            .join(` ${filter.logic} `);

    const brackets = wrapIf(() => filter.filters.length > 1);
    return brackets`(${filterExpression})`;
};

export const serializeFilter = (filter: CompositeFilterDescriptor): string => {
    if (filter.filters && filter.filters.length) {
        return "$filter=" + compositeFilterSerializationFn(filter);
    }
    return "";
};

ORIGINAL:

const join = x => ` ${x.logic} `;
const serialize = x => filterOperators[<string>x.operator](x);

const serializeAll = serializeFilters(
     filter => either(isCompositeFilterDescriptor, serializeAll, serialize)(filter),
     join
     );

 // resides in a separate file
 export const serializeFilters = (map, join) => (filter: CompositeFilterDescriptor): string => {
     const brackets = wrapIf(() => filter.filters.length > 1)
     let f =
         filter.filters
             .map(map)
             .join(join(filter));
     return brackets`(${f})`;
 };

export const serializeFilter = (filter: CompositeFilterDescriptor): string => {
    if (filter.filters && filter.filters.length) {
        return "$filter=" + serializeAll(filter);
    }
    return "";
};

OData serialzer - rules - optimization

Since const rules is an object from which we take one property (via key), all the 4 functions are evaluated when object is requested.
Changing property values to functions we can avoid the call to the serialize*** functions, resulting in the execution of just teh selection.

NEW CODE CHANGES:

const rulesFn = (key, state, settings) => ({
    "filter": () => serializeFilter(state.filter || {}, settings),
    "skip": () => serializeKey`$skip=${state.skip}`,
    "sort": () => serializeSort(state.sort || [], settings),
    "take": () => serializeKey`$top=${state.take}`
}[key]);

export const toODataStringEx = (state: State, settings: ODataExSettings = new ODataExSettings(true)): string => {

    // As we use object alteration instead of mutation, this extra step is necessary!
    let stateClone: State = _.cloneDeep(state);

    return Object.keys(stateClone)
        .map(x => rulesFn(x, stateClone, settings)()) /* extra call due rulesFn usage */
        .filter(isNotNullOrEmptyString)
        .join('&');
};

Fixing field normalizers in eq/neq empty/null and simplifying doesnotcontain...

...by introducing const fieldFnExpression = (expression) => ({ field }) => ${field} ${expression} instead of appendEquals and replacing filter operators

doesnotcontain: composeEx(fieldFnExpression("eq -1"), stringFnOperator("indexof")),
isempty: composeEx(fieldFnExpression("eq ''"), normalizeField),
isnotempty: composeEx(fieldFnExpression("ne ''"), normalizeField),
isnotnull: composeEx(fieldFnExpression("ne null"), normalizeField),
isnull: composeEx(fieldFnExpression("eq null"), normalizeField),

Further extensions & limitations:

• applying global settings when serializing SORT to OData (minor: use ASC sort if no sort type is defined to a field)
• applying global settings when serializing FILTER to OData (major: due collation force toLower to value as well; also to set ignoreCase on a global level) - so allow at least one extra param inside functions
• the Transformation functions do not alter the data entered, but returns a new one, thus removing any extra params (yes, I know that we need a copy, but once that is done on the 1st step, we can reuse that object and allowing much more for those doing extensions and re-work)
• as we use object alteration instead on immutable values, we had to clone State in the initial toOdataStringEx() call - need to alter rules to rulesFn or do the clone before each step
• to avoid time-shift due timezones (if date-time has to be fixed to a single time-zone) we use date(datetime2sqlfieldname) eq ISO8601UTCtimevalue

In short, following changes were introduced in my custom solution:

export type Transformation = (f: FilterDescriptorEx, s: ODataExSettings) => FilterDescriptorEx;

/** change from immutables to object altering + ensure object copy creation to support new, non FilterDescriptorEx properties as well */
export const toLowerField: Transformation = (f: FilterDescriptorEx, s: ODataExSettings) => {
    f.field = wrapIf(() => f.ignoreCase || s.filter.globalIgnoreCase) `tolower(${f.field})`;
    return f;
};

/** extended filter serialization where some custom logic had to be introduced (to replace single filters with more complex ones) */
const compositeFilterSerializationFn = (filter: CompositeFilterDescriptor, settings: ODataExSettings): string => {
    let filterExpression =
        filter
            .filters
            .map((f: FilterDescriptor | CompositeFilterDescriptor ) => {

                //
                // handle regular composite filter
                if (isCompositeFilterDescriptor(f))
                    return compositeFilterSerializationFn(f, settings);

                //
                // specific filtering logic

                // handle date from-to to support 1 day selection
                if (isDate(f)
                    && (settings.filter.date24hMatch || (f as any).date24hMatch)
                    && ((f.operator as any).toLowerCase() === "eq" || (f.operator as any).toLowerCase() === "neq")
                    ) {

                    // replace single date-filter expression with a composite one that will consider a whole day (from-to) or outside it...
                    let typedDate = f.value as Date;
                    let fromDate = new Date(typedDate.getFullYear(), typedDate.getMonth() , typedDate.getDate());
                    let toDate = new Date(typedDate.getTime() + 86400000); // get next day: add 86400000ms = 1 day

                    // if date operator is 'eq' we use: date >= from && < to
                    // for the 'ne' we use: date < from && date >= to

                    let operatorLeft = (f.operator as any).toLowerCase() === "eq" ? "gte" : "lt";
                    let operatorRight = (f.operator as any).toLowerCase() === "eq" ? "lt" : "gte";

                    let dayDateCompositeFilter = {
                        logic: "and",
                        filters: [
                            { field: f.field, operator: operatorLeft, value: fromDate } as FilterDescriptor,
                            { field: f.field, operator: operatorRight, value: toDate } as FilterDescriptor
                        ] as FilterDescriptor[]
                    } as CompositeFilterDescriptor;

                    return compositeFilterSerializationFn(dayDateCompositeFilter, settings);
                }

                //
                // handle regular simple filter
                return singleFilterSerializationFn(f, settings);
            })
            .join(` ${filter.logic} `);

    const brackets = wrapIf(() => filter.filters.length > 1);
    return brackets`(${filterExpression})`;
};

My global settings

export class ODataExSettings {

    sort: {
        /** if true, sort by column will be ignored if dir (asc|desc) is not supplied within the SortDescriptor (default Kendo behavior) */
        gridSort: boolean;
    };

    filter: {
        /** if TRUE, will apply toLower always, regardless of the FilterDescriptor ignoreCase setting */
        globalIgnoreCase: boolean;
        /** if TRUE will apply toLower to value, if toLower is applied for the search */
        ignoreValueCase: boolean;
        /** if TRUE and date operator is eq/neq then the whole day will be eq/neq-ed so we will do a value >= day and value < day + 1 or value < day or value >= day + 1 */
        date24hMatch: boolean;
    };

    constructor(caseInsensitive: boolean = true, dateEqNeqDayExtension: boolean = true) {
        this.sort = {
            gridSort: true
        };

        this.filter = {
            globalIgnoreCase: caseInsensitive,
            ignoreValueCase: caseInsensitive,
            date24hMatch: dateEqNeqDayExtension
        };
    }
}

[hidegh]

As from Odata 4.0.1 the in function can be used (so select multiple values - and by multiple i mean > 20 :)

To support operator $field in ($value1, ...$valueN) i had to patch some functions (beyond those described above). Yet due to OData impl. there are some limitations:

1. for the string values we can't use the tolower function - field in (tolower(value1), ... tolower(valueN))
2. the field in (date1) will throw an error (seems it works just for strings and numbers)

Filter sample:

        state.filter.filters = [{
            field: "DispatchStatusID",
            value: [1, 2, 5], // array is now allowed
            operator: "in",
            ignoreCase: true,
        }];

Patches:

export const quote: Transformation = (f: FilterDescriptorEx) => {
    f.value = Array.isArray(f.value)
        ? f.value.map(v => `'${v.replace(/'/g, "''")}'`)
        : f.value = `'${f.value.replace(/'/g, "''")}'`;
    return f;
};

export const encodeValue: Transformation = (f: FilterDescriptorEx) => {
    f.value = Array.isArray(f.value)
        ? f.value.map(v => `${encodeURIComponent(v)}`)
        : `${encodeURIComponent(f.value)}`;
    return f;
};

export const toLowerValue: Transformation = (f: FilterDescriptorEx, s: ODataExSettings) => {
    f.value = Array.isArray(f.value)
        ? f.value.map(v => wrapIf(() => (s.filter.ignoreValueCase || f.ignoreValueCase)) `tolower(${v})`)
        : wrapIf(() => (s.filter.ignoreValueCase || f.ignoreValueCase)) `tolower(${f.value})`;
    return f;
};

// and also FormatDate

[hidegh]

Proof of extensibility due refactor

Recent refactor changes on the compositeFilterSerializationFn includes the move the inner part to a separate, reusable serializeFilterFn function. The name although reminds the main entry function serializeFilter (which I dislike) - but currently still trying to keep kendo naming...

export const compositeFilterSerializationFn = (filter: CompositeFilterDescriptor, settings: ODataExSettings): string => {
    let filterExpression =
        filter
            .filters
            .map(f => serializeFilterFn(f, settings))
            .join(` ${filter.logic} `);

    const brackets = wrapIf(() => filter.filters.length > 1);
    return brackets`(${filterExpression})`;
};

export const serializeFilterFn = (filter: any, settings: ODataExSettings): string => {

    //
    // handle regular composite filter
    if (isCompositeFilterDescriptor(filter))
        return compositeFilterSerializationFn(filter, settings);

    //
    // handle special collection filter
    if (isCollectionFilterDescriptorEx(filter))
        return collectionFilterExSerializationFn(filter, settings);

    //
    // handle special operators
    if (isNegationOperatorFilterEx(filter))
        return negationOperatorExSerializationFn(filter, settings);

    //
    // specific filtering logic

    // handle date from-to to support 1 day selection (replace simple filter with a composite)
    // NOTE:
    // No need to handle inside singleFilterSerializationFn as we always use State and start with a CompositeFilterDescriptor...
    // ...otherwise we'd need to handle the change from FilterDescriptor to Composite FilterDescriptor inside serializeFilter
    if (isDate(filter)
        && (settings.filter.date24hMatch || (filter as any).date24hMatch)
        && ((filter.operator as any).toLowerCase() === "eq" || (filter.operator as any).toLowerCase() === "neq")
    ) {

        // replace single date-filter expression with a composite one that will consider a whole day (from-to) or outside it...
        let typedDate = filter.value as Date;
        let fromDate = new Date(typedDate.getFullYear(), typedDate.getMonth(), typedDate.getDate());
        let toDate = new Date(typedDate.getTime() + 86400000); // get next day: add 86400000ms = 1 day

        // if date operator is 'eq' we use: date >= from && < to
        // for the 'ne' we use: date < from && date >= to

        let operatorLeft = (filter.operator as any).toLowerCase() === "eq" ? "gte" : "lt";
        let operatorRight = (filter.operator as any).toLowerCase() === "eq" ? "lt" : "gte";

        let dayDateCompositeFilter = {
            logic: "and",
            filters: [
                { field: filter.field, operator: operatorLeft, value: fromDate } as FilterDescriptor,
                { field: filter.field, operator: operatorRight, value: toDate } as FilterDescriptor
            ] as FilterDescriptor[]
        } as CompositeFilterDescriptor;

        return compositeFilterSerializationFn(dayDateCompositeFilter, settings);
    }

    //
    // handle regular simple filter
    return singleFilterSerializationFn(filter, settings);

}

This change allowed me to create additional filter operators:

1. NegationOperatorDescriptorEx - which simply negates a boolean expression (e.g. not startswith)
2. CollectionFilterDescriptorEx - to support to search within collections (OData /any and /all)

import { FilterDescriptor, isCompositeFilterDescriptor, CompositeFilterDescriptor } from "@progress/kendo-data-query";
import { isPresent } from "@progress/kendo-data-query/dist/npm/utils";
import { ODataExSettings } from "./odata";
import { compositeFilterSerializationFn, singleFilterSerializationFn, serializeFilterFn } from "./odata-filtering";
/**
 * Set-up sample - NOTE: in some cases when we need to use CollectionFilterDescriptorEx inside a CompositeFilterDescriptor, we need the <any> as the filtering works by default just with FilterDescriptor and CompositeFilterDescriptor items!
 * 
 *  let collectionFilter = {
 *      field: "MissingRequirements",
 *      operator: "any",
 *      ref: "p0",
 *      filter: {
 *          logic: "or".
 *          filters: filters // NOTE: the inner filters must use p0/Name as field
 *      } as CompositeFilterDescriptor
 * } as CollectionFilterDescriptorEx;
 * 
 *  let compositeFilter = {
 *      logic: "or",
 *      filters: [ <any>(collectionFilter) ]
 *  }
 * 
 * Samples:
 * $filter=Items/any(d:d/Quantity gt 100)
 * $filter=Items/all(d:d/Quantity gt 100)
 * 
 * URL:
 * http://docs.oasis-open.org/odata/odata/v4.0/errata03/os/complete/part2-url-conventions/odata-v4.0-errata03-os-part2-url-conventions-complete.html#_Toc444868730
 */
export interface CollectionFilterDescriptorEx {
    field: string;
    operator: "any" | "all";

    /* specifics to the coll. filter */
    ref: string;
    filter: FilterDescriptor | CompositeFilterDescriptor;
}

export const isCollectionFilterDescriptorEx = (source: any /** any due to future extensions, otherwise: CollectionFilterDescriptorEx | CompositeFilterDescriptor | FilterDescriptor */): source is CollectionFilterDescriptorEx => {
    return isPresent((<CollectionFilterDescriptorEx>source).ref) && isPresent((<CollectionFilterDescriptorEx>source).filter);
};

export const collectionFilterExSerializationFn = (collectionFilter: CollectionFilterDescriptorEx, settings: ODataExSettings): string => {
    let normalizedCollcetionFieldName = (<string>collectionFilter.field).replace(/\./g, "/");
    let innerFilterExpression = serializeFilterFn(collectionFilter.filter, settings);
    return `${normalizedCollcetionFieldName}/${collectionFilter.operator}(${collectionFilter.ref}: ${innerFilterExpression})`;
};

import { FilterDescriptor, isCompositeFilterDescriptor, CompositeFilterDescriptor } from "@progress/kendo-data-query";
import { isPresent } from "@progress/kendo-data-query/dist/npm/utils";
import { ODataExSettings } from "./odata";
import { serializeFilterFn } from "./odata-filtering";

/**
 * Set-up - NOTE: we need the <any> as the filtering works by default just with FilterDescriptor and CompositeFilterDescriptor items!
 *  {
 *      logic: "and",
 *      filters: [ <any>({ filter: cf } as NegationOperatorDescriptorEx) ]
 *  } as CompositeFilterDescriptor
 * 
 * Odata samples:
 * $filter=not(Items/any(d:d/Quantity gt 100))
 * $filter=not endswith(Name, 'ilk')
 * 
 * URL:
 * https://www.odata.org/documentation/odata-version-3-0/url-conventions/
 * 
 * NOTE:
 * At this moment support for arithmetic ops is not considered: $filter=( 4 add 5 ) mod ( 4 sub 1 ) eq 0
 */
export interface NegationOperatorDescriptorEx {
    filter: any /** any due to future extensions, otherwise: FilterDescriptor | CompositeFilterDescriptor | CollectionFilterDescriptorEx; */
}

export const isNegationOperatorFilterEx = (source: any): source is NegationOperatorDescriptorEx => {
    return isPresent((<NegationOperatorDescriptorEx>source).filter);
};

export const negationOperatorExSerializationFn = (negationFilterOperator: NegationOperatorDescriptorEx, settings: ODataExSettings): string => {
    let filterExpression = serializeFilterFn(negationFilterOperator.filter, settings);
    return `not ${filterExpression}`;
};

[EricSch]

:) Wow, what a proposal. @hidegh And not a response until now?

[hidegh]

@EricSch actually they got everything, yet no response. if they would ask, I probably would be allowed to give over the working code (so-so it's almost in fully described here). internally we already use the extended toOdataStringEx, all those extras helped us to achieve cleaner custom filter design (NoopFilterDescriptorEx), easier search inside inner collections, etc. etc.

So here is an another small code that was necessary for creating complex custom filters. We had to wrap the filter into a NoopFilterDescriptorEx so the UI could do a match based on field name - and thus we also modified the originaly NegationOperatorDescriptorEx that I posted in the past. New code below:

/**
 * The $noop filter is actually an unary filter.
 * It's just a container around a real filter.
 *
 * It is needed on the UI, to create custom reusable FilterComponents for the grid.
 * Especially needed with collection filtering (all/any) over complex data.
 *
 * Rules for the filter component:
 * - The grid uses the field parameter to profide the filter component with the proper filter expression (related to the given column).
 * - The UI needs a value to be set, otherwise the filter button won't be active (so we need a value and we need to set also a 'fake' value).
 * - The grid will keep also any extra parameters set on the filter expression.
 */
export class NoopFilterDescriptorEx implements UnaryFilterDescriptorEx {

  // UnaryFilterDescriptorEx with defaulting to $noop
  operator: any = '$noop';
  filter: FilterDescriptor | CompositeFilterDescriptor | any;

  // container extension - so the expression works well with the filter
  field: string;
  value: any = 'fake';

  public constructor(field: string, filter: any, extensionObject?: object) {
    if (extensionObject)
      Object.assign(this, extensionObject);

    this.field = field;
    this.filter = filter;
  }
}

/**
 * Currently supported ops:
 *  not   - negation operator - odata sample:  $filter=not(Items/any(d:d/Quantity gt 100)); $filter=not endswith(Name, 'ilk')
 *
 * URL:
 *  https://www.odata.org/documentation/odata-version-3-0/url-conventions/
 *
 * NOTE:
 *  At this moment support for arithmetic ops is not considered: $filter=( 4 add 5 ) mod ( 4 sub 1 ) eq 0
 */
export interface UnaryFilterDescriptorEx {
  operator: '$noop' | 'not';
  filter: any; /** any due to future extensions, otherwise: FilterDescriptor | CompositeFilterDescriptor | CollectionFilterDescriptorEx; */
}

export const isUnaryFilterDescriptorEx = (source: any): source is UnaryFilterDescriptorEx => {
  const unaryFilter = source as UnaryFilterDescriptorEx;
  return isPresent(unaryFilter.filter) && ['$noop', 'not'].some(op => op === unaryFilter.operator);
};

export const unaryFilterExSerializationFn = (unaryFilter: UnaryFilterDescriptorEx, settings: ODataExSettings): string => {

  switch (unaryFilter.operator) {
    case '$noop':
      // simply just serialize the inside
      return serializeFilterFn(unaryFilter.filter, settings);
    case 'not':
      // TODO: check if the serializeFilterFn wraps complex expression inside brackets
      const notFilterExpression = serializeFilterFn(unaryFilter.filter, settings);
      return `not ${notFilterExpression}`;
    default:
      throw new Error(`Unsupported operator: '${unaryFilter.operator}' in the UnaryFilterDescriptorEx: '${JSON.stringify(unaryFilter)}'`);
  }

};

[dtopalov]

Thank you for proposals and code samples. We reviewed all suggestions again, and decided to address them as follows:

• Track adding support for the "in" in operator in toODataString() in our Feedback portal:

  https://feedback.telerik.com/kendo-angular-ui/1522494-implement-in-operator-on-toodatastring

• "Btw. would also check the UTC date string, it should not be generated via JSON.stringify as that might be overriden (we did that to keep original time-zone values, but of course that valid change within ODATA query expression was not correct) - this dependency should be removed."

  We will replace JSON.stringify() with Date.toISOString() in our date-formatting logic.