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

shell: Manifest validation upon import #21449

Open
wants to merge 2 commits 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions files.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ const info = {
"base1/test-types.ts",
"base1/test-user.js",
"base1/test-websocket.js",
"base1/test-import-json.ts",

"kdump/test-config-client.js",

Expand Down
264 changes: 264 additions & 0 deletions pkg/base1/test-import-json.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,264 @@
/*
* This file is part of Cockpit.
*
* Copyright (C) 2024 Red Hat, Inc.
*
* Cockpit is free software; you can redistribute it and/or modify it
* under the terms of the GNU Lesser General Public License as published by
* the Free Software Foundation; either version 2.1 of the License, or
* (at your option) any later version.
*
* Cockpit is distributed in the hope that it will be useful, but
* WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with Cockpit; If not, see <https://www.gnu.org/licenses/>.
*/

import { JsonValue } from 'cockpit';
import {
import_json_object, import_optional, import_mandatory,
Copy link
Member

Choose a reason for hiding this comment

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

get_() would be more consistent with our Python API; "import" sounds a bit exaggerated, but no strong opinion.

import_string, import_number, import_boolean,
import_array, import_record,
validate
} from "import-json";

import QUnit from 'qunit-tests';

// Hook into console.error

let console_errors: string[] = [];
const console_error = console.error;
console.error = function (msg) {
console_errors.push(msg);
console_error(msg);
};

QUnit.hooks.beforeEach(() => {
console_errors = [];
});

QUnit.test("import_string", function(assert) {
assert.equal(import_string("foo"), "foo", "string");
assert.throws(() => import_string(12), "not a string");
assert.deepEqual(
console_errors,
[
'JSON validation error for : Not a string: 12'
],
"console errors"
);
});

QUnit.test("import_number", function(assert) {
assert.equal(import_number(12), 12, "number");
assert.throws(() => import_number("foo"), "not a number");
assert.deepEqual(
console_errors,
[
'JSON validation error for : Not a number: "foo"'
],
"console errors"
);
});

QUnit.test("import_boolean", function(assert) {
assert.equal(import_boolean(true), true, "boolean");
assert.throws(() => import_boolean("foo"), "not a boolean");
assert.deepEqual(
console_errors,
[
'JSON validation error for : Not a boolean: "foo"'
],
"console errors"
);
});

QUnit.test("import_array", function(assert) {
assert.deepEqual(import_array(["a", "b", "c"], import_string), ["a", "b", "c"], "array of strings");
assert.deepEqual(import_array([1, 2, 3], import_number), [1, 2, 3], "array of numbers");
assert.deepEqual(import_array([1, 2, "c"], import_number), [1, 2], "array of numbers with a string");
Copy link
Member

Choose a reason for hiding this comment

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

IMHO this should also throw, instead of giving you a truncated array -- that just silently hides errors, and may make the rest of the entries invalid as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is the big philosophical question. Personally, I like the non-strict approach, but I have no problem with making this strict.

My best argument for making this strict is to amplify that something is wrong. If you make a mistake with your search terms, your whole page doesn't get loaded instead of some search terms being omitted.

assert.throws(() => import_array("foo", import_string), "not an array");
assert.deepEqual(
console_errors,
[
'JSON validation error for [2]: Not a number: "c"',
'JSON validation error for : Not an array: "foo"'
],
"console errors"
);
});

QUnit.test("import_record", function(assert) {
assert.deepEqual(
import_record({ a: "a", b: "b", c: "c" }, import_string),
Copy link
Member

Choose a reason for hiding this comment

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

JSON parlance is "object", not "record". I suppose you want to give this a different name to point out that all values have to be of the same type. But a "record" (or "struct") is precisely the opposite -- fixed number and names of keys, and different values.

One term that comes to my mind that conveys this meaning is "dictionary" (they usually map one type to a single value type).

Copy link
Member Author

Choose a reason for hiding this comment

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

The "record" term is from the TypeScript utility class Record<Keys, Type>, which is indeed a Map or Dict, not a structure.

Copy link
Member

Choose a reason for hiding this comment

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

Ugh -- this is opposite to pretty much everywhere else https://en.wikipedia.org/wiki/Record_(computer_science) -- shame on you, MS.

{ a: "a", b: "b", c: "c" },
"record of strings");
assert.deepEqual(
import_record({ a: 1, b: 2, c: 3 }, import_number),
{ a: 1, b: 2, c: 3 },
"record of numbers");
assert.deepEqual(
import_record({ a: 1, b: 2, c: "c" }, import_number),
{ a: 1, b: 2 },
"record of numbers with a string");
Comment on lines +103 to +106
Copy link
Member

Choose a reason for hiding this comment

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

Similar to the inconsistent array above, IMHO this should throw and ignore the value in its entirety.

assert.throws(() => import_record("foo", import_string), "not a record");
assert.deepEqual(
console_errors,
[
'JSON validation error for .c: Not a number: "c"',
'JSON validation error for : Not an object: "foo"'
],
"console errors"
);
});

QUnit.test("validate", function(assert) {
assert.equal(validate("test input", "foo", import_string, ""), "foo", "string");
assert.equal(validate("test input", 12, import_string, ""), "", "not a string");
Comment on lines +119 to +120
Copy link
Member

Choose a reason for hiding this comment

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

Could you do "" → "default", to make it more obvious what that does?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes.

assert.deepEqual(
console_errors,
[
'JSON validation error for test input: Not a string: 12'
],
"console errors"
);
});

interface Player {
name: string;
age?: number;
}

function import_Player(val: JsonValue): Player {
const obj = import_json_object(val);
const res: Player = {
name: import_mandatory(obj, "name", import_string),
};
import_optional(res, obj, "age", import_number);
return res;
}
Comment on lines +130 to +142
Copy link
Member

Choose a reason for hiding this comment

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

This is quite pleasant to read and cleanly and robustly typed. Neat! :clap


interface Team {
name: string;
players: Player[];
}

function import_Team(val: JsonValue): Team {
const obj = import_json_object(val);
const res: Team = {
name: import_mandatory(obj, "name", import_string),
players: import_mandatory(obj, "players", v => import_array(v, import_Player))
};
return res;
}

interface Teams {
[name: string]: Team;
}

function import_Teams(val: JsonValue): Teams {
return import_record(val, import_Team);
}

QUnit.test("objects", function(assert) {
const checks: [JsonValue, Teams][] = [
[
{
MAC: {
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I just have to ask: What does "MAC" mean in soccer context?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's supposed to be the short form of "ManCity"...

name: "ManCity",
players: [
{ name: "Haaland", age: 24 },
{ name: "De Bruyne", age: 33 }
],
stadium: "City of Manchester Stadium"
},
},
{
MAC: {
name: "ManCity",
players: [
{ name: "Haaland", age: 24 },
{ name: "De Bruyne", age: 33 }
],
},
Copy link
Member

Choose a reason for hiding this comment

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

Can you please add a description comment before each scenario? Like, this one is probably "No stadium (it's optional)" or so

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep.

}

],
[
{
MAC: {
name: "ManCity",
players: [
{ name: "Haaland", age: "unknown" },
{ name: "De Bruyne", age: 33 }
],
stadium: "City of Manchester Stadium"
},
},
{
MAC: {
name: "ManCity",
players: [
{ name: "Haaland" },
{ name: "De Bruyne", age: 33 }
],
},
}

],
[
{
MAC: {
name: "ManCity",
players: [
{ name: ["Erling", "Braut", "Haaland"] },
{ name: "De Bruyne", age: 33 }
],
stadium: "City of Manchester Stadium"
},
},
{
MAC: {
name: "ManCity",
players: [
{ name: "De Bruyne", age: 33 }
],
},
}
],
[
{
MAC: {
name: "ManCity",
players: "TBD",
stadium: "City of Manchester Stadium"
},
},
{}
],
[
"...",
{}
]
];

for (let i = 0; i < checks.length; i++) {
assert.deepEqual(validate("test input", checks[i][0], import_Teams, {}), checks[i][1]);
}

assert.deepEqual(
console_errors,
[
'JSON validation error for test input.MAC.players[0].age: Not a number: "unknown"',
'JSON validation error for test input.MAC.players[0].name: Not a string: ["Erling","Braut","Haaland"]',
'JSON validation error for test input.MAC.players: Not an array: "TBD"',
'JSON validation error for test input: Not an object: "..."'
],
"console errors"
);
});

QUnit.start();
Loading
Loading