Fix validation hook merge function passthrough

[acburdine]

In #9166 a merge function was added to ensure that the deprecated valdiateInput/validateDelete syntax could be used by users alongside the newer validate syntax used by builtin fields.

Because we're passing through the user-specified hooks and only overriding the validate one, however, the deprecated hooks are still being passed through which leads to an error in the parseFieldHooks check later on.

This sets the deprecated fields to undefined in the merge function so that they don't get passed through from user hooks.

[acburdine]

@dcousens without this fix anyone who updates to 6.2.0 and uses the deprecated validate field hook syntax in conjunction with built-in field validation will get an error (kind of the inverse of the problem I had before trying to use new syntax 😅 )

[kennedybaird]

@acburdine - That change LGTM - but I'm wondering if L#55 could also be an issue? Don't really have my head around it fully, but seems like that lets the old hooks through

[acburdine]

@kennedybaird I don't believe L#55 should be an issue - the problem occurs when you mix the old validateInput syntax with the new unified validate syntax. In the case of L#55 (user hooks using deprecated syntax, no builtin hooks), there would only be the user hooks in the final hooks output (meaning there isn't a mix and thus no issue)

[kennedybaird]

@kennedybaird I don't believe L#55 should be an issue - the problem occurs when you mix the old validateInput syntax with the new unified validate syntax. In the case of L#55 (user hooks using deprecated syntax, no builtin hooks), there would only be the user hooks in the final hooks output (meaning there isn't a mix and thus no issue)

Ok, I'm wondering if we want to handle this for users during the transition? If I change the Person list in examples/usecase-todo/schema.ts to:

  Person: list({
    access: allowAll,
    fields: {
      name: text({
        validation: { isRequired: true }, isIndexed: 'unique',
        hooks: {
          validateInput: ({ resolvedData, operation, addValidationError }) => {
            console.log('validateInput', operation, resolvedData)
            if (resolvedData.name === 'forbidden') {
              addValidationError('name')
            }
          },
          validate: {
            create: ({ resolvedData, addValidationError }) => {
              console.log('validate create', resolvedData)
              if (resolvedData.name === 'forbidden') {
                addValidationError('name_second_create')
              }
            }
          }
        },
      }),

      tasks: relationship({ ref: 'Task.assignedTo', many: true }),
    },
  }),

then run pnpm dev, and create a new person with name forbidden - it only ever uses the validateInput method (even if the order is swapped around).

Again, not sure if it's an issue - just noting in case it is. To me it seems a bit odd, I get no indication in my editor that this won't behave normally:

[acburdine]

@kennedybaird hmm - that's a good point. That might be solvable with typings though - I can see if there's an easy typing solution to make it clear it's an either-or

[acburdine]

hmm - so I added the type changes to make the different new/old syntaxes exclusive, but it does cause some issues for people defining custom fields (it currently breaks the custom field example)

there are a couple of options I can think of, depending on how long it'll be until there's a planned major release and the deprecated syntax will be removed:

1. leave the types as-is for now, maybe update the documentation to note that the different syntaxes are mutually exclusive
2. export the mergeFieldHooks or resolveValidateHooks function from packages/core/fields/resolve-hooks, allowing custom fields to more easily compose hooks (might necessitate moving the function to core/src/types, idk)

[dcousens]

If we assume that fields are responsible [for now] to ensure only one syntax prevails, we can apply that to fields in this function as we move forward.

Inevitably they are "changed" internally to use the new syntax anyway, we can simply move that change higher.

[dcousens]

Remembering for now too, technically this syntax isn't completely supported by Keystone's external field configuration type, more that each of our fields are helpfully supporting the new syntax for you to upgrade earlier.

[acburdine]

Went ahead and removed the type change in favor of modifying the resolveValidateHook function to just merge the old and new syntax together - seems to me like that's the best approach until there's a breaking release (that could be what you all were suggesting and it's just too late in the day for me to understand words 😅)

[kennedybaird]

LGTM 🚀 nice @acburdine

[gautamsi]

I have some work done in #9204 which would completely remove the dual syntax and use one only, possibly we can add a check if people are using older syntax we throw error at run time instead of just type error.