Merge pull request #538 from authzed/537-fix-warning-output

Fix regression in warning output

Author: josephschorr

internal/cmd/validate-test/warnings-point-at-right-line.yaml

@@ -0,0 +1,28 @@
+schema: |
+  definition user {}
+
+  definition organization {}
+
+  definition platform {}
+
+  definition resource {
+    /** platform is the platform to which the resource belongs */
+    relation platform: platform
+
+    /**
+     * organization is the organization to which the resource belongs
+     */
+    relation organization: organization
+
+    /** admin is a user that can administer the resource */
+    relation admin: user
+
+    /** viewer is a read-only viewer of the resource */
+    relation viewer: user
+
+    /** can_admin allows a user to administer the resource */
+    permission can_admin = admin
+
+    /** delete_resource allows a user to delete the resource. */
+    permission delete_resource = can_admin
+  }

internal/cmd/validate-test/warnings-point-at-right-line.zed

@@ -0,0 +1,27 @@
+definition user {}
+
+definition organization {}
+
+definition platform {}
+
+definition resource {
+	/** platform is the platform to which the resource belongs */
+	relation platform: platform
+
+	/**
+	 * organization is the organization to which the resource belongs
+	 */
+	relation organization: organization
+
+	/** admin is a user that can administer the resource */
+	relation admin: user
+
+	/** viewer is a read-only viewer of the resource */
+	relation viewer: user
+
+	/** can_admin allows a user to administer the resource */
+	permission can_admin = admin
+
+	/** delete_resource allows a user to delete the resource. */
+	permission delete_resource = can_admin
+}

internal/cmd/validate.go

@@ -150,6 +150,7 @@ func validateCmdFunc(cmd *cobra.Command, filenames []string) (string, bool, erro
 		}
 
 		var parsed validationfile.ValidationFile
+		// the decoder is also where compilation happens.
 		validateContents, isOnlySchema, err := decoder(&parsed)
 		standardErrors, composableErrs, otherErrs := classifyErrors(err)
 
@@ -208,6 +209,8 @@ func validateCmdFunc(cmd *cobra.Command, filenames []string) (string, bool, erro
 			return "", false, err
 		}
 		if devErrs != nil {
+			// Calculate the schema offset, used for outputting errors and warnings
+			// and having them point to the right place regardless of zed vs yaml
 			schemaOffset := parsed.Schema.SourcePosition.LineNumber
 			if isOnlySchema {
 				schemaOffset = 0

internal/cmd/validate_test.go

@@ -215,7 +215,7 @@ complete - 0 relationships loaded, 0 assertions run, 0 expected relations valida
 				filepath.Join("validate-test", "missing-relation.zed"),
 			},
 			expectNonZeroStatusCode: true,
-			expectStr: "error: parse error in `write`, line 2, column 20: relation/permission `write` not found under definition `test`                   \n" +
+			expectStr: "error: parse error in `write`, line 2, column 21: relation/permission `write` not found under definition `test`                   \n" +
 				" 1 |  definition test {\n" +
 				" 2 >   permission view = write\n" +
 				"   >                     ^~~~~\n " +
@@ -297,6 +297,18 @@ complete - 0 relationships loaded, 0 assertions run, 0 expected relations valida
 complete - 0 relationships loaded, 0 assertions run, 0 expected relations validated
 `,
 		},
+		`warnings_point_at_correct_line_in_zed`: {
+			files: []string{
+				filepath.Join("validate-test", "warnings-point-at-right-line.zed"),
+			},
+			expectStr: "warning: Permission \"delete_resource\" references parent type \"resource\" in its name; it is recommended to drop the suffix (relation-name-references-parent)\n 23 |  permission can_admin = admin\n 24 | \n 25 |  /** delete_resource allows a user to delete the resource. */\n 26 >  permission delete_resource = can_admin\n    >             ^~~~~~~~~~~~~~~\n 27 | }\n 28 | \n\ncomplete - 0 relationships loaded, 0 assertions run, 0 expected relations validated\n",
+		},
+		`warnings_point_at_correct_line_in_yaml`: {
+			files: []string{
+				filepath.Join("validate-test", "warnings-point-at-right-line.yaml"),
+			},
+			expectStr: "warning: Permission \"delete_resource\" references parent type \"resource\" in its name; it is recommended to drop the suffix (relation-name-references-parent)\n 23 |     /** can_admin allows a user to administer the resource */\n 24 |     permission can_admin = admin\n 25 | \n 26 >     /** delete_resource allows a user to delete the resource. */\n    >         ^~~~~~~~~~~~~~~\n 27 |     permission delete_resource = can_admin\n 28 |   }\n\ncomplete - 0 relationships loaded, 0 assertions run, 0 expected relations validated\n",
+		},
 	}
 
 	for name, tc := range testCases {
@@ -306,7 +318,6 @@ complete - 0 relationships loaded, 0 assertions run, 0 expected relations valida
 			require := require.New(t)
 			cmd := zedtesting.CreateTestCobraCommandWithFlagValue(t,
 				zedtesting.StringFlag{FlagName: "schema-type", FlagValue: tc.schemaTypeFlag},
-				zedtesting.BoolFlag{FlagName: "force-color", FlagValue: false},
 				zedtesting.IntFlag{FlagName: "batch-size", FlagValue: 100},
 				zedtesting.IntFlag{FlagName: "workers", FlagValue: 1},
 				zedtesting.BoolFlag{FlagName: "fail-on-warn", FlagValue: false},

internal/decode/decoder.go

@@ -200,7 +200,9 @@ func compileSchemaFromData(filename, schemaString string, out interface{}) error
 		SchemaString: schemaString,
 	}, composable.AllowUnprefixedObjectType(), composable.SourceFolder(inputSourceFolder))
 
-	if composableCompileErr == nil {
+	// We'll only attempt to generate the composable schema string if we don't already
+	// have one from standard schema compilation
+	if composableCompileErr == nil && vfile.Schema.Schema == "" {
 		compiledSchemaString, _, err := generator.GenerateSchema(composableCompiled.OrderedDefinitions)
 		if err != nil {
 			return fmt.Errorf("could not generate string schema: %w", err)