shell: Even more types #21425

mvollmer · 2024-12-12T07:00:31Z

No description provided.

mvollmer · 2024-12-12T07:01:26Z

This time I tried out fully typing two largish files without any intermediate "relaxed" steps. It's fun but also quite intense.

mvollmer · 2024-12-12T08:17:05Z

pkg/shell/nav.tsx

@@ -366,6 +412,7 @@
            target: {
                host: current_machine.address,
                path: current_machine_manifest_items.items.apps.path,
+                hash: "/",


We could add a test for this. This is about the "Edit" button in the "Applications" section. It only appears when at least one applications is installed.

mvollmer · 2024-12-13T11:23:09Z

pkg/shell/util.tsx

@@ -91,6 +91,16 @@ export interface ManifestItem {
    keywords: ManifestKeyword[];
 }

+export interface ManifestParentSection {


Hmm... we could make some functions like manifest_parent_section(m: Manifest): ManifestParentSection which would ide all the "unsafe" JSON manipulations.

The whole Manifest stuff should go to its own file, actually.

Like, helpers which do type validation? what would they do on errors then?

This feels simultaneously too strict (hard to handle errors at that level, as these are effectively user-provided files) and too specific (we parse JSON everywhere, and should generalize the mechanics).

Are you aware of the bridge's jsonutil? That may be a more adequate way to parse JSON with strict typing, and the API would then also include default values.

There is big WIP commit now that implements one approach: validating all manifests as they are received from the bridge. It's quite a bit of code, and it totally wants to be autogenerated, but I think that's not worth it just yet. (Also, it's not enough code to justify dragging in some toolkit/framework for this, unless we find a really nice one.)

Addendum: the validation is pretty fine-grained: If you forget the label of a docs entry, only that one docs entry is omitted. If you use a string for the weight of a keyword instead of a number, you get an error message but the shell continues with a weight of 0.

martinpitt

Thanks! Nice that this found some bugs! Most of the comments are nice, bikeshedding, praise, and questions. But I have serious concerns about using as on externally read data, that feels just wrong and dangerous to me.

martinpitt · 2024-12-13T11:38:58Z

pkg/shell/util.tsx

@@ -91,6 +91,16 @@ export interface ManifestItem {
    keywords: ManifestKeyword[];
 }

+export interface ManifestParentSection {


Like, helpers which do type validation? what would they do on errors then?

This feels simultaneously too strict (hard to handle errors at that level, as these are effectively user-provided files) and too specific (we parse JSON everywhere, and should generalize the mechanics).

Are you aware of the bridge's jsonutil? That may be a more adequate way to parse JSON with strict typing, and the API would then also include default values.

pkg/shell/util.tsx

martinpitt · 2024-12-13T11:44:54Z

pkg/shell/util.tsx

+            const manifest_section = (manifest[section] || {}) as ManifestSection;
+            Object.entries(manifest_section).forEach(([prop, info]) => {


This cast is weird and IMHO detrimental. This isn't a case of "ts can't figure it out", it's literally "this could be anything as it's an user-provided (or at least user-customizable) external file on disk". So even after the as you can in no way be sure that you actually have a ManifestSection object, and the type checker will hide errors such as "component is actually an int". I.e. you'll get runtime errors because you forgot to check the type. That's the opposite of what typing should achieve.

That's why I think for this case something like json_get_object(manifest, section, {}) would be much cleaner, more robust, and also more general here. The original code was actually fine already, but the helper functions could produce proper console warnings that help admins to figure out why their manifest customizations are being ignored.

This cast is weird and IMHO detrimental.

Yes... it is keeping the status quo, but we should try to do better eventually, agreed.

I.e. you'll get runtime errors because you forgot to check the type. That's the opposite of what typing should achieve.

Hmm, I would call this input validation rather than static typing. Currently, we validate manifest input by just pretending it's all valid and let the shell crash when it isn't. (And we have been fine with that since forever.) Static typing makes this obvious, but by itself is not enough to improve it.

We would have to write new code to do real input validation. I was hoping there is a way to get TypeScript to do that. Here are some good pointers: https://stackoverflow.com/questions/33800497/check-if-an-object-implements-an-interface-at-runtime-with-typescript

Hmm, I would call this input validation rather than static typing

Yes, exactly. And input validation is a runtime thing, the type checker can't help with that. We either need some "JSON schema validation" thing, or just pick out the values individually with the expected type and default (which I personally favor -- it's explicit, tsc can help us get it right, and we don't reject manifests with wrong keys wholesale, which would be a behaviour changes).

martinpitt · 2024-12-13T11:46:52Z

pkg/shell/util.tsx

-            if (comp && comp.parent && comp.parent.component)
-                component = comp.parent.component as string;
+            if (comp && comp.parent) {
+                const parent = comp.parent as ManifestParentSection;
+                if (parent.component)
+                    component = parent.component;
+            }


In the same spirit as above, I think componennt = comp?.parent?.component would be both cleaner and more robust. Alternatively, wrap this into some json_get_object() helper with warning messages -- i.e. "nonexisting" is fine (default null) but "exists but wrong type" should warn.

So you are asking for "The Shell should validate the schema of all manifests". That's a good idea. We could have done that without TypeScript all these years, but we didn't. A bug in a manifests is like a bug in JavaScript, and should be found and fixed by the developer.

And while TypeScript might benefit from it once schema validation is done (think generating TS types from JSON schemas), I don't think TypeScript by itself is gonna help. All it does it point out our sins...

So, can I ask to keep the manifest schema validation yak out of this PR? :-)

So, can I ask to keep the manifest schema validation yak out of this PR?

I am fine with that. I am not fine with pretending that everything is good here and papering over the issue with as -- it really isn't, and tsc rightfully complains. So if it's too hard to fix these places, then let's add something greppable and obvious like @ts-expect-error. (Because as does have valid use cases, and isn't greppable)

Summary from meeting: My gut feeling/slight preference is the get_json_string() kind of accessor on usage, but I agree that for common keys we may rather want some more centralized "schema validation" thing, with a constructor. My minimum requirement here would be to add a // HACK: unsafe "as" or similar so that we can find these again.

There is now import_Manifests and import_ShellManifest that concentrate this issue in one place.

I still want to fully type our Manifest data structures and then access them from our code without any additional accessors. I.e., I want to do this:

interface Foo { name: string; } function bar(f: Foo) { console.log("Hi", f.name.toUpper()); } bar(import_Foo(get_some_json());

and not

function bar(f: JsonValue) { console.log("Hi", json_as_string(json_get(f, "name"), "<unknown>").toUpper()); } bar(get_some_json());

This needs a import_Foo function, which is not as trivial to write as the accessors, but I still want it for Christmas! :-)

pkg/shell/nav.tsx

martinpitt · 2024-12-13T12:05:38Z

pkg/shell/topnav.tsx

+    superuser_connection: cockpit.DBusClient | null = null;
+    superuser: cockpit.DBusProxy | null = null;
+
+    handleClickOutside = () => this.setState({ menuOpened: false, docsOpened: false });


Defining this outside of the ctor at the class level looks weird. What would this even be here? I suppose the this gets resolved at call time even for an arrow, but then wouldn't that be the this of the call environment instead of the TopNav instance?

The tests prove it works -- my eyes just trip over it. JS has too much magic!

Defining this outside of the ctor at the class level looks weird.

I guess... I moved it back into the constructor.

martinpitt · 2024-12-13T12:08:55Z

pkg/shell/topnav.tsx

-        if (this.state.osRelease.DOCUMENTATION_URL)
-            docItems.push(<DropdownItem key="os-doc" to={this.state.osRelease.DOCUMENTATION_URL} target="blank" rel="noopener noreferrer" icon={<ExternalLinkAltIcon />}>
+        if (this.state.osRelease?.DOCUMENTATION_URL)
+            docItems.push(<DropdownItem key="os-doc" to={this.state.osRelease.DOCUMENTATION_URL as string} target="blank" rel="noopener noreferrer" icon={<ExternalLinkAltIcon />}>


I could tolerate this, as runtime errors here aren't catastrophic. But DOC_URL is optional and can well be undefined (or someone could valalize their os-release to be an object or int). This really shouldn't become a pattern, something like json_get_str() would be much better here.

I have changed this to if (typeof ...DOC_URL == "string"), then the "as" is not necessary. Thanks for pointing these out! I will be much more careful with "as" in the future.

martinpitt · 2024-12-13T12:10:12Z

pkg/shell/topnav.tsx

                {cockpit.format(_("$0 documentation"), this.state.osRelease.NAME)}
            </DropdownItem>);

+        const shell_manifest = (cockpit.manifests.shell || {}) as ShellManifest;


This is the same "unvalidated user-provided document" issue as above. We cannot claim that this is a valid ShellManifest. The typechecker will then falsely claim that our code is correct, while it is missing runtime type checks.

This has been de-assed with import_ShellManifest.

martinpitt · 2024-12-13T12:12:09Z

pkg/shell/topnav.tsx

+                                        this.setState((prevState: TopNavState) => {
+                                            return { docsOpened: !prevState.docsOpened };
+                                        });


btw, => ({ key: value }) is a nicer way to return an object in an arrow. Avoids the extra { return }; wrapper.

Yeah, subjective! :-) I am always amused that people complain about all the parentheses in Lisp and then produce code with )})}}};}}; in it. :-)

martinpitt · 2024-12-13T12:14:31Z

pkg/shell/shell-modals.tsx

-    const manifest = cockpit.manifests.shell || { };
+    const manifest = (cockpit.manifests.shell || { }) as ShellManifest;


pkg/shell/manifests.ts

mvollmer · 2024-12-17T09:10:41Z

pkg/shell/util.tsx

@@ -202,11 +204,11 @@ export class CompiledComponents {
    }
 }

-export function compile_manifests(manifests?: Manifests): CompiledComponents {
+export function compile_manifests(manifests?: { [pkg: string]: Manifest }): CompiledComponents {


This is a leftover and should be removed. Manifests is still a real type.

mvollmer · 2024-12-17T10:19:58Z

About the validation stuff: I think we shouldn't do it in machines.js, but rather in CompiledComponents. And ShellState should keep a CompiledComponents per machine that is only updated when the machine state changes. That way we don't do validation on each shell state change.
But I let this PR rest for a while now.

martinpitt

Thanks! I'm happy with this direction now. I leave it up to you if you want to land the stuff until the WIP and do that separately, or work it out all together here. I still have a few comments, but just thinking aloud/Kleinkram.

pkg/shell/manifests.ts

martinpitt · 2024-12-17T10:26:54Z

pkg/shell/nav.tsx

+            } else if (cur instanceof HTMLElement) {
                cur.click();
            }


For our own sanity, this could have an else console.error() so that tests fail if we forget something here.

pkg/shell/nav.tsx

martinpitt · 2024-12-17T10:31:21Z

pkg/shell/state.tsx

+// NOTE - this is defined in pkg/lib/notifications.js and should be
+// imported from there once that file has been typed.
+//
+export interface PageStatus {


OOI -- could we just create a pkg/lib/notifications.d.ts and put that there, as an intermediate step? Or would that require moving more chairs around the deck?

(Not blocking or a veto, just learning)

I think we could!

Hmm, let's port it to TS right away, separately.

pkg/shell/nav.tsx

martinpitt · 2024-12-17T10:40:42Z

pkg/shell/manifests.ts

+function import_string(val: JsonValue, fallback?: string): string {
+    if (typeof val == "string")
+        return val;
+    validation_error(`Not a string: ${JSON.stringify(val)}`);
+    if (fallback !== undefined)
+        return fallback;
+    throw new ValidationError();


This is very close to what I had in mind, except that I'd handle "absent" (undefined) differently from "wrong type" -- after all, almost every field in a manifest is optional, and we shouldn't spew console errors for these.

(and maybe fallback → default)

Yes, there is some dicussion to be had. My idea here is that default values are not handled at all during validation. They are provided by the code that works with these data structures. The fallbacks here are only for the case were a value is of the wrong type, but we want to hobble along anyway.

The only fallbacks in use are:

something is supposed to be an array but is something else, then we continue with an empty array (after emitting the error)

something is supposed to be a dictionary but is something else, then we continue with an empty dict (after emitting an error)

when the "weight" of a keyword or the order of a menu entry is not a number, then we use 0 (after emitting an error)

when the "translate" of a keyword is not a boolean, then we continue with "false" (after emitting an error)

I think we could remove the last two cases actually. Using { weight: "heavy" } as a keyword should probably disqualify (and skip) the whole entry.

Let's make a separate PR for this.

We should think long and hard about how callers want to use it -- i.e. if a manifest override says e.g. preload: 1 or doesn't specify preload, then it seems to me the only difference should be that the former causes a console warning. But in both cases the caller (specifying a reasonably default such as [] or null) would get that default in both cases?

In other words, having two fallbacks (one for "wrong type" and another one for "absent") feels like accidental complexity and prone to confusion to me, WDYT?

Yes, I agree.

pkg/shell/manifests.ts

The router will always use a string when jumping, and all functions are actually happy with a partial Location as input. This let's us remove a "as" type cast.

Collect all definitions into one place and prepare for proper validation.

The cockpit.spawn function takes only two arguments (unlike cockpit.script). Location.reload() doesn't take any arguments, the "force_reload" argument is a Firefox extension. There is no "this.onSelect" in CockpitNav.

martinpitt

Thanks! This seems to break TestMultiMachineKeyAuth.testBasic, but otherwise LGTM now! (with lots of possible follow-ups..)

mvollmer · 2024-12-18T08:54:24Z

Thanks! This seems to break TestMultiMachineKeyAuth.testBasic, but otherwise LGTM now! (with lots of possible follow-ups..)

I think this is actually another case of the slow opening animation of the host switcher clashing with the real mouse clicks. It might be time to share open_host_switcher between the tests.

cockpituous · 2024-12-18T10:04:10Z

pkg/shell/nav.tsx

                cur.click();
+            } else {
+                console.error("Active element not a HTMLElement");


This added line is not executed by any test.

cockpituous · 2024-12-18T10:04:11Z

pkg/shell/nav.tsx

                if (begin < 0)
                    begin = all.length - 1;
                all[begin].focus();
            } else {
                let i = all.findIndex(item => item === cur);
                i += step;
                if (i < 0 || i >= all.length)
-                    document.querySelector("#" + sel + " .pf-v5-c-text-input-group__text-input").focus();
+                    document.querySelector<HTMLElement>("#" + sel + " .pf-v5-c-text-input-group__text-input")?.focus();


This added line is not executed by any test.

cockpituous · 2024-12-18T10:04:11Z

pkg/shell/nav.tsx

+                                            if (g.action)
+                                                this.props.jump(g.action.target);


These 2 added lines are not executed by any test.

cockpituous · 2024-12-18T10:04:11Z

pkg/shell/shell-modals.tsx

    }

-    const manifest = cockpit.manifests.shell || { };
+    const manifest = import_ShellManifest(cockpit.manifests.shell || { });


This added line is not executed by any test.

cockpituous · 2024-12-18T10:04:11Z

pkg/shell/shell-modals.tsx

@@ -140,8 +144,9 @@
                        <MenuList>
                            {
                                (() => {
-                                    const filteredLocales = Object.keys(manifest.locales || {})
-                                            .filter(key => !searchInput || manifest.locales[key].toLowerCase().includes(searchInput.toString().toLowerCase()));
+                                    const locales = manifest.locales || {};


This added line is not executed by any test.

cockpituous · 2024-12-18T10:04:11Z

pkg/shell/topnav.tsx

            docItems.push(<DropdownItem key="os-doc" to={this.state.osRelease.DOCUMENTATION_URL} target="blank" rel="noopener noreferrer" icon={<ExternalLinkAltIcon />}>
                {cockpit.format(_("$0 documentation"), this.state.osRelease.NAME)}
            </DropdownItem>);

+        const shell_manifest = import_ShellManifest(cockpit.manifests.shell || {});


This added line is not executed by any test.

cockpituous · 2024-12-18T10:04:11Z

pkg/shell/topnav.tsx

        // global documentation for cockpit as a whole
-        (cockpit.manifests.shell?.docs ?? []).forEach(doc => {
+        (shell_manifest.docs ?? []).forEach(doc => {


This added line is not executed by any test.

martinpitt

Ah, thanks! I'm happy to refactor the helper into testlib.py after this.

mvollmer commented Dec 12, 2024

View reviewed changes

mvollmer marked this pull request as ready for review December 12, 2024 13:14

mvollmer mentioned this pull request Dec 12, 2024

shell: More types than ever #21426

Draft

1 task

mvollmer commented Dec 13, 2024

View reviewed changes

martinpitt requested changes Dec 13, 2024

View reviewed changes

mvollmer force-pushed the shell-typing-3 branch from 35a2d1e to baa20a0 Compare December 16, 2024 13:51

github-advanced-security bot found potential problems Dec 16, 2024

View reviewed changes

pkg/shell/manifests.ts Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Dec 16, 2024

View reviewed changes

pkg/shell/manifests.ts Fixed Show fixed Hide fixed

pkg/shell/manifests.ts Fixed Show fixed Hide fixed

mvollmer force-pushed the shell-typing-3 branch from 5dce77b to 5890f70 Compare December 16, 2024 16:59

mvollmer marked this pull request as draft December 16, 2024 17:05

mvollmer requested a review from martinpitt December 16, 2024 17:16

mvollmer force-pushed the shell-typing-3 branch from 5890f70 to 284b65b Compare December 17, 2024 08:44

mvollmer commented Dec 17, 2024

View reviewed changes

mvollmer added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Dec 17, 2024

mvollmer force-pushed the shell-typing-3 branch from 284b65b to 2ffbb98 Compare December 17, 2024 09:17

martinpitt reviewed Dec 17, 2024

View reviewed changes

shell: Improve Location type a bit

be7f518

The router will always use a string when jumping, and all functions are actually happy with a partial Location as input. This let's us remove a "as" type cast.

mvollmer removed the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Dec 17, 2024

mvollmer force-pushed the shell-typing-3 branch from 2ffbb98 to 4eb85af Compare December 17, 2024 11:32

mvollmer marked this pull request as ready for review December 17, 2024 11:32

mvollmer requested a review from martinpitt December 17, 2024 11:32

mvollmer added 5 commits December 17, 2024 14:33

shell: Improve manifest types

3fd766d

Collect all definitions into one place and prepare for proper validation.

shell: Fully type "nav"

edf067a

shell: Fully type "topnav"

ae879ef

shell: Check types in "shell-modals"

7e96a90

shell: Fix bugs found by TypeScript

9c19728

The cockpit.spawn function takes only two arguments (unlike cockpit.script). Location.reload() doesn't take any arguments, the "force_reload" argument is a Firefox extension. There is no "this.onSelect" in CockpitNav.

mvollmer force-pushed the shell-typing-3 branch from 4eb85af to 9c19728 Compare December 17, 2024 12:33

martinpitt reviewed Dec 17, 2024

View reviewed changes

test: Robustify opening the host switcher in more places

a14ba82

cockpituous reviewed Dec 18, 2024

View reviewed changes

mvollmer requested a review from martinpitt December 18, 2024 12:51

mvollmer mentioned this pull request Dec 18, 2024

shell: Manifest validation upon import #21449

Draft

1 task

martinpitt approved these changes Dec 18, 2024

View reviewed changes

martinpitt merged commit 71d6efb into cockpit-project:main Dec 18, 2024
85 checks passed

		const manifest_section = (manifest[section] \|\| {}) as ManifestSection;
		Object.entries(manifest_section).forEach(([prop, info]) => {

		const manifest = cockpit.manifests.shell \|\| { };
		const manifest = (cockpit.manifests.shell \|\| { }) as ShellManifest;

shell: Even more types #21425

shell: Even more types #21425

Conversation

mvollmer commented Dec 12, 2024

mvollmer commented Dec 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvollmer commented Dec 17, 2024 • edited Loading

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpitt left a comment

Choose a reason for hiding this comment

mvollmer commented Dec 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpitt left a comment

Choose a reason for hiding this comment

mvollmer commented Dec 17, 2024 •

edited

Loading