pods: Fix PodCreateModal validation issues #1884

mvollmer · 2024-10-17T10:28:23Z

I have found four issues:

Validation results would not always make it into the UI while typing into the fields. This was confusing, but hitting "Create" would always straighten things out.
Removing port rows and then hitting "Create" while something was invalid would shift the (genuine) validation errors into the places of the previous removed ports. There they would invisbly keep the "Create" button from ever getting enabled again.
<TextInput type="number" /> does not usefully distinguish between empty strings and strings that are not numbers, and it sends fewer change notifications than expected. For example, starting to type random letters into a previously empty field will produce no change events and we get no opportunity to run our validation code.
The Volumes are not validated at the top-level, but do have their own validation rules. (Just "container path can not be empty".) Instead of showing validation errors, invalid volumes are simply omitted when creating the pod.

We need to construct a new one. Otherwise React will assume that the state hasn't actually changed. This might have worked by accident in the past, but React is clear that you should not mutate old state objects.

The "publish" and "validationFailed.publish" arrays must always be aligned. The "validateForm" function was erronously filtering out undefined entries in validationFailed.publish. Those undefined entries correspond to empty slots in the "publish" array and are left behind by the DynamicListForm component when removing rows in the UI. It does that for its own convenience and we must respect that. The observable result of filtering out these entries was that validation errors would shift to unrelated publish entries. If that unrelated entry was a empty slot, it would thereafter never be updated and keep its validation error forever, which would keep the "Create" button disabled forever.

mvollmer · 2024-10-17T10:35:17Z

src/PodCreateModal.jsx

-                delete prevState[key];
-            return prevState;
+            const newState = Object.assign({}, prevState, { [key]: value });
+            if (newState[key].every(a => a === undefined))


Yeah, I think this code never triggers, and wouldn't make any difference if it did.

mvollmer · 2024-10-17T11:45:21Z

Here is a step-by-step play-through of the bug re keeping publish and validationFailed aligned. The value shown come from JSON.stringify, so "undefined" and "empty slot" show as "null".

 - "Create pod"

   publish: []
   valididationFailed: {}

 - "Add port mapping"

    publish: [{"key":0,"IP":null,"containerPort":null,"hostPort":null,"protocol":"tcp"}]
    valididationFailed: {}

 - Trashcan

   publish: [null]
   valididationFailed: {}

 - "Add port mapping"

   publish: [null,{"key":1,"IP":null,"containerPort":null,"hostPort":null,"protocol":"tcp"}]
   valididationFailed: {}

 - Type "-3" into "Container port"

   publish: [null,{"key":1,"IP":null,"containerPort":"-3","hostPort":null,"protocol":"tcp"}]
   valididationFailed: {"publish":[null,{"containerPort":"1 to 65535"}]}

   This is correct, there is a "null" entry in validationFailed to
   correspond to the "null" entry in publish. The error shows up in
   the UI in the correct spot.

 - Trashcan

   publish: [null,null]
   valididationFailed: {}

 - "Add port mapping"

   publish: [null,null,{"key":2,"IP":null,"containerPort":null,"hostPort":null,"protocol":"tcp"}]
   valididationFailed: {}

 - "Create"

    publish: [null,null,{"key":2,"IP":null,"containerPort":null,"hostPort":null,"protocol":"tcp"}]
    valididationFailed: {"publish":[{"containerPort":"Container port must not be empty"}]}

    ^- This is the bug: validateForm() has removed the undefined
    entries from validationFailed and shifted the validation error
    into index 0. The error doesn not appear in the UI, and from now
    on, isFormInvalid will always say "true" and the "Create" button
    will stay disabled.

mvollmer · 2024-10-17T11:47:20Z

Since the tests don't actually remove ports in the dialog, I think all the flakiness was caused by the bug in dynamicListOnValidationChange.

mvollmer · 2024-10-17T12:47:55Z

Since the tests don't actually remove ports in the dialog, I think all the flakiness was caused by the bug in dynamicListOnValidationChange.

Oh, they do, sorry.

mvollmer · 2024-10-17T12:50:24Z

The failures on TF were cause by its relative slowness, I think: Validation functions run after 500ms, and when they deliver their result for a row that has since been removed, isFormInvalid will still look at them and disable the button. My theory is that in our own CI we are done with the whole dialog in less than 500ms, but TF is so slow that it gets hit with that bug.

They might show up when the port row is removed before the debounced validation has actually run.

That should work now.

cockpituous · 2024-10-17T14:34:49Z

src/PodCreateModal.jsx

-                delete prevState[key];
-            return prevState;
+            const newState = Object.assign({}, prevState, { [key]: value });
+            if (newState[key].every(a => a === undefined))


This added line is not executed by any test.

mvollmer · 2024-10-17T14:51:45Z

I guess I look at the TestApplication.testCreateContainerInPodUser flakes next.

jelly · 2024-10-17T17:39:33Z

src/PodCreateModal.jsx

+            return Object.values(row)
+                    .filter(val => val) // Filter out empty/undefined properties
+                    .length > 0; // If one field has error, the whole group (dynamicList) is invalid
+        }


This might also need to be fixed in CreateContainerModal.

martinpitt

Awesome, thanks! Even if there are still other races left, these are all good and important fixes, so let's get them in.

mvollmer added 2 commits October 17, 2024 13:16

pods: Don't mutate the state object in PodCreateModal

41ddefd

We need to construct a new one. Otherwise React will assume that the state hasn't actually changed. This might have worked by accident in the past, but React is clear that you should not mutate old state objects.

mvollmer requested review from martinpitt, jelly and tomasmatus October 17, 2024 10:29

mvollmer commented Oct 17, 2024

View reviewed changes

mvollmer added the no-test label Oct 17, 2024

mvollmer force-pushed the fix-pod-create-dialog-validation branch from 0b5f3c7 to 45a0fe4 Compare October 17, 2024 12:47

mvollmer removed the no-test label Oct 17, 2024

mvollmer force-pushed the fix-pod-create-dialog-validation branch from 45a0fe4 to 2b00fee Compare October 17, 2024 14:01

mvollmer added 2 commits October 17, 2024 17:03

pods: Always ignore validation errors for empty slots in PodCreateModal

8f541dd

They might show up when the port row is removed before the debounced validation has actually run.

test: Run pod creation tests also on Firefox

0f7cccb

That should work now.

mvollmer force-pushed the fix-pod-create-dialog-validation branch from 2b00fee to 0f7cccb Compare October 17, 2024 14:07

cockpituous reviewed Oct 17, 2024

View reviewed changes

mvollmer marked this pull request as ready for review October 17, 2024 14:51

jelly approved these changes Oct 17, 2024

View reviewed changes

martinpitt approved these changes Oct 18, 2024

View reviewed changes

martinpitt merged commit 51690a7 into cockpit-project:main Oct 18, 2024
33 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pods: Fix PodCreateModal validation issues #1884

pods: Fix PodCreateModal validation issues #1884

mvollmer commented Oct 17, 2024 •

edited

Loading

mvollmer Oct 17, 2024

mvollmer commented Oct 17, 2024

mvollmer commented Oct 17, 2024 •

edited

Loading

mvollmer commented Oct 17, 2024

mvollmer commented Oct 17, 2024

cockpituous Oct 17, 2024

mvollmer commented Oct 17, 2024

jelly Oct 17, 2024

martinpitt left a comment

pods: Fix PodCreateModal validation issues #1884

pods: Fix PodCreateModal validation issues #1884

Conversation

mvollmer commented Oct 17, 2024 • edited Loading

mvollmer Oct 17, 2024

Choose a reason for hiding this comment

mvollmer commented Oct 17, 2024

mvollmer commented Oct 17, 2024 • edited Loading

mvollmer commented Oct 17, 2024

mvollmer commented Oct 17, 2024

cockpituous Oct 17, 2024

Choose a reason for hiding this comment

mvollmer commented Oct 17, 2024

jelly Oct 17, 2024

Choose a reason for hiding this comment

martinpitt left a comment

Choose a reason for hiding this comment

mvollmer commented Oct 17, 2024 •

edited

Loading

mvollmer commented Oct 17, 2024 •

edited

Loading