From 0ce47ba5d9daa754cde3177255a913d57f1ffc04 Mon Sep 17 00:00:00 2001 From: Daniel Jeffery Date: Mon, 29 Jul 2024 10:47:51 -0700 Subject: [PATCH] chore: migrating server tests --- .../validation/ValidationErrorsBuilder.java | 6 +- .../transformer/modules/modules-to-model.ts | 8 +- pkg/js/util/exceptions.ts | 6 + pkg/js/validator/validate-dsl.ts | 90 ++- tests/data/dsl-semantic-validation-cases.yaml | 533 ++++++++++++++++++ 5 files changed, 589 insertions(+), 54 deletions(-) diff --git a/pkg/java/src/main/java/dev/openfga/language/validation/ValidationErrorsBuilder.java b/pkg/java/src/main/java/dev/openfga/language/validation/ValidationErrorsBuilder.java index fc4815fe..162dd407 100644 --- a/pkg/java/src/main/java/dev/openfga/language/validation/ValidationErrorsBuilder.java +++ b/pkg/java/src/main/java/dev/openfga/language/validation/ValidationErrorsBuilder.java @@ -108,7 +108,11 @@ public void raiseAssignableRelationMustHaveTypes(int lineIndex, String symbol) { public void raiseInvalidType(int lineIndex, String symbol, String typeName) { var message = "`" + typeName + "` is not a valid type."; - var errorProperties = buildErrorProperties(message, lineIndex, symbol); + var errorProperties = buildErrorProperties(message, lineIndex, symbol, (wordIdx, rawLine, type) -> { + // Split line at definition as InvalidType should mark the value, not the key + var splitLine = rawLine.split(":"); + return splitLine[0].length() + splitLine[1].indexOf(typeName) + 1; + }); var metadata = new ValidationMetadata(symbol, ValidationError.InvalidType); errors.add(new ModelValidationSingleError(errorProperties, metadata)); } diff --git a/pkg/js/transformer/modules/modules-to-model.ts b/pkg/js/transformer/modules/modules-to-model.ts index f1c48675..8923aa3f 100644 --- a/pkg/js/transformer/modules/modules-to-model.ts +++ b/pkg/js/transformer/modules/modules-to-model.ts @@ -293,14 +293,20 @@ function resolveWordIndex(e: ModelValidationSingleError, line: string): number { return -1; } + const re = new RegExp("\\b" + metadata.symbol + "\\b"); + let wordIdx; switch (metadata.errorType) { + case ValidationError.InvalidType: + // Split line at definition as InvalidType should mark the value, not the key + const splitLine = line.split(":"); + wordIdx = splitLine[0].length + splitLine[1].search(re) + 1; + break; case ValidationError.TuplesetNotDirect: const clauseStartsAt = line.indexOf("from") + "from".length; wordIdx = clauseStartsAt + line.slice(clauseStartsAt).indexOf(metadata.symbol); break; default: - const re = new RegExp("\\b" + metadata.symbol + "\\b"); wordIdx = line?.search(re); } diff --git a/pkg/js/util/exceptions.ts b/pkg/js/util/exceptions.ts index eccae9dc..ec8b9492 100644 --- a/pkg/js/util/exceptions.ts +++ b/pkg/js/util/exceptions.ts @@ -212,6 +212,12 @@ const createInvalidTypeError = (props: BaseProps, typeName: string) => { lines, lineIndex, metadata: { symbol, errorType: ValidationError.InvalidType, typeName: typeName, file, module, relation }, + customResolver: (wordIndex: number, rawLine: string, symbol: string): number => { + // Split line at definition as InvalidType should mark the value, not the key + const re = new RegExp("\\b" + symbol + "\\b"); + const splitLine = rawLine.split(":"); + return splitLine[0].length + splitLine[1].search(re) + 1; + }, }), ); }; diff --git a/pkg/js/validator/validate-dsl.ts b/pkg/js/validator/validate-dsl.ts index 19d9da04..0fe37a5d 100644 --- a/pkg/js/validator/validate-dsl.ts +++ b/pkg/js/validator/validate-dsl.ts @@ -146,13 +146,13 @@ function hasEntryPointOrLoop( relationName: string | undefined, rewrite: Userset, visitedRecords: Record>, -): [boolean, boolean] { +): { hasEntry: boolean; loop: boolean } { // Deep copy const visited = deepCopy(visitedRecords); if (!relationName) { // nothing to do if relation is undefined - return [false, false]; + return { hasEntry: false, loop: false }; } if (!visited[typeName]) { @@ -162,13 +162,13 @@ function hasEntryPointOrLoop( const currentRelation = typeMap[typeName].relations; if (!currentRelation || !currentRelation[relationName]) { - return [false, false]; + return { hasEntry: false, loop: false }; } const relationMetadata = typeMap[typeName].metadata?.relations; if (!typeMap[typeName].relations || !typeMap[typeName].relations![relationName]) { - return [false, false]; + return { hasEntry: false, loop: false }; } if (rewrite.this) { @@ -177,58 +177,57 @@ function hasEntryPointOrLoop( )) { const { decodedType, decodedRelation, isWildcard } = destructTupleToUserset(assignableType); if (!decodedRelation || isWildcard) { - return [true, false]; + return { hasEntry: true, loop: false }; } const assignableRelation = typeMap[decodedType].relations![decodedRelation]; if (!assignableRelation) { - return [false, false]; + return { hasEntry: false, loop: false }; } if (visited[decodedType]?.[decodedRelation]) { continue; } - const [hasEntry] = hasEntryPointOrLoop(typeMap, decodedType, decodedRelation, assignableRelation, visited); + const { hasEntry } = hasEntryPointOrLoop(typeMap, decodedType, decodedRelation, assignableRelation, visited); if (hasEntry) { - return [true, false]; + return { hasEntry: true, loop: false }; } } - return [false, false]; + return { hasEntry: false, loop: false }; } else if (rewrite.computedUserset) { const computedRelationName = rewrite.computedUserset.relation; if (!computedRelationName) { - return [false, false]; + return { hasEntry: false, loop: false }; } if (!typeMap[typeName].relations![computedRelationName]) { - return [false, false]; + return { hasEntry: false, loop: false }; } const computedRelation = typeMap[typeName].relations![computedRelationName]; if (!computedRelation) { - return [false, false]; + return { hasEntry: false, loop: false }; } // Loop detected if (visited[typeName][computedRelationName]) { - return [false, true]; + return { hasEntry: false, loop: true }; } - const [hasEntry, loop] = hasEntryPointOrLoop(typeMap, typeName, computedRelationName, computedRelation, visited); - return [hasEntry, loop]; + return hasEntryPointOrLoop(typeMap, typeName, computedRelationName, computedRelation, visited); } else if (rewrite.tupleToUserset) { const tuplesetRelationName = rewrite.tupleToUserset.tupleset.relation; const computedRelationName = rewrite.tupleToUserset.computedUserset.relation; if (!tuplesetRelationName || !computedRelationName) { - return [false, false]; + return { hasEntry: false, loop: false }; } const tuplesetRelation = typeMap[typeName].relations![tuplesetRelationName]; if (!tuplesetRelation) { - return [false, false]; + return { hasEntry: false, loop: false }; } for (const assignableType of getTypeRestrictions( @@ -240,7 +239,7 @@ function hasEntryPointOrLoop( continue; } - const [hasEntry] = hasEntryPointOrLoop( + const { hasEntry } = hasEntryPointOrLoop( typeMap, assignableType, computedRelationName, @@ -248,60 +247,47 @@ function hasEntryPointOrLoop( visited, ); if (hasEntry) { - return [true, false]; + return { hasEntry: true, loop: false }; } } } - return [false, false]; + return { hasEntry: false, loop: false }; } else if (rewrite.union) { - let loop = false; + let hasLoop = false; for (const child of rewrite.union.child) { - const [entryPoint, childLoop] = hasEntryPointOrLoop(typeMap, typeName, relationName, child, deepCopy(visited)); - if (entryPoint) { - return [true, false]; + const { hasEntry, loop } = hasEntryPointOrLoop(typeMap, typeName, relationName, child, deepCopy(visited)); + if (hasEntry) { + return { hasEntry: true, loop: false }; } - loop = loop || childLoop; + hasLoop = hasLoop || loop; } - return [false, loop]; + return { hasEntry: false, loop: hasLoop }; } else if (rewrite.intersection) { for (const child of rewrite.intersection.child) { - const [hasEntry, childLoop] = hasEntryPointOrLoop(typeMap, typeName, relationName, child, deepCopy(visited)); + const { hasEntry, loop } = hasEntryPointOrLoop(typeMap, typeName, relationName, child, deepCopy(visited)); if (!hasEntry) { - return [false, childLoop]; + return { hasEntry: false, loop }; } } - - return [true, false]; + return { hasEntry: true, loop: false }; } else if (rewrite.difference) { const visited = deepCopy(visitedRecords); - const [hasEntryBase, loopBase] = hasEntryPointOrLoop( - typeMap, - typeName, - relationName, - rewrite.difference.base, - visited, - ); - if (!hasEntryBase) { - return [false, loopBase]; + const baseResult = hasEntryPointOrLoop(typeMap, typeName, relationName, rewrite.difference.base, visited); + if (!baseResult.hasEntry) { + return { hasEntry: false, loop: baseResult.loop }; } - const [hasEntrySubtract, loopSubtract] = hasEntryPointOrLoop( - typeMap, - typeName, - relationName, - rewrite.difference.subtract, - visited, - ); - if (!hasEntrySubtract) { - return [false, loopSubtract]; + const subtractResult = hasEntryPointOrLoop(typeMap, typeName, relationName, rewrite.difference.subtract, visited); + if (!subtractResult.hasEntry) { + return { hasEntry: false, loop: subtractResult.loop }; } - return [true, false]; + return { hasEntry: true, loop: false }; } - return [false, false]; + return { hasEntry: false, loop: false }; } const geConditionLineNumber = (conditionName: string, lines?: string[], skipIndex?: number) => { @@ -465,7 +451,7 @@ function childDefDefined( for (const item of fromPossibleTypes) { const { decodedType, decodedRelation, isWildcard, decodedConditionName } = destructTupleToUserset(item); if (!typeMap[decodedType]) { - // type is not defined + // Split line at definition as InvalidType should mark the value, not the key const typeIndex = getTypeLineNumber(type, lines); const lineIndex = getRelationLineNumber(relation, lines, typeIndex); collector.raiseInvalidType(decodedType, type, relation, { file, module }, lineIndex); @@ -725,7 +711,7 @@ function modelValidation( fileToModuleMap[file].add(module); } - const [hasEntry, loop] = hasEntryPointOrLoop( + const { hasEntry, loop } = hasEntryPointOrLoop( typeMap, typeName, relationName, diff --git a/tests/data/dsl-semantic-validation-cases.yaml b/tests/data/dsl-semantic-validation-cases.yaml index f8a555f4..caca702a 100644 --- a/tests/data/dsl-semantic-validation-cases.yaml +++ b/tests/data/dsl-semantic-validation-cases.yaml @@ -1412,6 +1412,508 @@ end: 29 metadata: errorType: invalid-relation-type + +# Cycle tests + +- name: cycle 1 should fail + dsl: | + model + schema 1.1 + type resource + relations + define x: y + define y: x + expected_errors: + - msg: "`x` is an impossible relation for `resource` (potential loop)." + line: + start: 4 + end: 4 + column: + start: 11 + end: 12 + metadata: + errorType: relation-no-entry-point + - msg: "`y` is an impossible relation for `resource` (potential loop)." + line: + start: 5 + end: 5 + column: + start: 11 + end: 12 + metadata: + errorType: relation-no-entry-point + +- name: cycle 2 should fail + dsl: | + model + schema 1.1 + type resource + relations + define x: y + define y: z + define z: x + expected_errors: + - msg: "`x` is an impossible relation for `resource` (potential loop)." + line: + start: 4 + end: 4 + column: + start: 11 + end: 12 + metadata: + errorType: relation-no-entry-point + - msg: "`y` is an impossible relation for `resource` (potential loop)." + line: + start: 5 + end: 5 + column: + start: 11 + end: 12 + metadata: + errorType: relation-no-entry-point + - msg: "`z` is an impossible relation for `resource` (potential loop)." + line: + start: 6 + end: 6 + column: + start: 11 + end: 12 + metadata: + errorType: relation-no-entry-point + +# Cycles currnetly not being caught + +- name: cycle 3 should fail + skip: true + dsl: | + model + schema 1.1 + type user + type resource + relations + define x: [user] or y + define y: [user] or z + define z: [user] or x + expected_errors: + - msg: "`x` is an impossible relation for `resource` (potential loop)." + line: + start: 5 + end: 5 + column: + start: 11 + end: 12 + metadata: + errorType: relation-no-entry-point + - msg: "`y` is an impossible relation for `resource` (potential loop)." + line: + start: 6 + end: 6 + column: + start: 11 + end: 12 + metadata: + errorType: relation-no-entry-point + - msg: "`z` is an impossible relation for `resource` (potential loop)." + line: + start: 7 + end: 7 + column: + start: 11 + end: 12 + metadata: + errorType: relation-no-entry-point + +- name: cycle 4 should fail + skip: true + dsl: | + model + schema 1.1 + type user + type resource + relations + define x: [user] but not y + define y: [user] but not z + define z: [user] or x + expected_errors: + - msg: "`x` is an impossible relation for `resource` (potential loop)." + line: + start: 5 + end: 5 + column: + start: 11 + end: 12 + metadata: + errorType: relation-no-entry-point + - msg: "`y` is an impossible relation for `resource` (potential loop)." + line: + start: 6 + end: 6 + column: + start: 11 + end: 12 + metadata: + errorType: relation-no-entry-point + - msg: "`z` is an impossible relation for `resource` (potential loop)." + line: + start: 7 + end: 7 + column: + start: 11 + end: 12 + metadata: + errorType: relation-no-entry-point + +- name: cycle 5 should fail + skip: true + dsl: | + model + schema 1.1 + type user + type group + relations + define member: [user] or memberA or memberB or memberC + define memberA: [user] or member or memberB or memberC + define memberB: [user] or member or memberA or memberC + define memberC: [user] or member or memberA or memberB + expected_errors: + - msg: "`member` is an impossible relation for `group` (potential loop)." + line: + start: 5 + end: 5 + column: + start: 11 + end: 17 + metadata: + errorType: relation-no-entry-point + - msg: "`memberA` is an impossible relation for `group` (potential loop)." + line: + start: 6 + end: 6 + column: + start: 11 + end: 18 + metadata: + errorType: relation-no-entry-point + - msg: "`memberB` is an impossible relation for `group` (potential loop)." + line: + start: 7 + end: 7 + column: + start: 11 + end: 18 + metadata: + errorType: relation-no-entry-point + - msg: "`memberC` is an impossible relation for `group` (potential loop)." + line: + start: 8 + end: 8 + column: + start: 11 + end: 18 + metadata: + errorType: relation-no-entry-point + +- name: cycle 6 should fail + skip: true + dsl: | + model + schema 1.1 + type user + type account + relations + define admin: [user] or member or super_admin or owner + define member: [user] or owner or admin or super_admin + define super_admin: [user] or admin or member or owner + define owner: [user] + expected_errors: + - msg: "`admin` is an impossible relation for `account` (potential loop)." + line: + start: 5 + end: 5 + column: + start: 11 + end: 16 + metadata: + errorType: relation-no-entry-point + - msg: "`member` is an impossible relation for `account` (potential loop)." + line: + start: 6 + end: 6 + column: + start: 11 + end: 17 + metadata: + errorType: relation-no-entry-point + - msg: "`super_admin` is an impossible relation for `account` (potential loop)." + line: + start: 7 + end: 7 + column: + start: 11 + end: 22 + metadata: + errorType: relation-no-entry-point + +# - name: cycle 7 +# dsl: | +# model +# schema 1.1 +# type user +# type document +# relations +# define editor: [user] +# define viewer: [document#viewer] or editor +# expected_errors: [] + +# Validate + +- name: direct_relationship_with_entrypoint should pass + dsl: | + model + schema 1.1 + type user + + type document + relations + define viewer: [user] + expected_errors: [] + +- name: computed_relationship_with_entrypoint should pass + dsl: | + model + schema 1.1 + type user + + type document + relations + define editor: [user] + define viewer: editor + expected_errors: [] + +- name: no_entrypoint_1 should fail + dsl: | + model + schema 1.1 + type user + + type document + relations + define admin: [user] + define action1: admin and action2 and action3 + define action2: admin and action1 and action3 + define action3: admin and action1 and action2 + expected_errors: + - msg: "`action1` is an impossible relation for `document` (potential loop)." + line: + start: 7 + end: 7 + column: + start: 11 + end: 18 + metadata: + errorType: relation-no-entry-point + - msg: "`action2` is an impossible relation for `document` (potential loop)." + line: + start: 8 + end: 8 + column: + start: 11 + end: 18 + metadata: + errorType: relation-no-entry-point + - msg: "`action3` is an impossible relation for `document` (potential loop)." + line: + start: 9 + end: 9 + column: + start: 11 + end: 18 + metadata: + errorType: relation-no-entry-point + +- name: no_entrypoint_2 should fail + dsl: | + model + schema 1.1 + type user + + type document + relations + define admin: [user] + define action1: admin but not action2 + define action2: admin but not action3 + define action3: admin but not action1 + expected_errors: + - msg: "`action1` is an impossible relation for `document` (potential loop)." + line: + start: 7 + end: 7 + column: + start: 11 + end: 18 + metadata: + errorType: relation-no-entry-point + - msg: "`action2` is an impossible relation for `document` (potential loop)." + line: + start: 8 + end: 8 + column: + start: 11 + end: 18 + metadata: + errorType: relation-no-entry-point + - msg: "`action3` is an impossible relation for `document` (potential loop)." + line: + start: 9 + end: 9 + column: + start: 11 + end: 18 + metadata: + errorType: relation-no-entry-point + +- name: no_entrypoint_3a should fail + dsl: | + model + schema 1.1 + type user + + type document + relations + define viewer: [document#viewer] and editor + define editor: [user] + expected_errors: + - msg: "`viewer` is an impossible relation for `document` (no entrypoint)." + line: + start: 6 + end: 6 + column: + start: 11 + end: 17 + metadata: + errorType: relation-no-entry-point + +- name: no_entrypoint_3b should fail + dsl: | + model + schema 1.1 + type user + + type document + relations + define viewer: [document#viewer] but not editor + define editor: [user] + expected_errors: + - msg: "`viewer` is an impossible relation for `document` (no entrypoint)." + line: + start: 6 + end: 6 + column: + start: 11 + end: 17 + metadata: + errorType: relation-no-entry-point + +- name: no_entrypoint_4 should fail + dsl: | + model + schema 1.1 + type user + + type folder + relations + define parent: [document] + define viewer: editor from parent + + type document + relations + define parent: [folder] + define editor: viewer + define viewer: editor from parent + expected_errors: + - msg: "the `editor` relation definition on type `document` is not valid: `editor` does not exist on `parent`, which is of type `folder`." + line: + start: 13 + end: 13 + column: + start: 19 + end: 37 + metadata: + errorType: invalid-relation-on-tupleset + +- name: self_referencing_type_restriction_with_entrypoint_1 should pass + dsl: | + model + schema 1.1 + type user + + type document + relations + define restricted: [user] + define editor: [user] + define viewer: [document#viewer] or editor + define can_view: viewer but not restricted + define can_view_actual: can_view + expected_errors: [] + +- name: self_referencing_type_restriction_with_entrypoint_2 should pass + dsl: | + model + schema 1.1 + type user + + type document + relations + define editor: [user] + define viewer: [document#viewer] or editor + expected_errors: [] + +# Models with loops, but are valid because of an entry point + +- name: union does not require all child to have entry + skip: true + dsl: | + model + schema 1.1 + type user + type doc + relations + define admin: [user] + define action1: admin or action2 or action3 + define action2: admin or action3 or action1 + define action3: admin or action1 or action2 + expected_errors: [] + +- name: union does not require all child to have entry even for intersection child + skip: true + dsl: | + model + schema 1.1 + type user + type docs + relations + define admin: [user] + define union1: admin or union2 + define union2: admin or union1 + define intersection1: union1 and union2 + define intersection2: union1 and union2 + expected_errors: [] + +- name: union does not require all child to have entry even for exclusion child + skip: true + dsl: | + model + schema 1.1 + type user + type docs + relations + define admin: [user] + define union1: admin or union2 + define union2: admin or union1 + define exclusion1: admin but not union1 + define exclusion2: admin but not union2 + expected_errors: [] + #- name: invalid cel expression # dsl: | # model @@ -1451,3 +1953,34 @@ # } # expected_errors: # - msg: some error + +- name: invalid type error maps to wrong location when duplicate relation name is used in direct assignment + dsl: | + model + schema 1.1 + type custome + type bank + relations + define customer : [customer] + type account + relations + define owner : [customer] + expected_errors: + - msg: "`customer` is not a valid type." + line: + start: 5 + end: 5 + column: + start: 23 + end: 31 + metadata: + errorType: invalid-type + - msg: "`customer` is not a valid type." + line: + start: 8 + end: 8 + column: + start: 20 + end: 28 + metadata: + errorType: invalid-type