Frontend Validation

[kingsleyzissou]

I've been looking into typescript validation libraries like https://zod.dev/ & https://valibot.dev/.

We could possibly benefit from using something like this in our frontend repo to help us tighten up our validation.

The benefits of using a library like this is that they have a lot of builtin validation types already and so we don't have to re-implement these (e.g. URL validation).

The validation is all declarative too and we could make re-usable schemas for common items such as SSH keys and CA certs. I'm sure there are more examples of this too. We can also assert that these validation schemas match the image builder api generated types so that there isn't any drift there.

We would then have a unified way of validating user input and hopefully this could improve the dev experience.

I have a simple example of how we can do this below. I chose timezone since it is fairly straightforward with enough custom logic to show how we could handle this:

const TimeZoneSchema = z.object({
  // this is just an example, not our actual validation rules
  // in reality, we would check if the input timezone is in the list of
  // known timezones
  timezone: z.string().min(3, 'Time zone too short').optional(),
  ntpservers: z
    .array(
      z
        .regex(
          /^([a-z0-9-]+)?(([.:/]{1,3}[a-z0-9-]+)){1,}$/,
          'Invalid NTP Server',
        ),
    )
    // we can add custom validation for duplicate items for example
    .superRefine((items, ctx) => {
     // I haven't included this function in the example
     // since it should be pretty clear what it does
      getDuplicateIndices(items).forEach((index) => {
        ctx.addIssue({
          code: 'custom',
          message: 'Duplicate NTP item',
          input: items,
          path: [index],
        });
      });
    }).optional(),
});

We can assert that the inferred schema type matches our image-builder-api type:

type Timezone = z.infer<typeof TimezoneSchema>
// type Timezone = {
//    timezone?: string | undefined;
//    ntpservers?: string[] | undefined;
// }

Here is how we can then test input against the validation schema:

const values = ['hello', 'hey', 'this', 'that', 'hi', 'this'];
const result = Schema.safeParse({ timezone: 'timezone', ntpservers: values });

With output:

ZodError: [
  {
    "origin": "string",
    "code": "invalid_format",
    "format": "regex",
    "pattern": "/^([a-z0-9-]+)?(([.:/]{1,3}[a-z0-9-]+)){1,}$/",
    "path": [
      "ntpservers",
      0
    ],
    "message": "Invalid NTP Server"
  },
  {
    "origin": "string",
    "code": "invalid_format",
    "format": "regex",
    "pattern": "/^([a-z0-9-]+)?(([.:/]{1,3}[a-z0-9-]+)){1,}$/",
    "path": [
      "ntpservers",
      1
    ],
    "message": "Invalid NTP Server"
  },
  {
    "origin": "string",
    "code": "invalid_format",
    "format": "regex",
    "pattern": "/^([a-z0-9-]+)?(([.:/]{1,3}[a-z0-9-]+)){1,}$/",
    "path": [
      "ntpservers",
      2
    ],
    "message": "Invalid NTP Server"
  },
  {
    "origin": "string",
    "code": "invalid_format",
    "format": "regex",
    "pattern": "/^([a-z0-9-]+)?(([.:/]{1,3}[a-z0-9-]+)){1,}$/",
    "path": [
      "ntpservers",
      3
    ],
    "message": "Invalid NTP Server"
  },
  {
    "origin": "string",
    "code": "invalid_format",
    "format": "regex",
    "pattern": "/^([a-z0-9-]+)?(([.:/]{1,3}[a-z0-9-]+)){1,}$/",
    "path": [
      "ntpservers",
      4
    ],
    "message": "Invalid NTP Server"
  },
  {
    "origin": "string",
    "code": "invalid_format",
    "format": "regex",
    "pattern": "/^([a-z0-9-]+)?(([.:/]{1,3}[a-z0-9-]+)){1,}$/",
    "path": [
      "ntpservers",
      5
    ],
    "message": "Invalid NTP Server"
  },
  {
    "code": "custom",
    "message": "Duplicate NTP item",
    "path": [
      "ntpservers",
      2
    ]
  },
  {
    "code": "custom",
    "message": "Duplicate NTP item",
    "path": [
      "ntpservers",
      5
    ]
  }
]

What's nice about this is that it returns all the errors and not just the first one. This could & should simplify the validation logic that we have. This also should make it easy to disable the next button when needed inside the Wizard. The downside is that it is a new "framework" for devs to learn.

[avitova]

Thanks for looking into this. I am already familiar with Zod a bit. I was actually thinking about implementing it into my thesis for university recently. From what I see, I believe once we implement it, we might as well benefit from it when it comes to the import feature, in case something breaks/we want to make it work better. It looks robust.
Now, as much as I like to make my life easier, I am also a little bit afraid of using such a library to handle our errors. On one side, we're probably dealing with things that others had to deal with as well. On the other hand, I view this as a very important part of our logic, and therefore I believe we might want to have it under control more. I do not know, honestly.
I'd be much happier if we had written something ourselves, although it will be lots of work. We usually have to deal with:

• Duplicate values - for chips, we can just not allow adding them (however, import complicates this), for textfields (related to usernames, for example), we need to show an error underneath both that are related)
• Invalid values - we usually add some kind of information on what the correct value should look like
• Empty value
• Warning - in the case of ssh or certificates, I do not think we should be judges of what value is correct. Let's help users as much as we can, but for SSH keys/certificates validation, we'd have to add some library anyway, and I do not like that.

The pain points I see:

• Sometimes there is more possible errors in one step, which is the hardest thing to solve, imho.
• There are arrays of structs, then we have to solve: 1) are all items in the array correct?, 2) is the array as a whole correct? Are there no duplicates in it? If so - return from the validation enough info to be able to tell: on which index there's an error, what errors are there in the whole array.
• Selectors: we should validate these properly as well. Although users cannot pick incorrect value directly, we must be able to validate it as well - that's needed for example on edit or on import. I'd be very happy if basically all fields had their validation.
• Already mentioned but the import can break lots of stuff, and with the new design for validations, I see this as an oportunity to really make this bulletproof and foolproof.

Something we might want to decide on:

1. Should we split validations into subfolders for specific steps? It might be a wise thing to start with, but I'd also be afraid of too much duplication.

What's nice about this is that it returns all the errors and not just the first one. This could & should simplify the validation logic that we have. This also should make it easy to disable the next button when needed inside the Wizard. The downside is that it is a new "framework" for devs to learn.

With the revamp, the disableNext might get very useless. With the number of fields in each step, we should make it VERY CLEAR what is expected from users. It would be very nice to show errors right when we see some, but also not to terrorise users too soon.

Some more thoughts:

• I would like it if we had validations rather split into specific fields following the WizardSlice structure. Maybe the same way we have selectUsers, we could have selectUsersValidation. I would like it if with errors, we mirrored the WizardSlice structure completely. So while selectUsers results in:

[
  {
    "name": "a",
    "password": "password",
    "ssh_key": "ssh-ed25519 xxx",
    "groups": [
      "wheel"
    ],
    "isAdministrator": true
  },
  {
      ...
  },
  {
      ...
  }
]

• selectUserValidations could be something like:

{
  "isValid": false,
  "errors": {
    "0": {
      "name": [
        "Name must be at least 2 characters"
      ],
    },
  },
  "warnings": {
    "0": {
      "ssh_key": [
        "SSH key something something"
      ]
    }
  }
}

And then in the same component where we call selectUsers, we'd also call selectUsersValidation:
const users = useAppSelector(selectUsers);
const usersValidations = useAppSelector(usersValidations);
Wherever we pass users[index], we pass also usersValidations[index].

users.map((user, index) => (
  <UserRow
    key={index}
    user={user}
    // Pass the specific validation slice for this user row
    validation={ {
        errors: usersValidation.errors[index] || {},
        warnings: usersValidation.warnings[index] || {}
    } }
  />
));

[kingsleyzissou]

Building our own thing from scratch scares me a little bit to be honest. Seems like a lot of work. As far as I know, zod can't handle warnings, but I'd have to look into that.

Funnily enough, the import case was where my mind went to. I actually needed that code when I was working on decomposer and I used zod to convert the blueprint type from the hosted version to on-prem and zod worked really well for that. I don't think we'll go ahead with that project, but here is a link to show this:
https://github.com/osbuild/decomposer/blob/38fc1690c8b14f392cddcbbebe950b484118490f/src/blueprint/hosted-to-on-prem.ts#L136

I do like the idea of splitting validation into folders to be honest. Should make the code a bit cleaner.

• Duplicate values - for chips, we can just not allow adding them (however, import complicates this), for textfields (related to usernames, for example), we need to show an error underneath both that are related)
• Invalid values - we usually add some kind of information on what the correct value should look like
• Empty value
• Warning - in the case of ssh or certificates, I do not think we should be judges of what value is correct. Let's help users as much as we can, but for SSH keys/certificates validation, we'd have to add some library anyway, and I do not like that.

Zod can handle duplicate values, invalid values & empty values. The above example I showed handles all of those. Warnings is the one thing I'm not sure on.

The pain points I see:

• Sometimes there is more possible errors in one step, which is the hardest thing to solve, imho.
• There are arrays of structs, then we have to solve: 1) are all items in the array correct?, 2) is the array as a whole correct? Are there no duplicates in it? If so - return from the validation enough info to be able to tell: on which index there's an error, what errors are there in the whole array.
• Selectors: we should validate these properly as well. Although users cannot pick incorrect value directly, we must be able to validate it as well - that's needed for example on edit or on import. I'd be very happy if basically all fields had their validation.
• Already mentioned but the import can break lots of stuff, and with the new design for validations, I see this as an oportunity to really make this bulletproof and foolproof.

I think zod can handle all of these usecases. I think this is a good example of where we should have unit tests to handle all of this.

[avitova]

Makes sense. As much as I would like to create my own validation here, I agree that this is a lot of work, so if we choose to use zod, I am happy with that decision as well. But one thought - if we really wanted to do it ourselves, I suggest borrowing our "backend guys". :) Might be a non-drastic way to include the rest of the team in the frontend effort. 👿

[regexowl]

Thanks for bringing this up!

The way we're handling validation at the moment sometimes feels overwhelming and incomplete at the same time to be honest.

Just from a quick look - zod looks quite interesting, not overly complicated and its weekly downloads on npm are going up so they have to be doing something right :D I like the examples and having re-usable schemas would be lovely.

The downside is that it is a new "framework" for devs to learn.

I mean yeah, but that's just part of the job 🤷

The main pain point seems to currently be inconsistency in validation between create/edit and import. We don't really allow users to input invalid values when they're creating/editing a blueprint, but they can still "force" the values in via import. Meaning there are two ways to input data and two ways we need to validate it, leaving us with possible gaps that are bubbling up to the surface and require more attention along the way. If this could be de-duplicated that would be perfect.

I can see there are also valid arguments for implementing something on our side, current validation or at least what we need from it can be quite complex, with nested/multiple errors. It's probably not possible to say if we'll be perfectly fine with a "pre-made" solution or if we'll encounter issues we'd need a custom rules and schemas for (possibly missing warnings might be one of blockers).

But I'd say we won't know until we give it a try 🤷

What I'd be interested in is how would new validation schemas work with components we already use.

Also all for splitting validation into more manageable files! I've been already tempted to add max-lines rule to our ESLint config several times, but didn't bring myself to do it as splitting everything in the same PR would be a pain. And it also might be a slight overkill... 🫠

[kingsleyzissou]

I can see there are also valid arguments for implementing something on our side, current validation or at least what we need from it can be quite complex, with nested/multiple errors. It's probably not possible to say if we'll be perfectly fine with a "pre-made" solution or if we'll encounter issues we'd need a custom rules and schemas for (possibly missing warnings might be one of blockers).

I think my reluctance with doing it ourselves is all the edge cases around even basic types. Using a library takes all of this away from us.

I was working on a proof of concept/spike for the last sprint and will carry on with it this sprint too (I'm away next week though). I have just tried the user creation fields for now and will maybe push the spike. But @lucasgarfield was quite keen for us to do an ADR document to capture all of this in a more formal way.

[regexowl]

I think my reluctance with doing it ourselves is all the edge cases around even basic types. Using a library takes all of this away from us.

Agree, managing the edge cases is pain. Excited to see the proof of concept!

[avitova]

Amazing, thanks for this! I think that the POC is a very good idea in this case, and it might give us more information on whether zod is a good thing for our use case. 👍 So thanks, and feel free to reach out. After you have the POC, I am happy to look into it!:)

[kingsleyzissou]

Here is a draft PR in the meantime:
osbuild/image-builder-frontend#3750

(I haven't hooked up the groups validation just yet)