Add JSON schema for top-level config file + use for validation #489

alexdewar · 2024-08-13T13:46:49Z

This PR adds a JSON schema for top-level config files and changes the code to use it for validation. In contrast to the existing schema for index.json, I've split the schema into lots of small subschemas (see #452) for readability and maintainability and I've set additionalProperties to false everywhere (see #486).

There are a couple of small changes that will be needed to existing top-level config files in order for them to work with this new code:

The schema URL needs to be specified ("$ref": "https://raw.githubusercontent.com/imperialCHEPI/healthgps/main/schemas/v1/config.json")
The version property should be dropped

Other than that, it should work as before. I've tested that all the config files in the examples repo work (or, rather, they will after we merge imperialCHEPI/healthgps-examples#13), but it is possible that the schema is overly restrictive (e.g. a given value can be null, but we aren't allowing for this).

I also had to refactor the C++ code for working with schemas to make it more generic, but it essentially does what it did before.

Closes #449.

@jzhu20 FYI. If you send me the config files you're using at present I can check they work with the new schema (hopefully they will!).

I've dropped the version property as it's redundant now that we have the $schema property.

Closes #449.

github-actions

clang-tidy made some suggestions

src/HealthGPS.Input/configuration.cpp

github-actions · 2024-08-13T13:53:15Z

src/HealthGPS.Input/configuration.cpp

@@ -35,6 +36,9 @@
 namespace {
 using namespace hgps::input;

+static constexpr const char *ConfigSchemaFileName = "config.json";
+static constexpr int ConfigSchemaVersion = 1;


warning: 'ConfigSchemaVersion' is a static definition in anonymous namespace; static is redundant here [readability-static-definition-in-anonymous-namespace]

Suggested change

static constexpr int ConfigSchemaVersion = 1;

constexpr int ConfigSchemaVersion = 1;

src/HealthGPS.Input/datamanager.cpp

src/HealthGPS.Input/schema.cpp

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions

clang-tidy made some suggestions

src/HealthGPS.Input/configuration.cpp

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

TinyMarsh

Cool, just a couple of questions.

TinyMarsh · 2024-08-15T14:29:49Z

schemas/v1/config/interventions.json

+ },
+ "coverage_cutoff_time": {
+ "type": "integer",
+ "minValue": 0


I don't really know, but it seems the keyword for setting a minimum value is minimum, rather than minValue. See here:
https://json-schema.org/understanding-json-schema/reference/numeric#range

Good spot! I'm not sure how I managed that 🤦. We also have a pre-commit hook to check the schemas are valid, but it seems you can add extra properties and it won't complain 🤷

TinyMarsh · 2024-08-15T14:29:58Z

schemas/v1/config/interventions.json

+ },
+ "child_cutoff_age": {
+ "type": "integer",
+ "minValue": 0


TinyMarsh · 2024-08-15T14:34:21Z

schemas/v1/config/modelling.json

+ "function_parameters": {
+ "type": "array",
+ "items": {
+ "type": "number"
+ }


Just checking this array doesn't require a minimum number of values? Same applies for other arrays in this schema I guess.

I'm not sure... @jamesturner246?

I think these depend on the SES model -- e.g. normal has mean, std. Maybe there are other models that have none? Never uses the SES module.

Hmm... ok. I'll leave as is for now. We can always add extra constraints in future if needed.

TinyMarsh · 2024-08-15T14:37:35Z

schemas/v1/config/modelling.json

Do any of the string types here require a minLength to ensure they are not empty?

That's a good idea. I don't know whether I can be bothered to put minLengths all over the place though, but maybe I'm just being lazy 😆.

What do you think @jamesturner246? How pedantic do we want to be?

I won't push you -- one would hope users know not to put empty string fields... Certainly won't hurt though.

alexdewar · 2024-08-15T16:07:14Z

Thanks for going through this @TinyMarsh. I know it's probably not much fun just reviewing a bunch of JSON 😆

jamesturner246

LGTM!

I won't push to add extra validation like string length checking now if it's tons of work. This looks like the bulk of it. And some of the arrays like SES module I don't even know how long they are supposed to be. We can look at it later if problems occur.

jamesturner246 · 2024-08-16T13:49:35Z

src/HealthGPS.Input/schema.cpp

+ // **YUCK**: We have to read in the data with jsoncons here rather than reusing the
+ // nlohmann-json representation :-(


Would there be any benefit to switching over to another JSON lib, e.g. jsoncons in future? I saw an issue opened by someone: #173 (comment). No need to worry now.

I'm not sure. nlohmann-json is kind of the standard one, but it seems like jsoncons has a very similar interface. It bugs me that we're using two JSON libraries, but I fear it might involve a fair amount of refactoring to swap everything over to jsoncons.

alexdewar · 2024-08-27T10:26:50Z

It seems that you can provide relative paths to schemas more tersely, so I've fixed that.

alexdewar · 2024-08-27T15:10:38Z

After our discussion in the meeting, I think I'll change this so that it's fully backwards compatible with existing config files, just to make everyone's life easier. Shouldn't be hard to do.

We still warn if it is missing.

github-actions

clang-tidy made some suggestions

github-actions · 2024-08-27T15:47:48Z

src/HealthGPS.Input/schema.cpp

+ throw std::runtime_error(message);
+ } else {
+ fmt::print(fmt::fg(fmt::color::dark_salmon), "{}\n", message);
+ }


warning: do not use 'else' after 'throw' [readability-else-after-return]

Suggested change

}

} fmt::print(fmt::fg(fmt::color::dark_salmon), "{}\n", message);

github-actions

clang-tidy made some suggestions

src/HealthGPS.Input/schema.cpp

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

for more information, see https://pre-commit.ci

github-actions

clang-tidy made some suggestions

github-actions · 2024-08-27T16:01:49Z

src/HealthGPS.Input/schema.cpp

+ throw std::runtime_error(message);
+ } else {
+ fmt::print(fmt::fg(fmt::color::dark_salmon), "{}\n", message);
+ }


warning: do not use 'else' after 'throw' [readability-else-after-return]

Suggested change

}

} fmt::print(fmt::fg(fmt::color::dark_salmon), "{}\n", message);

fmt::print(fmt::fg(fmt::color::dark_salmon), "{}\n", message);

alexdewar added 7 commits August 13, 2024 14:35

Add JSON schema for main config files

b2d3e74

I've dropped the version property as it's redundant now that we have the $schema property.

Split config schema into subschemas

072c5f1

We don't need to include the base URL in $refs

7b8fc9f

Add a subschema for age ranges and use in several places

c09ccee

Split interventions into own subschema

dde3892

Make JSON validation code generic

89b8833

Use JSON schema to validate top-level config files

714f6e1

Closes #449.

alexdewar requested review from jamesturner246 and TinyMarsh August 13, 2024 13:46

github-actions bot reviewed Aug 13, 2024

View reviewed changes

Remove unnecessary statics

64f5249

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions bot reviewed Aug 13, 2024

View reviewed changes

src/HealthGPS.Input/configuration.cpp Outdated Show resolved Hide resolved

Remove another static

f2e849a

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

TinyMarsh approved these changes Aug 15, 2024

View reviewed changes

Fix: minValue should be minimum

4fb3a96

jamesturner246 approved these changes Aug 16, 2024

View reviewed changes

Use relative paths in schemas

ca5d13d

alexdewar and others added 4 commits August 27, 2024 16:40

load_and_validate_json: Allow for parsing files without $schema property

2f23fa7

We still warn if it is missing.

Make $schema property optional for now

2c6e30a

Make version property optional

c1dcf33

Merge branch 'main' into config_schema

341d3b4

alexdewar enabled auto-merge August 27, 2024 15:42

alexdewar disabled auto-merge August 27, 2024 15:44

github-actions bot reviewed Aug 27, 2024

View reviewed changes

src/HealthGPS.Input/schema.cpp Show resolved Hide resolved

alexdewar enabled auto-merge August 27, 2024 15:56

Don't use else after throw

ee63655

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

[pre-commit.ci] auto fixes from pre-commit.com hooks

434b170

for more information, see https://pre-commit.ci

github-actions bot reviewed Aug 27, 2024

View reviewed changes

alexdewar merged commit 470eaf0 into main Aug 27, 2024
5 checks passed

alexdewar deleted the config_schema branch August 29, 2024 08:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add JSON schema for top-level config file + use for validation #489

Add JSON schema for top-level config file + use for validation #489

alexdewar commented Aug 13, 2024

github-actions bot left a comment

github-actions bot Aug 13, 2024

github-actions bot left a comment

TinyMarsh left a comment

TinyMarsh Aug 15, 2024

alexdewar Aug 15, 2024

TinyMarsh Aug 15, 2024

TinyMarsh Aug 15, 2024

alexdewar Aug 15, 2024

jamesturner246 Aug 16, 2024

alexdewar Aug 27, 2024

TinyMarsh Aug 15, 2024

alexdewar Aug 15, 2024

jamesturner246 Aug 16, 2024

alexdewar commented Aug 15, 2024

jamesturner246 left a comment

jamesturner246 Aug 16, 2024

alexdewar Aug 27, 2024 •

edited

Loading

alexdewar commented Aug 27, 2024

alexdewar commented Aug 27, 2024

github-actions bot left a comment

github-actions bot Aug 27, 2024

github-actions bot left a comment

github-actions bot left a comment

github-actions bot Aug 27, 2024

	static constexpr int ConfigSchemaVersion = 1;
	constexpr int ConfigSchemaVersion = 1;

		// YUCK: We have to read in the data with jsoncons here rather than reusing the
		// nlohmann-json representation :-(

	}
	} fmt::print(fmt::fg(fmt::color::dark_salmon), "{}\n", message);

	}
	} fmt::print(fmt::fg(fmt::color::dark_salmon), "{}\n", message);
	fmt::print(fmt::fg(fmt::color::dark_salmon), "{}\n", message);

Add JSON schema for top-level config file + use for validation #489

Add JSON schema for top-level config file + use for validation #489

Conversation

alexdewar commented Aug 13, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Aug 13, 2024

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

TinyMarsh 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

alexdewar commented Aug 15, 2024

jamesturner246 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexdewar Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

alexdewar commented Aug 27, 2024

alexdewar commented Aug 27, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Aug 27, 2024

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Aug 27, 2024

Choose a reason for hiding this comment

alexdewar Aug 27, 2024 •

edited

Loading