Sanitise input data #200

alexdewar · 2023-08-09T11:17:40Z

So I think this is finally ready for review. I'm afraid it's rather on the long side if you go by the diffstat, however a lot of the change is just moving things from one file to another and much of the rest is tests so I hope you'll forgive me 😉.

The goal of this PR was to refactor the config parsing code to make loading JSON files safer. Currently, exceptions are only used intermittently and in many places, if the JSON file is malformed then a warning will be emitted, but the simulation will still run, just using default values. I've tried to make these config parsing errors as informative as possible, by printing error messages whenever malformed values are found and only throwing an exception at the last possible point. I've tried not to modify the file format in any way and the existing example data files load (seemingly) correctly.

I've also added tests for all of the config parsing code as well as (I think) any other bits I've changed, so hopefully we won't break any of this going forward.

I've rearranged files somewhat too. In particular, configuration.cpp ended up rather long by the time I'd finished with it, so I moved the code for parsing command line options into its own files as well as the lower-level config parsing code.

Finally, while I was working on this I introduced smatterings of type changes all over to make the types more descriptive and safer, e.g. converting std::strings to std::filesystem::paths where appropriate.

Fixes #161.

Fixes #186.

Fixes #198.

github-actions · 2023-08-09T11:27:15Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2023-08-09T12:27:57Z

clang-tidy review says "All clean, LGTM! 👍"

…rings

They make more sense there.

github-actions

clang-tidy made some suggestions

src/HealthGPS.Console/configuration_parsing_helpers.h

github-actions · 2023-08-09T13:09:18Z

clang-tidy review says "All clean, LGTM! 👍"

This is so we can test them with GTest.

github-actions · 2023-08-09T14:45:58Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions

clang-tidy made some suggestions

src/HealthGPS.Console/configuration_parsing.cpp

github-actions

clang-tidy made some suggestions

src/HealthGPS.Tests/Configuration.Test.cpp

dalonsoa

I've just commented on a couple of very high level points. Otherwise, I think it looks good and, as far as I can tell, the split of functionality in new files makes sense.

I don't know if it is common in C++, but it might be useful to add a documentation header in each file explaining what the contents of that file are about, so developers - old and new - can learn what to expect in each case when reading the code. We should do the same in our python projects, by the way.

dalonsoa · 2023-08-16T05:55:53Z

src/HealthGPS.Console/jsonparser.h

The same way you have done with the configuration files, I miss some more thorough docstrings in this one.

dalonsoa · 2023-08-16T05:58:12Z

src/HealthGPS.Console/poco.h

I hope poco is a clever acronym of something meaningful in Health-GPS. In Spanish, it means little, so it always looks odd to me when reading the code :P

Apparently it means "plain old class object": https://imperialchepi.github.io/healthgps/architecture

I'm not sure that this naming is terribly informative though. And I don't know why these structs live in host::poco rather than hgps::poco either 🤷. Maybe we should rename it at some point.

dalonsoa · 2023-08-16T05:59:14Z

src/HealthGPS.Console/poco.h

As above, some docstrings - at least for the functions you have touched - will be useful.

dalonsoa · 2023-08-16T06:01:09Z

src/HealthGPS.Console/program.cpp

Why there's no header file for this one? Not that matters, but I'm curious about why sometimes there is and sometimes there isn't, being rather novice in C++.

It's basically because there are no other .cpp that need any of the functions in program.cpp, so we don't need to bother. It seems like most functions in HealthGPS are defined in headers, but a lot of these definitions could be dropped if we wanted because the functions aren't actually being used outside of a single .cpp file.

These functions may give different values and notably do so on the Windows GitHub Runner.

github-actions · 2023-08-16T10:05:18Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2023-08-16T10:07:31Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2023-08-16T10:36:15Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2023-08-16T13:18:54Z

clang-tidy review says "All clean, LGTM! 👍"

jamesturner246

It looks a bunch safer, with some nice reuse of existing core code. 💯

There's quite a lot going on here, though. I wasn't able to go through with as much rigour as I'd like, so I've probably missed some things. I think breaking up these changes and planning/synchronising should be in our minds going forward in these kinds of big PRs.

I've picked some things that jumped out at me, and perhaps I'll have a few more once I digest it tomorrow.

jamesturner246 · 2023-08-16T14:27:06Z

src/HealthGPS.Console/jsonparser.h

+template <typename T>
+void to_json(nlohmann::json &j, const std::optional<hgps::core::Interval<T>> &p) {
+    if (p) {
+        j = *p;
+    } else {
+        // Null interval expressed as empty JSON array
+        j = nlohmann::json::array();
+    }
+}
+
+template <typename T>
+void from_json(const nlohmann::json &j, std::optional<hgps::core::Interval<T>> &p) {
+    // Treat null JSON values and empty arrays as a null Interval
+    if (j.is_null() || (j.is_array() && j.empty())) {
+        p = std::nullopt;
+    } else {
+        p = j.get<hgps::core::Interval<T>>();
+    }
+}
+


These aren't needed, since they will be handled by the Interval<T> to/from functions indirectly though the optional<T> to/from.

That's true, but currently the code uses empty arrays to indicate a null interval rather than a JSON null value, which is really what it should be, so I added these helpers to avoid introducing a breaking change. I meant to add a comment saying as much, but forgot.

That said, I don't like this and I think the right solution is to do away with this special-casing and just use null values instead, though that would mean changing config files too. Shall I open an issue for it?

Agree that the null values are preferred. I think I already started using null values elsewhere for things. /Go ahead.

jamesturner246 · 2023-08-16T15:48:45Z

src/HealthGPS.Console/CMakeLists.txt

@@ -5,16 +5,15 @@ find_package(TBB CONFIG REQUIRED)
 find_path(RAPIDCSV_INCLUDE_DIRS "rapidcsv.h")
 find_package(Threads REQUIRED)

-add_executable(HealthGPS.Console "")
-target_compile_features(HealthGPS.Console PUBLIC cxx_std_${CMAKE_CXX_STANDARD})


why no target_compile_features(HealthGPS.Console PUBLIC cxx_std_${CMAKE_CXX_STANDARD})?

It's redundant because targets are already built using ${CMAKE_CXX_STANDARD} anyway.

jamesturner246 · 2023-08-16T15:59:09Z

src/HealthGPS.Console/poco.h

    std::map<std::string, std::string> columns;
+
+    auto operator<=>(const FileInfo &rhs) const = default;


Why the spaceship operators?

This is a new C++20 thing. If you add a default spaceship operator, then it automatically generates other comparison functions too (i.e. operator==, operator< etc.): https://en.cppreference.com/w/cpp/language/default_comparisons

Oddly you don't get a default operator== otherwise, which seems a little odd to me.

jamesturner246 · 2023-08-16T16:10:19Z

src/HealthGPS.Console/jsonparser.cpp

+// Data file information
+void to_json(json &j, const FileInfo &p) {
+    j = json{
+        {"name", p.name}, {"format", p.format}, {"delimiter", p.delimiter}, {"columns", p.columns}};
+}


I don't think this is needed.

It's currently used by one of the unit tests.

Ah, mu bad.

jamesturner246 · 2023-08-16T16:15:42Z

src/HealthGPS.Console/jsonparser.cpp

+// SES Model Information
+void to_json(json &j, const SESInfo &p) {
+    j = json{{"function_name", p.function}, {"function_parameters", p.parameters}};


... in fact, I don't think any of the POCO-type to_json functions are actually needed anywhere. The only JSON writing is internal hgps event messages, and done in result_file_writer.

I thought the same and went through deleting a bunch of them, but then when I got round
to writing unit tests I found they were useful for that.

src/HealthGPS/mapping.h

jamesturner246 · 2023-08-16T16:51:59Z

src/HealthGPS.Console/configuration_parsing.cpp

+}
+
+// NOLINTNEXTLINE(bugprone-exception-escape)
+void rebase_valid_path_to(const json &j, const std::string &key, std::filesystem::path &out,


Why is the return-by-reference also needed?

It's not, really. It's just that out might not be updated. Maybe in that case,
returning an empty std::filesystem::path wouldn't be terrible though.

Alternatively, this function could just return a bool indicating whether it's
succeeded, which is what I've done elsewhere (e.g. for the get_to function). That
would at least be consistent.

What do you think?

Sorry, I meant that there's two rebase_valid_path_to, one on ln 34 which returns success as the return value, and another which returns success via a bool reference on ln 51. Are both necessary?

Yeah, for the sake of consistency, the boolean flag method is fine.

The overload returning a bool isn't actually used anywhere, but I figured we might as
well keep it because we might do in future and it kind of makes sense to implement the
other overload using it anyway. That said, it's up to you. I could get rid of it and it
would save us a few lines of code.

jamesturner246 · 2023-08-16T17:21:47Z

src/HealthGPS.Console/configuration_parsing_helpers.h

Looks like this header is more for risk factor model JSON parsing stuff, right? Was the plan to have a separate header for the parsing interfaces of each model type? Maybe renaming this something like configuration_parsing_risk_factor.h would clarify this?

Good idea. The reason I put these bits into their own header was really just because it felt like configuration_parsing.h was getting a bit cluttered and these functions only needed to be exported so we could access them for the tests.

Co-authored-by: James Paul Turner <[email protected]>

alexdewar · 2023-08-17T09:53:32Z

Thanks for the review!

There's quite a lot going on here, though. I wasn't able to go through with as much rigour as I'd like, so I've probably missed some things. I think breaking up these changes and planning/synchronising should be in our minds going forward in these kinds of big PRs.

Yeah, sorry about that 😞. If I'd split this up into smaller PRs it also would have made it less likely we'd end up with merge conflicts too. For the next big refactor I'll plan ahead a bit more rather than just winging it.

github-actions · 2023-08-17T09:53:43Z

clang-tidy review says "All clean, LGTM! 👍"

This reverts commit 0157f76.

github-actions · 2023-08-18T21:18:44Z

clang-tidy review says "All clean, LGTM! 👍"

We don't need the extra option to be able to use an empty array. Let's make users express null Intervals with a JSON null value.

github-actions · 2023-08-21T09:46:00Z

clang-tidy review says "All clean, LGTM! 👍"

jamesturner246

Go ahead 👍

github-actions · 2023-08-21T09:59:20Z

clang-tidy review says "All clean, LGTM! 👍"

alexdewar added 10 commits August 7, 2023 16:41

Remove create_output_folder() function

bed9892

Fixes #186.

Use std::filesystem::path cf. std::string in a few places

50505e1

Drop support for v1 config files

2700324

Fixes #198.

load_configuration(): Remove some indentation

358c762

Make separate function for rebasing paths

a2fd569

Put new functions for loading JSON into configuration.cpp

c66c0b8

Add ConfigurationError class and use in a few places

d9a8ab8

Remove unused field

100ffd7

Clean up and add extra error checking to config file loading

d1cc571

Remove superfluous helper functions

f480522

alexdewar force-pushed the sanitise_input_data branch from 6aa55d0 to 0e35a28 Compare August 9, 2023 12:20

alexdewar added 4 commits August 9, 2023 13:28

Fix: Seemingly MSVC isn't happy about casting filesystem::paths to st…

2597d32

…rings

Move some functions out of configuration.cpp to program.cpp

9a1188c

They make more sense there.

Put functionality related to command-line args in own cpp file

3efc2be

Move some parsing functions into configuration_parsing.cpp

0d1f803

alexdewar force-pushed the sanitise_input_data branch from 0e35a28 to 57e0a72 Compare August 9, 2023 12:44

github-actions bot reviewed Aug 9, 2023

View reviewed changes

src/HealthGPS.Console/configuration_parsing_helpers.h Show resolved Hide resolved

alexdewar force-pushed the sanitise_input_data branch from 57e0a72 to dc1a050 Compare August 9, 2023 13:01

jamesturner246 mentioned this pull request Aug 9, 2023

Load nutrients #199

Merged

Put configuration parsing helper function defs into own header

3d72290

This is so we can test them with GTest.

alexdewar force-pushed the sanitise_input_data branch from dc1a050 to 7931f38 Compare August 9, 2023 14:37

github-actions bot reviewed Aug 9, 2023

View reviewed changes

src/HealthGPS.Console/configuration_parsing.cpp Outdated Show resolved Hide resolved

src/HealthGPS.Console/configuration_parsing.cpp Outdated Show resolved Hide resolved

github-actions bot reviewed Aug 10, 2023

View reviewed changes

src/HealthGPS.Tests/Configuration.Test.cpp Outdated Show resolved Hide resolved

src/HealthGPS.Tests/Configuration.Test.cpp Outdated Show resolved Hide resolved

alexdewar force-pushed the sanitise_input_data branch 2 times, most recently from 6e1313b to d857633 Compare August 11, 2023 10:09

Rename some functions and mark noexcept

edc505d

alexdewar requested review from dalonsoa and jamesturner246 August 15, 2023 15:57

alexdewar marked this pull request as ready for review August 15, 2023 15:58

Fix narrowing warning on MSVC

9d9801d

dalonsoa approved these changes Aug 16, 2023

View reviewed changes

alexdewar added 3 commits August 16, 2023 10:12

Fix: Interval class's members should be default-initialised

7deec87

Fix/suppress bugprone-exception-escape clang-tidy warning

aafadfb

Fix: Use absolute cf. weakly canonical paths in rebase_valid_path()

6f19143

These functions may give different values and notably do so on the Windows GitHub Runner.

Fix docstring warnings and rename a couple of args

cbd23d7

alexdewar added 3 commits August 16, 2023 11:46

Add docstrings to poco.h

a592a1b

Remove unused function definition

26f1b31

Add file-level documentation for several headers

6438589

jamesturner246 requested changes Aug 16, 2023

View reviewed changes

range parameter doesn't need a default value

0157f76

Co-authored-by: James Paul Turner <[email protected]>

Revert "range parameter doesn't need a default value"

c9e3b2d

This reverts commit 0157f76.

Remove special-casing for std::optional<Interval<T>>

72d6519

We don't need the extra option to be able to use an empty array. Let's make users express null Intervals with a JSON null value.

jamesturner246 approved these changes Aug 21, 2023

View reviewed changes

Merge branch 'main' into sanitise_input_data

00fc19f

alexdewar enabled auto-merge August 21, 2023 09:48

alexdewar merged commit 44c8aea into main Aug 21, 2023

alexdewar deleted the sanitise_input_data branch August 21, 2023 10:09

		std::map<std::string, std::string> columns;

		auto operator<=>(const FileInfo &rhs) const = default;

Sanitise input data #200

Sanitise input data #200

Conversation

alexdewar commented Aug 9, 2023 • edited Loading

github-actions bot commented Aug 9, 2023

github-actions bot commented Aug 9, 2023

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 9, 2023

github-actions bot commented Aug 9, 2023

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

dalonsoa 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

github-actions bot commented Aug 16, 2023

github-actions bot commented Aug 16, 2023

github-actions bot commented Aug 16, 2023

github-actions bot commented Aug 16, 2023

jamesturner246 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

jamesturner246 Aug 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesturner246 Aug 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexdewar commented Aug 17, 2023

github-actions bot commented Aug 17, 2023

github-actions bot commented Aug 18, 2023

github-actions bot commented Aug 21, 2023

jamesturner246 left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 21, 2023

alexdewar commented Aug 9, 2023 •

edited

Loading

jamesturner246 Aug 19, 2023 •

edited

Loading

jamesturner246 Aug 16, 2023 •

edited

Loading