🐛 Bug Report: Scaffold field validates empty value even if the field is not required

[tylermichael]

📜 Description

Scaffold custom field validates empty value even if the field is not required.

👍 Expected behavior

Validation should not happen if the field is optional and the fields value is undefined / empty.

I feel like if you don't want to allow an empty value, then the field should be made required.

👎 Actual Behavior with Screenshots

Validation always happens.

👟 Reproduction steps

Component:

import React from 'react';
import { FieldValidation } from '@rjsf/core';
import { KubernetesValidatorFunctions } from '@backstage/catalog-model';
import {
  scaffolderPlugin,
  createScaffolderFieldExtension,
  FieldExtensionComponentProps,
} from '@backstage/plugin-scaffolder';
import { TextField } from '@material-ui/core';

const TextValuePicker = (props: FieldExtensionComponentProps<string>) => {
  const {
    onChange,
    required,
    schema: { title, description },
    rawErrors,
    formData,
    uiSchema: { 'ui:autofocus': autoFocus },
    idSchema,
    placeholder,
  } = props;

  return (
    <TextField
      id={idSchema?.$id}
      label={title}
      placeholder={placeholder}
      helperText={description}
      required={required}
      value={formData ?? ''}
      onChange={({ target: { value } }) => onChange(value)}
      margin="normal"
      error={rawErrors?.length > 0 && !formData}
      inputProps={{ autoFocus }}
    />
  );
};

export const k8sObjectNameValidation = (
  value: string,
  validation: FieldValidation,
) => {
  if (!KubernetesValidatorFunctions.isValidObjectName(value)) {
    validation.addError(
      'must start and end with an alphanumeric character, and contain only alphanumeric characters, hyphens, underscores, and periods. Maximum length is 63 characters.',
    );
  }
};

export const SafeTextExtension = scaffolderPlugin.provide(
  createScaffolderFieldExtension({
    name: 'SafeTextExtension',
    component: TextValuePicker,
    validation: k8sObjectNameValidation,
  }),
);

Template:

apiVersion: scaffolder.backstage.io/v1beta3
kind: Template
metadata:
  name: v1beta3-demo
  title: Node.js web API Project template
  description: Creates a web API written in Node.js
spec:
  owner: backstage/techdocs-core
  type: service

  # these are the steps which are rendered in the frontend with the form input
  parameters:
    - title: Fill in some steps
      required:
        - name
        - owner
      properties:
        name:
          title: Name
          type: string
          description: Unique name of the component
          ui:field: SafeTextExtension
        description:
          title: Description
          type: string
          description: Description of the project
        path:
          title: Path
          type: string
          description: Path to where the project should be generated
          ui:field: SafeTextExtension
        owner:
          title: Owner
          type: string
          description: Owner of the component
          ui:field: OwnerPicker
          ui:options:
            allowedKinds:
              - Group
  steps:
    - name: Dummy step
      action: debug:log
      input:
        listWorkspace: true

📃 Provide the context for the Bug.

I have an optional field that should only be validated if it has a value. It's fine if the value is empty.

I don't want to change my validator to explicitly allow empty values, because if the field is required or not is driven by the template.

🖥️ Your Environment

OS: Darwin 21.6.0 - darwin/arm64 node: v16.15.1 yarn: 1.18.0 cli: 0.19.0 (installed) backstage: 1.6.0

Dependencies: @backstage/app-defaults 1.0.6 @backstage/backend-common 0.15.1 @backstage/backend-plugin-api 0.1.2 @backstage/backend-tasks 0.3.5 @backstage/catalog-client 1.1.0 @backstage/catalog-model 1.1.1 @backstage/cli-common 0.1.10 @backstage/cli 0.19.0 @backstage/config-loader 1.1.4 @backstage/config 1.0.2 @backstage/core-app-api 1.1.0 @backstage/core-components 0.11.1 @backstage/core-plugin-api 1.0.6 @backstage/errors 1.1.1 @backstage/integration-react 1.1.4 @backstage/integration 1.3.1 @backstage/plugin-api-docs 0.8.9 @backstage/plugin-app-backend 0.3.36 @backstage/plugin-auth-backend 0.16.0 @backstage/plugin-auth-node 0.2.5 @backstage/plugin-catalog-backend 1.4.0 @backstage/plugin-catalog-common 1.0.6 @backstage/plugin-catalog-graph 0.2.21 @backstage/plugin-catalog-import 0.8.12 @backstage/plugin-catalog-node 1.1.0 @backstage/plugin-catalog-react 1.1.4 @backstage/plugin-catalog 1.5.1 @backstage/plugin-github-actions 0.5.9 @backstage/plugin-home 0.4.25 @backstage/plugin-org 0.5.9 @backstage/plugin-pagerduty 0.5.2 @backstage/plugin-permission-common 0.6.4 @backstage/plugin-permission-node 0.6.5 @backstage/plugin-permission-react 0.4.5 @backstage/plugin-proxy-backend 0.2.30 @backstage/plugin-scaffolder-backend 1.6.0 @backstage/plugin-scaffolder-common 1.2.0 @backstage/plugin-scaffolder 1.6.0 @backstage/plugin-search-backend-module-pg 0.4.0 @backstage/plugin-search-backend-node 1.0.2 @backstage/plugin-search-backend 1.0.2 @backstage/plugin-search-common 1.0.1 @backstage/plugin-search-react 1.1.0 @backstage/plugin-search 1.0.2 @backstage/plugin-stack-overflow 0.1.5 @backstage/plugin-tech-radar 0.5.16 @backstage/plugin-techdocs-backend 1.3.0 @backstage/plugin-techdocs-module-addons-contrib 1.0.4 @backstage/plugin-techdocs-node 1.4.0 @backstage/plugin-techdocs-react 1.0.4 @backstage/plugin-techdocs 1.3.2 @backstage/plugin-user-settings 0.4.8 @backstage/release-manifests 0.0.6 @backstage/test-utils 1.2.0 @backstage/theme 0.2.16 @backstage/types 1.0.0 @backstage/version-bridge 1.0.1 ✨ Done in 0.74s.

👀 Have you spent some time to check if this bug has been raised before?

• [X] I checked and didn't find similar issue

🏢 Have you read the Code of Conduct?

• [X] I have read the Code of Conduct

Are you willing to submit PR?

No, I don't have time to work on this right now

[freben]

Alright, makes sense I think. Hoping for it to be picked up soon by somebody. 🙏 At least there's a workaround as you mention, to permit the empty value in the validator itself.

[Rugvip]

I think the way forward here is to update your k8sObjectNameValidation function to allow empty values. I don't think we should move this to part of the framework tbh, because that would remove the ability to create custom errors messages for missing values, and potentially other checks for more elaborate value types.

[tylermichael]

With the component from my initial example (and if the logic I proposed is implemented), you can allow empty values by not marking the field required, while having validation logic IF it was given a value. If the field is marked required, the validation always happens. The framework already doesn't allow empty values if the field is required.

So the key here is to mark the field required if you don't want to allow empty values. Then, the inverse for optional fields is consistent. If you don't pass a value, it doesn't run the validator and it doesn't raise an error about an empty value.

For my use case, my text input is used for different fields, some are required, others aren't. So now to have this logic work I'd have to have a duplicate component with slightly different validation logic and use them accordingly, which isn't ideal.

[Rugvip]

@tylermichael it's a safe assumption that if you mark a field required, then you must enter some value. I wouldn't say it's a safe assumption that just because you don't mark it required you don't want to run any validation at all. You might for example have a hidden dependency on a different field and want to make it required only in some specific circumstances. I don't think it makes sense to lock that down at the framework level.

I'm not seeing how you need separate components though, like you said, the framework takes care of checking for required values. I think all you need to do is to make sure it's not throwing an error if it's empty?

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[benjdlambert]

discussion on this topic came up here https://github.com/backstage/backstage/pull/16365

We decided that validation should only be ran for required values, and struggled to come up with a use case that we want to run the validation function when no field that can't be expressed with jsonschema. Feeling that we can go this route for now, and if this is a requirement, we should update the types so that it is a concious breaking change, that validation functions have an optional first argument that you need to guard against because right now this behaviour is not captured by typescript which causes unexpected behaviour.

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.