Centralize validation for Mechanism

[boulderdaze]

This PR:

• Separating the parsing into: FileToYaml(), Validate(), and Parse()
• Reworked UniversalParser to read the version first and then proceeding with parsing
• In the entire mechanism validation, if species or phases fail validation, the function returns immediately, because the parsers for reactions, models need them for their validation.
• Closes Centralize validation for Mechanism #233

[github-actions]

📄 Documentation for this branch is available at: https://ncar.github.io/MechanismConfiguration/branch/233-validator-mechanism/

[Copilot]

Pull Request Overview

This PR refactors the development parser to centralize validation for the Mechanism class. The main changes include:

• Separating the parsing workflow into three distinct methods: FileToYaml(), Validate(), and Parse()
• Renaming header files for clarity: parser.hpp → mechanism.hpp, validator.hpp → type_validators.hpp, mechanism_parsers.hpp → type_parsers.hpp
• Updating all test files to use the new three-step parsing API
• Reordering test entries in CMakeLists.txt to alphabetize them

Reviewed Changes

Copilot reviewed 69 out of 71 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
test/unit/development/*.cpp	Updated all test files to use new Parser API with separate FileToYaml, Validate, and Parse calls
test/unit/development/reactions/*.cpp	Updated reaction test files to use new Parser API pattern
test/unit/development/models/*.cpp	Updated model test files to use new Parser API pattern
test/integration/*.cpp	Updated integration tests to use new API and exception-based error handling
src/development/*.cpp	Refactored parser implementation and renamed files for clarity
include/mechanism_configuration/development/*.hpp	Renamed and reorganized header files with updated function signatures
CMakeLists.txt files	Reordered test entries and updated source file lists

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (94a42ff) to head (79dbb91).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##             main      #237       +/-   ##
============================================
+ Coverage   79.31%   100.00%   +20.68%     
============================================
  Files           4         4               
  Lines          29         7       -22     
============================================
- Hits           23         7       -16     
+ Misses          6         0        -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[K20shores]

The universal parser is used in musica, since we won't know which version of mechanism we are getting. Please replace the universal parser

[boulderdaze]

The universal parser is used in musica, since we won't know which version of mechanism we are getting. Please replace the universal parser

Good point, thanks! I updated the universal parser to check the version first and then parsing. Could you review it again?

[K20shores]

I'm not sure we need to be parsing the version. The parsers already do that, but I'm happy to be convinced otherwise

[K20shores]

We still need to be parsing v0 since that is the most used version. MusicBox Interactive still writes out those versions. We have a task to update that (NCAR/music-box-interactive#505). We always need to support v0 and v1.

Also, I'm still not convinced that we are going to make the development parser be a major version update. The breaking changes we've made could easily handle both options in code without breaking current versions. And, the new reactions are just an extension of v1, so we could do a minor version update. We'll save that conversation for later though

Either way, we still need to support v0.

Do we really need to parse the version here? Each mechanism will report if a parse was successful or not and the existing implementation is about half the number of lines.

[boulderdaze]

The conversation I had with you and Matt was that we'll eventually drop support for the older version as the parser evolves. That's why I introduced the changes.
Instead of initiating every parser, we read the version and create the only one. If you think our parser is not there yet, I'm happy to revert.

The current dev version breaks the v1 parser and they are not compatible. (for examaple, species_name is not supported ; Phase specifies PhaseSpecies, not std::string)

Regarding reading the YAML twice, yes, this PR does that in order to extract the version, but I think it's acceptable to support for the code created in-place where a yaml node is passed directly for validation.

Since you have a better understanding of how this is used, I'm open to following what you suggest. Let me know what you think is the best approach

[K20shores]

It's true that we removed species_name, but we could also add back support for that, especially since that parsing/validation is now more shared than it was before.

There's nothing wrong with parsing as it is, but it seems like the try/parse method we had before is just as good. The actual objects aren't very large in memory so i doubt it's a huge performance impact. If you like it as is, we can keep it though

[boulderdaze]

Would you like to have this kind of version? I copied and pasted the code here because this fails, as the dev code checks the config path to ensure it isn't a directory while the v0 parser allows it, so I'll figure out after this is confirmed.

The main drawback of this approach is that it tries to parse for every version of the parser. I think this is related to performance, but I’m open to following your suggestion

  ParserResult<GlobalMechanism> UniversalParser::Parse(const std::filesystem::path& config_path)
  {
    ParserResult<GlobalMechanism> result;

    if (!std::filesystem::exists(config_path))
    {
      result.errors.push_back({ 
        ConfigParseStatus::FileNotFound, 
        std::format("Configuration file '{}' does not exist.", 
          config_path.string())
      });
      return result;
    }

    v1::Parser v1_parser;
    auto v1_result = v1_parser.Parse(config_path);

    if (v1_result)
    {
      result.mechanism = std::move(v1_result.mechanism);
      return result;
    }

    development::Parser dev_parser;
    try
    {
      YAML::Node object = dev_parser.FileToYaml(config_path); // This can throw an exception
      auto dev_errors = dev_parser.Validate(object);
      if (dev_errors.empty())
      {
        result.mechanism = std::make_unique<development::types::Mechanism>(dev_parser.Parse(object));
        return result;
      }
      else
      {
        result.errors.insert(result.errors.end(), dev_errors.begin(), dev_errors.end());
      }
    }
    catch(const std::exception& e)
    {
      result.errors.push_back({ ConfigParseStatus::UnexpectedError, e.what()});
    }

    v0::Parser v0_parser;
    auto v0_result = v0_parser.Parse(config_path);

    if (v0_result)
    {
      result.mechanism = std::move(v0_result.mechanism);
      return result;
    }

    result.errors = v1_result.errors;
    result.errors.insert(result.errors.end(), v0_result.errors.begin(), v0_result.errors.end());
    return result;
  }

[K20shores]

I prefer this only because we aren't doing the version checks. Each parser already checks for version compatibility, and the code returns early on the first successful parse.

This also makes me wonder about the interface. I didn't question it at first, but now that I see here we are calling FileToYaml and Validate, I wonder if we should be consider a refactor that further refines our interfaces around Parseing and Validateing and the different places the information can come from.

In the v1 parser we have three parsing interfaces

MechanismConfiguration/include/mechanism_configuration/v1/parser.hpp

Lines 21 to 23 in 89583c6

	ParserResult<types::Mechanism> Parse(const std::filesystem::path& config_path);
	ParserResult<types::Mechanism> ParseFromString(const std::string& content);
	ParserResult<types::Mechanism> ParseFromNode(const YAML::Node& object);

I guess a straightforward interface would be (for any specific parser and the universal parser)

ParserType parser;

// read and validate from a file
Mechanism m = parser.ReadAndValidateFromFile(file);

// read from a string
std::string config = R"< a configuration> ";
Mechanism m = parser.ReadAndValidateFromString(file);

// parse a yaml node
yamlnode node = <whatever>;
Mechanism m = parser.ReadAndValidateFromNode(node);

Additionally, we will also need something that allows us to validate an already formed mechanism. Something along these lines:

// in musica somewhere
bool valid(Mechanism m) {
   return m.validate();
   // or maybe
   V1Parser p;
   Errors e = p.validate(m);
    if !(e) <raise an exception>
    return true;
}

All that to say, probalby what you have is fine, but these are the considerations that are running through my head. I'm curious to know your thoughts on them. Perhaps we should accept what you have here and refactor to support what we need in python for the next bit of work

[boulderdaze]

Yes, I agree. Checking the version is being done twice to support validation for non-file forms, and we will add that in the next issue:

• Handle duplicate version checks #238.

Those interfaces are wrapper that can contain Validate() and Parse(). The return types (ParserResult<types::Mechanism>) make more sense to me than just returning Mechanism because they also include errors.

// current
ParserResult<types::Mechanism> Parse(const std::filesystem::path& config_path); 
// suggested
Mechanism m = parser.ReadAndValidateFromFile(file);

Yes, we can merge this for now and create issues to refactor or/and address them

[K20shores]

sounds good

[K20shores]

I do have a preference for the try/parse flow we had for the universal parser before, but there's nothing functionally wrong here