fix(ui): Trim leading and trailing whitespaces in forms

[bs-ondem]

Some PUT and POST endpoints have built-in backend validation, which can cause a 400 Bad Request error in the frontend. Trimming leading and trailing whitespaces from those request bodies prevents such errors.

[sschuberth]

Some PUT and POST endpoints have built-in backend validation

A general question here: Why don't we simply trim values in the backend, instead of requiring all frontends (UI, CLI) to trim values, and then just do a validation in the backend?

[bs-ondem]

Trimming the values in the backend might also an option, didn't think about it. Could that cause an other issue, when we do it in the backend? However, this PR trims the values in the frontend.

[sschuberth]

Could that cause an other issue, when we do it in the backend?

I'm not sure. Maybe we do not want such "magic" to happen automatically, as there would be no notification to the user about this? Because usually, users probably expect values to be used exactly as entered.

However, as it's currently implemented in this PR, it's still "magic", just implemented in the frontend instead of the backend. A very transparent and "non-magic" way to implement this would be to forbid the user to submit something if there are trailing spaces, and require the user to remove them by themself.

So in total I see these options:

1. Do automatic trimming
   a. in the fronent (like in the current state of this PR)
   b. in the backend (avoids the need to implement trimming in all frontends)
2. Not do automatic trimming
   a. implement validation in the frontend that forbids users to submit with trailing spaces
   b. implement validation in the backend that sends back an error response to show to the user

[mnonnenmacher]

I think we can safely assume that users never intentionally want to add leading or trailing whitespace to entity names so automatic trimming in the backend should be fine. But I think in addition we should also trim those names in the frontend.
@bs-ondem How does your implementation work, does it trim only when sending the request or does it also update the visible values in the form when changing focus?

[bs-ondem]

@bs-ondem How does your implementation work, does it trim only when sending the request or does it also update the visible values in the form when changing focus?

It only trims the values when sending the request.

[bs-ondem]

So in total I see these options:

1. Do automatic trimming
   a. in the fronent (like in the current state of this PR)
   b. in the backend (avoids the need to implement trimming in all frontends)
2. Not do automatic trimming
   a. implement validation in the frontend that forbids users to submit with trailing spaces
   b. implement validation in the backend that sends back an error response to show to the user

Option 2b is already implemented, but maybe we could also implement option 2a? This would avoid any magic in the frontend. But I'm not sure.

[sschuberth]

But I think in addition we should also trim those names in the frontend.

Why do both, trimming in the frontend and backend? To me, the main point of trimming in the backend is to avoid the need to do it elsewhere, to reduce code duplication that can easily go out-of-sync.

[mnonnenmacher]

But I think in addition we should also trim those names in the frontend.

Why do both, trimming in the frontend and backend? To me, the main point of trimming in the backend is to avoid the need to do it elsewhere, to reduce code duplication that can easily go out-of-sync.

Just for UX, but that only makes sense if the form inputs are actually updated. To me also the backend trimming would be done only for UX to avoid user frustration when accidentally adding whitespace, from a technical perspective validation is totally sufficient.

[Etsija]

In here and similar places, omitting the .min(1) originally has clearly been an oversight from the UI developers, as there is no possibility for an infrastructure service to exist with an empty name. Also, a name is enforced by the backend; I can see it from the generated query client, where the name is non-nullable.

I would suggest that in all places similar to this, you replace z.string() with z.string().min(1).trim(), so trim the string, and require it to be at least 1 character after trimming the whitespaces.

[Etsija]

This comment is of course pending on the discussion of whether we should do everything in the back-end after all, instead of front-end.

We nowadays also have a plugin to generate Zod schemas from the OpenAPI spec, and will gradually use that in many places to partially or completely replace these manually typed form schemas throughout the UI.

Example: for creating infrastructure services, we use currently these "hand-made" form schemas:

const formSchema = z.object({
  name: z.string(),
  url: z.url(),
  description: z.string().optional(),
  usernameSecretRef: z.string(),
  passwordSecretRef: z.string(),
  credentialsTypes: z.array(z.enum(['NETRC_FILE', 'GIT_CREDENTIALS_FILE'])),
});

but we could just import and use an auto-generated Zod schema instead:

/**
 * PostInfrastructureService
 */
export const zPostInfrastructureService = z.object({
  credentialsTypes: z.optional(z.array(zCredentialsType)),
  description: z.optional(z.union([z.null(), z.string()])),
  name: z.string(),
  passwordSecretRef: z.string(),
  url: z.string(),
  usernameSecretRef: z.string(),
});

As I said, these are all auto-generated from the OpenAPI spec, but they of course don't include features like trimming, so if back-end doesn't do it, then we anyway need to, by refining or extending (simpler) the auto-generated schema further in the UI.

[Etsija]

The is an example of importing the auto-generated schema and extending it in the UI:

import { zPostInfrastructureService } from '@/api/zod.gen';

const formSchema = zPostInfrastructureService.extend({
  name: z.string().min(1).trim(),
});

[sschuberth]

What confuses me here is: Shouldn't it be .string().trim().min(1) to signal the the length should be at least 1 after trimming, not before?

[bs-ondem]

I was not aware of the generated Zod schemas. Depending on how we continue with this PR, i would just import the generated schema and extend it as shown.

What confuses me here is: Shouldn't it be .string().trim().min(1) to signal the the length should be at least 1 after trimming, not before?

Yes, it should be .strimg().trim().min(1).

But what are we doing now with this PR, do we want to have such a magic trimming of the values before sending the request, or not?

[Etsija]

I'm fine with it done in the front-end, at least for now.

[bs-ondem]

@sschuberth What do you think about it?

[sschuberth]

I have no strong opinion here right now. Please continue the review with @Etsija from my end, as it's him who still has pending requested changes, it seems.

[Etsija]

So from my part, it's enough for now to add the .min(1) for those fields where it doesn't make sense to allow empty strings (names, mostly), so z.string() -> z.string().trim().min(1). Once that's done, I can approve from my part.