'graphile behavior debug' & fix issue with badly scoped behaviors (#410)

Author: benjie

.changeset/three-coats-exist.md

@@ -0,0 +1,18 @@
+---
+"graphile-build-pg": patch
+"graphile-build": patch
+"graphile-utils": patch
+"postgraphile": patch
+---
+
+Use a single behavior check per location.
+
+In the past two weeks I added a few behavior strings like
+`array:attribute:filterBy` (a scoped form of `attribute:filterBy` to only be
+used by attributes that were arrays); however I've realised that this will
+require plugin authors to implement all the same logic to figure out what type
+an attribute is in order to then see if it has the relevant behavior. This goes
+against the design of the behavior system, and makes plugin authors' lives
+harder. So I've reverted this, and instead used the `entityBehaviors` system to
+add or remove the base `attribute:filterBy` (etc) behavior depending on what the
+type of the attribute is.

.changeset/young-kiwis-roll.md

@@ -0,0 +1,5 @@
+---
+"graphile": patch
+---
+
+Add new `graphile behavior debug` command for debugging behavior strings.

graphile-build/graphile-build-pg/src/plugins/PgAttributesPlugin.ts

@@ -8,7 +8,6 @@ import type {
   PgCodecAttribute,
   PgCodecAttributes,
   PgCodecList,
-  PgCodecRelation,
   PgCodecWithAttributes,
   PgConditionStep,
   PgSelectSingleStep,
@@ -294,7 +293,42 @@ export const PgAttributesPlugin: GraphileConfig.Plugin = {
 
   schema: {
     entityBehavior: {
-      pgCodecAttribute: "select base update insert",
+      pgCodecAttribute: {
+        provides: ["default"],
+        before: ["inferred", "override"],
+        callback(behavior, [codec, attributeName]) {
+          const behaviors = new Set([
+            "select base update insert filterBy orderBy",
+          ]);
+          const attribute = codec.attributes[attributeName];
+          function walk(codec: PgCodec) {
+            if (codec.arrayOfCodec) {
+              behaviors.add("-condition:attribute:filterBy");
+              behaviors.add(`-attribute:orderBy`);
+              walk(codec.arrayOfCodec);
+            } else if (codec.rangeOfCodec) {
+              behaviors.add(`-condition:attribute:filterBy`);
+              behaviors.add(`-attribute:orderBy`);
+              walk(codec.rangeOfCodec);
+            } else if (codec.domainOfCodec) {
+              // No need to add a behavior for domain
+              walk(codec.domainOfCodec);
+            } else if (codec.attributes) {
+              behaviors.add(`-condition:attribute:filterBy`);
+              behaviors.add(`-attribute:orderBy`);
+            } else if (codec.isBinary) {
+              // Never filter, not in condition plugin nor any other
+              behaviors.add(`-attribute:filterBy`);
+              behaviors.add(`-attribute:orderBy`);
+            } else {
+              // Done
+            }
+          }
+          walk(attribute.codec);
+
+          return [...behaviors, behavior];
+        },
+      },
     },
     hooks: {
       GraphQLInterfaceType_fields(fields, build, context) {
@@ -384,12 +418,7 @@ export const PgAttributesPlugin: GraphileConfig.Plugin = {
         return fields;
       },
       GraphQLInputObjectType_fields(fields, build, context) {
-        const {
-          extend,
-          inflection,
-          input: { pgRegistry },
-          sql,
-        } = build;
+        const { extend, inflection, sql } = build;
         const {
           scope: {
             isPgRowType,
@@ -440,56 +469,21 @@ export const PgAttributesPlugin: GraphileConfig.Plugin = {
         return Object.entries(attributes).reduce(
           (memo, [attributeName, attribute]) =>
             build.recoverable(memo, () => {
-              const action = isPgBaseInput
-                ? "base"
+              const fieldBehaviorScope = isPgBaseInput
+                ? `attribute:base`
                 : isPgPatch
-                ? "update"
+                ? `attribute:update`
                 : isPgCondition
-                ? "filterBy"
-                : "insert";
-
-              const behaviors: string[] = [];
-              function walk(codec: PgCodec) {
-                if (codec.arrayOfCodec) {
-                  behaviors.push(`array:attribute:${action}`);
-                  walk(codec.arrayOfCodec);
-                } else if (codec.rangeOfCodec) {
-                  behaviors.push(`range:attribute:${action}`);
-                  walk(codec.rangeOfCodec);
-                } else if (codec.domainOfCodec) {
-                  // No need to add a behavior for domain
-                  walk(codec.domainOfCodec);
-                } else if (codec.attributes) {
-                  behaviors.push(`composite:attribute:${action}`);
-                } else if (codec.isBinary) {
-                  behaviors.push(`binary:attribute:${action}`);
-                } else {
-                  behaviors.push(`scalar:attribute:${action}`);
-                }
-              }
-              walk(attribute.codec);
-
-              const relations = (
-                Object.values(
-                  pgRegistry.pgRelations[pgCodec.name] ?? {},
-                ) as PgCodecRelation[]
-              ).filter((r) => !r.isReferencee && r.isUnique);
-              const isPartOfRelation = relations.some((r) =>
-                r.localAttributes.includes(attributeName),
-              );
-              if (isPartOfRelation) {
-                behaviors.push(`relation:attribute:${action}`);
-              }
-
-              for (const behavior of behaviors) {
-                if (
-                  !build.behavior.pgCodecAttributeMatches(
-                    [pgCodec, attributeName],
-                    behavior,
-                  )
-                ) {
-                  return memo;
-                }
+                ? `condition:attribute:filterBy`
+                : `attribute:insert`;
+
+              if (
+                !build.behavior.pgCodecAttributeMatches(
+                  [pgCodec, attributeName],
+                  fieldBehaviorScope,
+                )
+              ) {
+                return memo;
               }
 
               const fieldName = inflection.attribute({
@@ -514,7 +508,7 @@ export const PgAttributesPlugin: GraphileConfig.Plugin = {
                   [fieldName]: fieldWithHooks(
                     {
                       fieldName,
-                      fieldBehaviorScope: `attribute:${action}`,
+                      fieldBehaviorScope,
                       pgCodec,
                       pgAttribute: attribute,
                       isPgConnectionConditionInputField: isPgCondition,

graphile-build/graphile-build-pg/src/plugins/PgConditionArgumentPlugin.ts

@@ -42,7 +42,6 @@ export const PgConditionArgumentPlugin: GraphileConfig.Plugin = {
   schema: {
     entityBehavior: {
       pgCodec: "select filter",
-      pgCodecAttribute: "filterBy -binary:attribute:filterBy",
       pgResource: {
         provides: ["default"],
         before: ["inferred", "override"],

graphile-build/graphile-build-pg/src/plugins/PgOrderAllAttributesPlugin.ts

@@ -2,7 +2,6 @@ import "./PgTablesPlugin.js";
 import "graphile-config";
 
 import type {
-  PgCodec,
   PgCodecAttribute,
   PgCodecWithAttributes,
   PgResourceUnique,
@@ -45,11 +44,6 @@ export const PgOrderAllAttributesPlugin: GraphileConfig.Plugin = {
   },
 
   schema: {
-    entityBehavior: {
-      // Enable ordering, but don't order by array, range or binary types
-      pgCodecAttribute:
-        "orderBy -array:attribute:orderBy -range:attribute:orderBy -binary:attribute:orderBy",
-    },
     hooks: {
       GraphQLEnumType_values(values, build, context) {
         const { extend, inflection, options } = build;
@@ -101,35 +95,14 @@ export const PgOrderAllAttributesPlugin: GraphileConfig.Plugin = {
           values,
           Object.entries(attributes).reduce(
             (memo, [attributeName, attribute]) => {
-              const behaviors: string[] = [];
-              function walk(codec: PgCodec) {
-                if (codec.arrayOfCodec) {
-                  behaviors.push("array:attribute:orderBy");
-                  walk(codec.arrayOfCodec);
-                } else if (codec.rangeOfCodec) {
-                  behaviors.push("range:attribute:orderBy");
-                  walk(codec.rangeOfCodec);
-                } else if (codec.domainOfCodec) {
-                  // No need to add a behavior for domain
-                  walk(codec.domainOfCodec);
-                } else if (codec.attributes) {
-                  behaviors.push("composite:attribute:orderBy");
-                } else if (codec.isBinary) {
-                  behaviors.push("binary:attribute:orderBy");
-                } else {
-                  behaviors.push("scalar:attribute:orderBy");
-                }
-              }
-              walk(attribute.codec);
-              for (const behavior of behaviors) {
-                if (
-                  !build.behavior.pgCodecAttributeMatches(
-                    [pgCodec, attributeName],
-                    behavior,
-                  )
-                ) {
-                  return memo;
-                }
+              const fieldBehaviorScope = `attribute:orderBy`;
+              if (
+                !build.behavior.pgCodecAttributeMatches(
+                  [pgCodec, attributeName],
+                  fieldBehaviorScope,
+                )
+              ) {
+                return memo;
               }
               const isUnique = uniques.some(
                 (list) => list.attributes[0] === attributeName,

graphile-build/graphile-build/src/behavior.ts

@@ -79,6 +79,8 @@ export class Behavior {
     };
   };
 
+  public behaviorEntityTypes: (keyof GraphileBuild.BehaviorEntities)[] = [];
+
   private globalDefaultBehavior: ResolvedBehavior;
   constructor(
     private resolvedPreset: GraphileConfig.ResolvedPreset,
@@ -152,6 +154,7 @@ export class Behavior {
       (this as this & BehaviorDynamicMethods).stringBehavior = (str) => str;
       return;
     }
+    this.behaviorEntityTypes.push(entityType);
     this.behaviorEntities[entityType] = {
       behaviorCallbacks: [],
       listCache: new Map(),
@@ -274,6 +277,14 @@ export class Behavior {
     } ${specString ?? ""}`;
     return stringMatches(finalBehaviorSpecsString, filter);
   }
+
+  parseBehaviorString(behaviorString: string) {
+    return parseSpecs(behaviorString);
+  }
+
+  parseScope(filter: string) {
+    return parseScope(filter);
+  }
 }
 
 /**
@@ -295,6 +306,7 @@ function parseSpecs(behaviorSpecsString: string): BehaviorSpec[] {
   const fragments = behaviorSpecsString.split(/\s+/);
   const specs: BehaviorSpec[] = [];
   for (const fragment of fragments) {
+    if (fragment === "") continue;
     // `+` is implicit
     const [pm, rest] = /^[+-]/.test(fragment)
       ? [fragment.slice(0, 1), fragment.slice(1)]
@@ -323,6 +335,10 @@ function parseSpecs(behaviorSpecsString: string): BehaviorSpec[] {
 function scopeMatches(
   specifiedScope: BehaviorScope,
   filterScope: BehaviorScope,
+  // We only need to know positive or negative in case the filter contains an `*`.
+  // This is because if you filter for '*:foo' against '+bar:foo -baz:foo' then
+  // we should skip the negative (`-baz:foo`) because we only need _one_ match,
+  // not *every* match.
   positive: boolean,
 ): boolean {
   if (specifiedScope.length > filterScope.length) {