Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use fsinfo for cockpit.file.watch() #20017

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 16 additions & 0 deletions pkg/base1/test-file.js
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,22 @@ QUnit.test("watching without reading", assert => {
}, { read: false });
});

QUnit.test("watching without reading pre-created", async assert => {
const done = assert.async();
assert.expect(3);

// Pre-create fsinfo test file
const file = cockpit.file(dir + "/fsinfo");
await file.replace("1234");
const watch = file.watch((content, tag) => {
assert.equal(content, null, "non-existant because read is false");
assert.notEqual(tag, null, "non empty tag");
assert.equal(tag.startsWith("1:"), true, "tag always stars with 1:");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

starts ⭐

watch.remove();
done();
}, { read: false });
});

QUnit.test("watching directory", assert => {
const done = assert.async();
assert.expect(20);
Expand Down
137 changes: 137 additions & 0 deletions pkg/lib/cockpit-fsinfo.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
"use strict";

function is_json_dict(value: cockpit.JsonValue): value is cockpit.JsonObject {
martinpitt marked this conversation as resolved.
Show resolved Hide resolved
return value?.constructor === Object;
}

/* RFC 7396 — JSON Merge Patch — functional */
function json_merge(current: cockpit.JsonValue, patch: cockpit.JsonValue): cockpit.JsonValue {
if (is_json_dict(patch)) {
const updated = is_json_dict(current) ? { ...current } : { };

for (const [key, value] of Object.entries(patch)) {
if (value === null) {
delete updated[key];
} else {
updated[key] = json_merge(updated[key], value);
}
}

return updated;
} else {
return patch;
}
}

interface FileInfo {
type?: string;
tag?: string;
mode?: number;
size?: number;
uid?: number;
user?: string | number;
gid?: number;
group?: string | number;
mtime?: number;
content?: string;
target?: string;
entries?: {
[filename: string]: FileInfo;
};
targets?: {
[filename: string]: FileInfo;
};
}

interface FsInfoError {
problem?: string;
message?: string;
errno?: string;
}

interface FileInfoState {
info: FileInfo | null;
error: FsInfoError | null;
}

interface FsInfoHandle {
close(): void;
effect(callback: ((state: FileInfoState) => void)): void;
entry(name: string): FileInfo | null;
state: FileInfoState;
target(name: string): FileInfo | null;
}

export function fsinfo(path: string, attrs: string[], options?: cockpit.JsonObject) {
const self: FsInfoHandle = {
close,
effect,
entry,
state: {
info: null,
error: null,
},
target,
};

const callbacks: ((state: FileInfoState) => void)[] = [];

function close() {
channel.close();
}

function effect(callback: (state: FileInfoState) => void) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I won't fight very hard for this, but at least a little: Let's please not call this effect(), it's super confusing as an API. The standard JS approach is to make this an EventTarget and attach a changed or similar event handler to it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My 2¢:

  • I hate the event mixin API
  • we have a lot of other areas in cockpit.js which implement their own callback list mechanism. We even have helper functions for it.
  • The name effect() is definitely suspect, as is the very react-flavoured API (returning the unregister callable).
  • Maybe we should change it anyway, just for uniformity with everything else in cockpit.js...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have a lot of other areas in cockpit.js which implement their own callback list mechanism

Yeah, but I wouldn't exactly call cockpit.js a role model for good/modern JS. It is really bad from today's POV, it was created pre-ES5 even, with jquery.

Copy link
Member Author

@jelly jelly Apr 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this seems to be the only real blocker? I am happy to rewrite .effect into something else the question is.. what is a Modern JS API :)

(I am kinda inclined to make it like cockpit.js)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But should we then just move this into cockpit? The issue is kinda how we then expose fsinfo, cockpit.file.info?

callback(self.state);
callbacks.push(callback);
return () => callbacks.splice(callbacks.indexOf(callback), 1);
}

function entry(name: string): FileInfo | null {
return self.state.info?.entries?.[name] ?? null;
}

function target(name: string): FileInfo | null {
const entries = self.state.info?.entries ?? {};
const targets = self.state.info?.targets ?? {};

let entry = entries[name] ?? null;
for (let i = 0; i < 40; i++) {
const target = entry?.target;
if (!target)
return entry;
entry = entries[target] ?? targets[target] ?? null;
}
return null; // ELOOP
}

const channel = cockpit.channel({
superuser: "try",
payload: "fsinfo",
path,
attrs,
watch: true,
...options
});

let state: cockpit.JsonValue = null;
channel.addEventListener("message", (_event: CustomEvent, payload: string) => {
state = json_merge(state, JSON.parse(payload));

if (is_json_dict(state) && !state.partial) {
self.state = {
info: is_json_dict(state.info) ? state.info : null,
error: is_json_dict(state.error) ? state.error : null,
};

for (const callback of callbacks) {
callback(self.state);
}
}
});

return self;
}

// FIXME: import at the end of the file to prevent circular import build issues
// this is a temporary measure until we move cockpit.js to cockpit.ts in a follow up.
import cockpit from "./cockpit.js";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh. That's unusual.

Please explain. :)

If having this at the end of the file is the full extent of your "ugly hack" then I could probably be convinced to live with it... but how does it work?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Soo I looked up some examples of folks trying to get around circular imports and it seems that loaders/import logic ignores imports at the end as cyclic. Possibly as the bundler first combines everything into one file? Either way this is a hack and we should resolve it.

I just didn't feel like doing a big bang rename of cockpit.js to cockpit.ts and merging the typing navigator? Let's discuss how we intend to integrate this all.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the record: we can rename pkg/lib/cockpit.js to cockpit.ts with absolutely no other changes and nothing bad seems to happen.

Then we could just add this there...

Note: one thing that I didn't check is to do the rename and then to try to use cockpit from a TS-using project (like navigator) which carries its own cockpit.d.ts file. I wonder if that would make tsc get angry...

martinpitt marked this conversation as resolved.
Show resolved Hide resolved
40 changes: 20 additions & 20 deletions pkg/lib/cockpit.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@

/* eslint-disable indent,no-empty */

import { fsinfo } from "./cockpit-fsinfo";

let url_root;

const meta_url_root = document.head.querySelector("meta[name='url-root']");
Expand Down Expand Up @@ -3614,24 +3616,23 @@ function factory() {
if (watch_channel)
return;

const opts = {
payload: "fswatch1",
path,
superuser: base_channel_options.superuser,
};
watch_channel = cockpit.channel(opts);
watch_channel.addEventListener("message", function (event, message_string) {
let message;
try {
message = JSON.parse(message_string);
} catch (e) {
message = null;
}
if (message && message.path == path && message.tag && message.tag != watch_tag) {
if (options && options.read !== undefined && !options.read)
fire_watch_callbacks(null, message.tag);
else
read();
watch_channel = fsinfo(path, ["tag"], { superuser: base_channel_options.superuser });
watch_channel.effect(state => {
if (state.error || state.info) {
// Behave like fsread1, not-found is not a fatal error
if (state.error && state.error?.problem !== "not-found") {
const error = new BasicError(state.error.problem, state.error.message);
fire_watch_callbacks(null, "-", error);
} else {
const tag = state?.info?.tag || "-";
if (tag !== watch_tag) {
if (tag === "-" || options?.read === false) {
fire_watch_callbacks(null, tag);
} else {
read();
}
}
}
}
});
} else {
Expand All @@ -3654,7 +3655,6 @@ function factory() {
ensure_watch_channel(options);

watch_tag = null;
read();

return {
remove: function () {
Expand All @@ -3675,7 +3675,7 @@ function factory() {
if (replace_channel)
replace_channel.close("cancelled");
if (watch_channel)
watch_channel.close("cancelled");
watch_channel.close();
}

return self;
Expand Down
2 changes: 1 addition & 1 deletion test/verify/check-users
Original file line number Diff line number Diff line change
Expand Up @@ -1046,7 +1046,7 @@ class TestAccounts(testlib.MachineCase):
b.wait_not_present("#password-expiration")

b.logout()
self.login_and_go("/users", user="scruffy")
self.login_and_go("/users", user="scruffy", superuser=False)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a separate commit with an explanation. It took you so long to figure out, let's please explain it in the commit message for our future selves 😁

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this change -at all-, and I absolutely veto it. It might be right for this test, but we're doing this to cover up a bug. At the same time, I agree that it's not directly related to this particular test scenario.

My proposal: in the same commit that "fixes" this testcase, we introduce a separate test case which tests this. I'm happy to have it marked as an xfail at first with a link to #20263 and (maybe or maybe not) #20262.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to add an expected failing test, I assume you mean integration not pytest?

b.go("#/scruffy")
b.wait_text("#account-user-name", "scruffy")
b.wait_text("#account-expiration-text", "Never expire account")
Expand Down