From 18bdf2f308508ab04ce487233232e4716e3dbe0b Mon Sep 17 00:00:00 2001 From: Jan Melcher Date: Fri, 18 Feb 2022 11:22:54 +0100 Subject: [PATCH 1/7] Drop support for node 12 --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 44de9baf9..007366a9d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -6,7 +6,7 @@ jobs: strategy: matrix: arango-image: ['arangodb:3.6', 'arangodb:3.7', 'arangodb:3.8'] - node-version: [12.x, 14.x, 16.x] + node-version: [14.x, 16.x] steps: - uses: actions/checkout@v2 - name: Use Node.js ${{ matrix.node-version }} From 80b2e5a2d63aae439e1e65fb3189f990932b85f8 Mon Sep 17 00:00:00 2001 From: Jan Melcher Date: Fri, 18 Feb 2022 11:23:10 +0100 Subject: [PATCH 2/7] Fix case-insensitive flexsearch filters in InMemoryAdapter --- src/database/inmemory/js-generator.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/database/inmemory/js-generator.ts b/src/database/inmemory/js-generator.ts index c298f5dec..e6128e192 100644 --- a/src/database/inmemory/js-generator.ts +++ b/src/database/inmemory/js-generator.ts @@ -964,11 +964,11 @@ register(OperatorWithAnalyzerQueryNode, (node, context) => { let rhs = processNode(node.rhs, context); if (isCaseInsensitive) { - lhs = js`${lhs}.toLowerCase()`; + lhs = js`(${lhs})?.toLowerCase()`; const rhsVar = js.variable('rhs'); rhs = jsExt.evaluatingLambda( rhsVar, - js`(Array.isArray(${rhsVar}) ? ${rhsVar}.map(value => value.toLowerCase()) : ${rhsVar}.toLowerCase())`, + js`(Array.isArray(${rhsVar}) ? ${rhsVar}.map(value => value?.toLowerCase()) : (${rhsVar})?.toLowerCase())`, rhs ); } From 5134708f9ee9dc8e411ad8fe272130697b388f8a Mon Sep 17 00:00:00 2001 From: Jan Melcher Date: Fri, 18 Feb 2022 10:14:48 +0100 Subject: [PATCH 3/7] Default isFlexSearchIndeCaseSensitive to false Also, add a model option to change this. --- core-exports.ts | 2 +- spec/regression/regression-suite.ts | 2 +- src/config/interfaces.ts | 21 +++++++++++-------- src/model/config/model.ts | 4 ++-- src/model/create-model.ts | 31 +++++++++++++++++++---------- src/model/implementation/field.ts | 2 +- src/model/implementation/model.ts | 15 +++++++++----- src/project/project.ts | 1 - src/schema/schema-builder.ts | 2 +- 9 files changed, 49 insertions(+), 31 deletions(-) diff --git a/core-exports.ts b/core-exports.ts index 3b226d80e..112176578 100644 --- a/core-exports.ts +++ b/core-exports.ts @@ -1,4 +1,4 @@ -export { RequestProfile, ProjectOptions, RequestContext, ModelValidationOptions } from './src/config/interfaces'; +export { RequestProfile, ProjectOptions, RequestContext, ModelOptions } from './src/config/interfaces'; export { FieldResolverParameters } from './src/graphql/operation-based-resolvers'; export { Project, ProjectConfig } from './src/project/project'; export { InvalidProjectError } from './src/project/invalid-project-error'; diff --git a/spec/regression/regression-suite.ts b/spec/regression/regression-suite.ts index 42e635b8b..c92e5f774 100644 --- a/spec/regression/regression-suite.ts +++ b/spec/regression/regression-suite.ts @@ -71,7 +71,7 @@ export class RegressionSuite { authRoles: context.authRoles, flexSearchMaxFilterableAndSortableAmount: context.flexSearchMaxFilterableAndSortableAmount }), - modelValidationOptions: { + modelOptions: { forbiddenRootEntityNames: [] }, ...options, diff --git a/src/config/interfaces.ts b/src/config/interfaces.ts index 17cc435f5..7ac4e83bf 100644 --- a/src/config/interfaces.ts +++ b/src/config/interfaces.ts @@ -25,13 +25,6 @@ export interface SchemaOptions { readonly maxOrderByRootEntityDepth?: number; } -export interface ModelValidationOptions { - /** - * A list of root entity names that are not allowed. - */ - readonly forbiddenRootEntityNames?: ReadonlyArray; -} - export interface ModelOptions { /** * Determines whether a slash in a source name indicates the target namespace for that source @@ -39,6 +32,18 @@ export interface ModelOptions { * Defaults to true. Explicitly specify false to disable this. */ readonly useSourceDirectoriesAsNamespaces?: boolean; + + /** + * Specifies the default for case-sensitiveness of flexSearch fields (can be overridden with the decorator) + * + * Default is false + */ + readonly isFlexSearchIndexCaseSensitiveByDefault?: boolean; + + /** + * A list of root entity names that are not allowed. + */ + readonly forbiddenRootEntityNames?: ReadonlyArray; } export interface ProjectOptions { @@ -47,7 +52,7 @@ export interface ProjectOptions { readonly getExecutionOptions?: (args: ExecutionOptionsCallbackArgs) => ExecutionOptions; readonly schemaOptions?: SchemaOptions; - readonly modelValidationOptions?: ModelValidationOptions; + readonly modelOptions?: ModelOptions; /** diff --git a/src/model/config/model.ts b/src/model/config/model.ts index e03477a3b..6e52172f9 100644 --- a/src/model/config/model.ts +++ b/src/model/config/model.ts @@ -1,4 +1,4 @@ -import { ModelValidationOptions } from '../../config/interfaces'; +import { ModelOptions } from '../../config/interfaces'; import { ValidationMessage } from '../validation'; import { BillingConfig } from './billing'; import { LocalizationConfig } from './i18n'; @@ -11,6 +11,6 @@ export interface ModelConfig { readonly validationMessages?: ReadonlyArray; readonly i18n?: ReadonlyArray; readonly billing?: BillingConfig; - readonly modelValidationOptions?: ModelValidationOptions; readonly timeToLiveConfigs?: ReadonlyArray; + readonly options?: ModelOptions; } diff --git a/src/model/create-model.ts b/src/model/create-model.ts index 49178bd92..b5dc2e96b 100644 --- a/src/model/create-model.ts +++ b/src/model/create-model.ts @@ -16,7 +16,7 @@ import { TypeDefinitionNode, valueFromAST } from 'graphql'; -import { ModelValidationOptions } from '../config/interfaces'; +import { ModelOptions } from '../config/interfaces'; import { ParsedGraphQLProjectSource, ParsedObjectProjectSource, @@ -115,16 +115,16 @@ import { parseI18nConfigs } from './parse-i18n'; import { parseTTLConfigs } from './parse-ttl'; import { ValidationContext, ValidationMessage } from './validation'; -export function createModel(parsedProject: ParsedProject, modelValidationOptions?: ModelValidationOptions): Model { +export function createModel(parsedProject: ParsedProject, options?: ModelOptions): Model { const validationContext = new ValidationContext(); return new Model({ - types: createTypeInputs(parsedProject, validationContext), + types: createTypeInputs(parsedProject, validationContext, options ?? {}), permissionProfiles: extractPermissionProfiles(parsedProject), i18n: extractI18n(parsedProject), validationMessages: validationContext.validationMessages, billing: extractBilling(parsedProject), - modelValidationOptions, - timeToLiveConfigs: extractTimeToLive(parsedProject) + timeToLiveConfigs: extractTimeToLive(parsedProject), + options }); } @@ -144,7 +144,11 @@ const VALIDATION_ERROR_MISSING_OBJECT_TYPE_DIRECTIVE = `Add one of @${ROOT_ENTIT const VALIDATION_ERROR_INVALID_DEFINITION_KIND = 'This kind of definition is not allowed. Only object and enum type definitions are allowed.'; -function createTypeInputs(parsedProject: ParsedProject, context: ValidationContext): ReadonlyArray { +function createTypeInputs( + parsedProject: ParsedProject, + context: ValidationContext, + options: ModelOptions +): ReadonlyArray { const graphQLSchemaParts = parsedProject.sources.filter( parsedSource => parsedSource.kind === ParsedProjectSourceBaseKind.GRAPHQL ) as ReadonlyArray; @@ -173,7 +177,7 @@ function createTypeInputs(parsedProject: ParsedProject, context: ValidationConte }; return enumTypeInput; case OBJECT_TYPE_DEFINITION: - return createObjectTypeInput(definition, schemaPart, context); + return createObjectTypeInput(definition, schemaPart, context, options); default: return undefined; } @@ -196,7 +200,8 @@ function createEnumValues(valueNodes: ReadonlyArray): R function createObjectTypeInput( definition: ObjectTypeDefinitionNode, schemaPart: ParsedGraphQLProjectSource, - context: ValidationContext + context: ValidationContext, + options: ModelOptions ): ObjectTypeConfig { const entityType = getKindOfObjectTypeNode(definition, context); @@ -204,7 +209,7 @@ function createObjectTypeInput( name: definition.name.value, description: definition.description ? definition.description.value : undefined, astNode: definition, - fields: (definition.fields || []).map(field => createFieldInput(field, context)), + fields: (definition.fields || []).map(field => createFieldInput(field, context, options)), namespacePath: getNamespacePath(definition, schemaPart.namespacePath), flexSearchLanguage: getDefaultLanguage(definition, context) }; @@ -457,7 +462,11 @@ function getLanguage(fieldNode: FieldDefinitionNode, context: ValidationContext) } } -function createFieldInput(fieldNode: FieldDefinitionNode, context: ValidationContext): FieldConfig { +function createFieldInput( + fieldNode: FieldDefinitionNode, + context: ValidationContext, + options: ModelOptions +): FieldConfig { const inverseOfASTNode = getInverseOfASTNode(fieldNode, context); const relationDeleteActionASTNode = getRelationDeleteActionASTNode(fieldNode, context); const referenceDirectiveASTNode = findDirectiveWithName(fieldNode, REFERENCE_DIRECTIVE); @@ -500,7 +509,7 @@ function createFieldInput(fieldNode: FieldDefinitionNode, context: ValidationCon isFlexSearchIndexCaseSensitive: flexSearchIndexCaseSensitiveNode?.value.kind === 'BooleanValue' ? flexSearchIndexCaseSensitiveNode.value.value - : undefined, + : options.isFlexSearchIndexCaseSensitiveByDefault, isFlexSearchIndexedASTNode: findDirectiveWithName(fieldNode, FLEX_SEARCH_INDEXED_DIRECTIVE), isFlexSearchFulltextIndexed: hasDirectiveWithName(fieldNode, FLEX_SEARCH_FULLTEXT_INDEXED_DIRECTIVE), isFlexSearchFulltextIndexedASTNode: findDirectiveWithName(fieldNode, FLEX_SEARCH_FULLTEXT_INDEXED_DIRECTIVE), diff --git a/src/model/implementation/field.ts b/src/model/implementation/field.ts index 5e245049a..837bc0f8b 100644 --- a/src/model/implementation/field.ts +++ b/src/model/implementation/field.ts @@ -1522,7 +1522,7 @@ export class Field implements ModelComponent { } get isFlexSearchIndexCaseSensitive(): boolean { - return this.input.isFlexSearchIndexCaseSensitive ?? true; + return this.input.isFlexSearchIndexCaseSensitive ?? false; } get flexSearchAnalyzer(): string | undefined { diff --git a/src/model/implementation/model.ts b/src/model/implementation/model.ts index 5bb46fdc7..2a27d48d6 100644 --- a/src/model/implementation/model.ts +++ b/src/model/implementation/model.ts @@ -1,6 +1,6 @@ import { groupBy, uniqBy } from 'lodash'; import memorize from 'memorize-decorator'; -import { ModelValidationOptions } from '../../config/interfaces'; +import { ModelOptions } from '../../config/interfaces'; import { flatMap, objectEntries, objectValues } from '../../utils/utils'; import { ModelConfig, TypeKind } from '../config'; import { NamespacedPermissionProfileConfigMap } from '../index'; @@ -31,7 +31,11 @@ export class Model implements ModelComponent { readonly i18n: ModelI18n; readonly permissionProfiles: ReadonlyArray; readonly billingEntityTypes: ReadonlyArray; - readonly modelValidationOptions?: ModelValidationOptions; + /** + * @deprecated use options + */ + readonly modelValidationOptions?: ModelOptions; + readonly options?: ModelOptions; readonly timeToLiveTypes: ReadonlyArray; constructor(private input: ModelConfig) { @@ -50,7 +54,8 @@ export class Model implements ModelComponent { this.billingEntityTypes = input.billing ? input.billing.billingEntities.map(value => new BillingEntityType(value, this)) : []; - this.modelValidationOptions = input.modelValidationOptions; + this.options = input.options; + this.modelValidationOptions = input.options; this.timeToLiveTypes = input.timeToLiveConfigs ? input.timeToLiveConfigs.map(ttlConfig => new TimeToLiveType(ttlConfig, this)) : []; @@ -243,10 +248,10 @@ export class Model implements ModelComponent { } get forbiddenRootEntityNames(): ReadonlyArray { - if (!this.modelValidationOptions || !this.modelValidationOptions.forbiddenRootEntityNames) { + if (!this.options || !this.options.forbiddenRootEntityNames) { return ['BillingEntity']; } - return this.modelValidationOptions!.forbiddenRootEntityNames; + return this.options!.forbiddenRootEntityNames; } } diff --git a/src/project/project.ts b/src/project/project.ts index 4c25a6c41..da998d9b3 100644 --- a/src/project/project.ts +++ b/src/project/project.ts @@ -118,7 +118,6 @@ export class Project { getOperationIdentifier: config.getOperationIdentifier, processError: config.processError, schemaOptions: config.schemaOptions, - modelValidationOptions: config.modelValidationOptions, modelOptions: config.modelOptions }; } diff --git a/src/schema/schema-builder.ts b/src/schema/schema-builder.ts index e3bea1af8..4fefa8993 100644 --- a/src/schema/schema-builder.ts +++ b/src/schema/schema-builder.ts @@ -81,7 +81,7 @@ export function validateAndPrepareSchema(project: Project): { validationResult: const preparedProject = executePreMergeTransformationPipeline({ sources: validParsedSources }); - const model = createModel(preparedProject, project.options.modelValidationOptions); + const model = createModel(preparedProject, project.options.modelOptions); const mergedSchema: DocumentNode = mergeSchemaDefinition(preparedProject); From f28f5875bf63c98a260456e42d5834223172e3c1 Mon Sep 17 00:00:00 2001 From: Jan Melcher Date: Fri, 18 Feb 2022 11:23:36 +0100 Subject: [PATCH 4/7] Make field Date: Fri, 18 Feb 2022 15:54:41 +0100 Subject: [PATCH 5/7] Do not emit flex search order if it matches primary sort We thought you should remove the sorting clause to get better performance, but actually sorting is no longer guaranteed if you omit it, and the docs say that performance is improved when sorting matches the primary sort. --- .../order-by-and-pagination-augmentation.ts | 34 +++++++------------ 1 file changed, 12 insertions(+), 22 deletions(-) diff --git a/src/schema-generation/order-by-and-pagination-augmentation.ts b/src/schema-generation/order-by-and-pagination-augmentation.ts index e07d3b837..df79a740f 100644 --- a/src/schema-generation/order-by-and-pagination-augmentation.ts +++ b/src/schema-generation/order-by-and-pagination-augmentation.ts @@ -119,33 +119,24 @@ export class OrderByAndPaginationAugmentation { // flexsearch? // we only offer cursor-based pagination if we can cover all required filters with flex search filters // this means we just replace the flexsearch listNode with a flexsearch listNode that does the cursor-filtering - let leaveUnordered = false; if (listNode instanceof FlexSearchQueryNode) { if (!orderByType) { throw new Error(`OrderBy type missing for flex search`); } - // whenever possible, use primary sort - if (orderArgMatchesPrimarySort(args[ORDER_BY_ARG], listNode.rootEntityType.flexSearchPrimarySort)) { - // this would be cleaner if the primary sort was actually parsed into a ModelComponent (see e.g. the Index and IndexField classes) - orderByValues = listNode.rootEntityType.flexSearchPrimarySort.map(clause => - orderByType.getValueOrThrow( - clause.field.path.replace('.', '_') + - (clause.direction === OrderDirection.ASCENDING ? '_ASC' : '_DESC') - ) - ); - leaveUnordered = true; - } else { - // this will generate a SORT clause that's not covered by the flexsearch index, - // but the TooManyObject check of flex-search-generator already handles this case to throw - // a TooManyObjects error if needed. - orderByValues = getOrderByValues(args, orderByType, { isAbsoluteOrderRequired }); - } + // we used to remove the sort clause if it matched the primary sort, but it turned out this is not + // ok. If the sorting matches the primary sort, sorting is efficient, but you still need to specify + // it, otherwise, sometimes the order can be wrong (after inserting or updating documents). + + // this may generate a SORT clause that is not covered by the flexsearch index, + // but the TooManyObject check of flex-search-generator already handles this case to throw + // a TooManyObjects error if needed. + orderByValues = getOrderByValues(args, orderByType, { isAbsoluteOrderRequired }); // for now, cursor-based pagination is only allowed when we can use flexsearch filters for the // paginationFilter. This simplifies the implementation, but maybe we should support it in the // future for consistency. Then however we need to adjust the too-many-objects check // do this check also if cursor is just requested so we get this error on the first page - if (isCursorRequested || !!afterArg) { + if (isCursorRequested || afterArg) { const violatingClauses = orderByValues.filter(val => val.path.some(field => !field.isFlexSearchIndexed) ); @@ -189,10 +180,9 @@ export class OrderByAndPaginationAugmentation { } // sorting always happens on the TransformListQueryNode and not in the FlexSearchQueryNode - const orderBy = - !orderByType || leaveUnordered - ? OrderSpecification.UNORDERED - : new OrderSpecification(orderByValues.map(value => value.getClause(objectNode))); + const orderBy = !orderByType + ? OrderSpecification.UNORDERED + : new OrderSpecification(orderByValues.map(value => value.getClause(objectNode))); if (orderBy.isUnordered() && maxCount == undefined && paginationFilter === ConstBoolQueryNode.TRUE) { return originalListNode; From f631f39ce88a1599abb80deb8436ea9acd43ff70 Mon Sep 17 00:00:00 2001 From: Jan Melcher Date: Fri, 18 Feb 2022 11:01:32 +0100 Subject: [PATCH 6/7] Use default primary sort of createdAt_DESC, id_DESC --- src/model/config/indices.ts | 3 ++- src/model/create-model.ts | 3 ++- src/model/implementation/root-entity-type.ts | 21 ++++++++++++++------ 3 files changed, 19 insertions(+), 8 deletions(-) diff --git a/src/model/config/indices.ts b/src/model/config/indices.ts index 629febda6..4180b540d 100644 --- a/src/model/config/indices.ts +++ b/src/model/config/indices.ts @@ -1,4 +1,5 @@ import { DirectiveNode, ObjectValueNode, StringValueNode } from 'graphql'; +import { OrderDirection } from '../implementation/order'; export interface IndexDefinitionConfig { readonly name?: string; @@ -23,7 +24,7 @@ export interface IndexDefinitionConfig { export interface FlexSearchPrimarySortClauseConfig { readonly field: string; - readonly asc: boolean; + readonly direction: OrderDirection; } export interface FlexSearchIndexConfig { diff --git a/src/model/create-model.ts b/src/model/create-model.ts index b5dc2e96b..5521718ae 100644 --- a/src/model/create-model.ts +++ b/src/model/create-model.ts @@ -110,6 +110,7 @@ import { } from './config'; import { BillingConfig } from './config/billing'; import { Model } from './implementation'; +import { OrderDirection } from './implementation/order'; import { parseBillingConfigs } from './parse-billing'; import { parseI18nConfigs } from './parse-i18n'; import { parseTTLConfigs } from './parse-ttl'; @@ -342,7 +343,7 @@ function getFlexSearchOrder(rootEntityDirective?: DirectiveNode): ReadonlyArray< .map((value: any) => { return { field: value.field, - asc: value.direction === 'ASC' ? true : false + direction: value.direction === 'DESC' ? OrderDirection.DESCENDING : OrderDirection.ASCENDING }; }); } diff --git a/src/model/implementation/root-entity-type.ts b/src/model/implementation/root-entity-type.ts index c5f923777..9d9fda5b1 100644 --- a/src/model/implementation/root-entity-type.ts +++ b/src/model/implementation/root-entity-type.ts @@ -2,6 +2,7 @@ import { GraphQLID, GraphQLString } from 'graphql'; import memorize from 'memorize-decorator'; import { ACCESS_GROUP_FIELD, + ENTITY_CREATED_AT, FLEX_SEARCH_FULLTEXT_INDEXED_DIRECTIVE, FLEX_SEARCH_INDEXED_DIRECTIVE, FLEX_SEARCH_ORDER_ARGUMENT, @@ -349,21 +350,29 @@ export class RootEntityType extends ObjectTypeBase { // primary sort is only used for sorting, so make sure it's unique // - this makes querying more consistent // - this enables us to use primary sort for cursor-based pagination (which requires an absolute sort order) + // the default primary sort should be createdAt_DESC, because this is useful most of the time. to avoid + // surprises when you do specify a primary sort, always add this default at the end (as long as it's not already + // included in the index) + if (!clauses.some(clause => clause.field === ENTITY_CREATED_AT)) { + clauses = [ + ...clauses, + { + field: ENTITY_CREATED_AT, + direction: OrderDirection.DESCENDING + } + ]; + } if (!clauses.some(clause => clause.field === this.discriminatorField.name)) { clauses = [ ...clauses, { field: this.discriminatorField.name, - asc: true + direction: OrderDirection.DESCENDING } ]; } return clauses.map( - c => - new FlexSearchPrimarySortClause( - new FieldPath({ path: c.field, baseType: this }), - c.asc ? OrderDirection.ASCENDING : OrderDirection.DESCENDING - ) + c => new FlexSearchPrimarySortClause(new FieldPath({ path: c.field, baseType: this }), c.direction) ); } From 4087a8a06719aec5b038cff535b2faec91a609f1 Mon Sep 17 00:00:00 2001 From: Jan Melcher Date: Thu, 3 Mar 2022 14:37:11 +0100 Subject: [PATCH 7/7] wip forgot what this is --- spec/regression/papers/model/paper.graphqls | 5 ++++- src/schema-generation/utils/flex-search-utils.ts | 1 + 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/spec/regression/papers/model/paper.graphqls b/spec/regression/papers/model/paper.graphqls index 1c1474c10..33ab63f2e 100644 --- a/spec/regression/papers/model/paper.graphqls +++ b/spec/regression/papers/model/paper.graphqls @@ -6,8 +6,11 @@ enum Category { Programming } +# need to specify flexSearchOrder with an id field so createdAt does not included (which we cannot test because it changes) "A scientific paper" -type Paper @rootEntity(flexSearch: true, flexSearchLanguage: EN) @roles(readWrite: ["admin"]) { +type Paper + @rootEntity(flexSearch: true, flexSearchLanguage: EN, flexSearchOrder: [{ field: "id", direction: ASC }]) + @roles(readWrite: ["admin"]) { key: String @key @flexSearch # need a key field for the reference title: String @index @flexSearch # for pagination performance test "The date this paper has been published in a scientific journal or conference" diff --git a/src/schema-generation/utils/flex-search-utils.ts b/src/schema-generation/utils/flex-search-utils.ts index 8e4c70768..7c30845d1 100644 --- a/src/schema-generation/utils/flex-search-utils.ts +++ b/src/schema-generation/utils/flex-search-utils.ts @@ -5,6 +5,7 @@ export function orderArgMatchesPrimarySort( clauses: ReadonlyArray | undefined, primarySort: ReadonlyArray ): boolean { + // TODO what about sort clauses that are added automatically because the user used cursor-based pagination? if (!clauses || !clauses.length) { return true; }