Skip to content

Conversation

@mwiebe
Copy link
Contributor

@mwiebe mwiebe commented Mar 24, 2025

What was the problem/requirement? (What/Why)

To support Open Job Description modeling parsing that differs depending on the revision and/or enabled extensions, the code needs a way to parse differently based on those values. This is required in order to experiment with proof of concepts for OpenJobDescription/openjd-specifications#79.

The question is, where to split the processing, should we do it inside the FormatString code, or should we do it in the models that use the FormatString code. This PR chooses the former because making the code choose the behavior in FormatString will be simpler to experiment with and will decouple expression-related changes from the rest of the Open Job Description models.

What was the solution? (How)

Refactor the expression string processing to make putting together different proofs of concept for OpenJobDescription/openjd-specifications#79 easier.

  • Introduce an abstract base class, ModelParsingContextInterface, that every model parsing context must be a subclass of. It has variables spec_rev and extensions, for parsing to reference.
  • Modify the DynamicConstrainedStr class to require a ModelParsingContextInterface during construction. The FormatString to handle template the expression syntax is a subset, so modifying this class to handle the Pydantic integration was the simplest approach.
    • As part of this, change DynamicConstrainedStr and subclasses to use new instead of init, so they all handle construction uniformly with the required context parameter.
  • Create a function format_string_expr_parse that handles parsing a string according to the format string expression parsing syntax, using a model parsing context to determine the specific syntax. This does not change the syntax, only introduces a place to add new syntax support.
  • Update the template variable reference pre-validation and template -> job instantiation code to work with the updated DynamicConstrainedStr that now requires a context. This required the instantiation logic to be more clear about the separation between Template and Job, since the latter instantiation happens without a context.

What is the impact of this change?

Putting together different proofs of concept for OpenJobDescription/openjd-specifications#79 will be easier.

After this change, the code to use Python ast for the expression parsing as considered in OpenJobDescription/openjd-specifications#79 (comment) is quite small: mwiebe@4456628.

How was this change tested?

  • Everywhere that the tests do model validation or construct DynamicConstrainedStr/FormatString instances, introduce the model parsing context. In some places had to change the content of the tests to reflect the more strict separation between model parsing and job instantiation.
  • Change various different model construction used in the tests to call decode_job_template instead. The latter function has its own tests, and using the specified Open Job Description syntax instead of object constructors isolates the tests from implementation details.

Was this change documented?

Docstrings for affected classes were updated.

Is this a breaking change?

BREAKING CHANGE: Any creation of a DynamicConstrainedStr or FormatString
will need to provide a model parsing context, that includes the Open
Job Description revision and any extensions that are enabled.

Does this change impact security?

No


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@mwiebe mwiebe requested a review from a team as a code owner March 24, 2025 23:58
@mwiebe mwiebe force-pushed the format-str-parsing-context branch from 8f6541d to 0dc7e13 Compare March 25, 2025 00:01
crowecawcaw
crowecawcaw previously approved these changes Mar 25, 2025
To support Open Job Description modeling parsing that differs depending
on the revision and/or enabled extensions, the code needs a way to
parse differently based on those values. The chosen approach is to
include these in the parsing context.

* Introduce an abstract base class, ModelParsingContextInterface, that
  every model parsing context must be a subclass of. It has variables
  spec_rev and extensions, for parsing to reference.
* Modify the DynamicConstrainedStr class to require a
  ModelParsingContextInterface during construction. The FormatString to
  handle template the expression syntax is a subset, so modifying this
  class to handle the Pydantic integration was the simplest approach.
   * As part of this, change DynamicConstrainedStr and subclasses to use
     __new__ instead of __init__, so they all handle construction
     uniformly with the required context parameter.
* Everywhere that the tests do model validation or construct
  DynamicConstrainedStr/FormatString instances, introduce the model
  parsing context. In some places had to change the content of the tests
  to reflect the more strict separation between model parsing and job
  instantiation.
* Change various different model construction used in the tests to call
  decode_job_template instead. The latter function has its own tests,
  and using the specified Open Job Description syntax instead of object
  constructors isolates the tests from implementation details.
* Create a function format_string_expr_parse that handles parsing a
  string according to the format string expression parsing syntax, using
  a model parsing context to determine the specific syntax. This does
  not change the syntax, only introduces a place to add new syntax
  support.
* Update the template variable reference pre-validation and template ->
  job instantiation code to work with the updated DynamicConstrainedStr
  that now requires a context. This required the instantiation logic to
  be more clear about the separation between Template and Job, since the
  latter instantiation happens without a context.
* Simplify the instantiate_model logic with a context manager to unify
  handling of validation errors. This reduces code duplication, and
  removed the optional loc and within_field arguments that were for
  internal recursion purposes.

BREAKING CHANGE: Any creation of a DynamicConstrainedStr or FormatString
    will need to provide a model parsing context, that includes the Open
    Job Description revision and any extensions that are enabled. Use of
    the instantiate_model can no longer provide optional loc and
    within_field arguments.

Signed-off-by: Mark Wiebe <399551+mwiebe@users.noreply.github.com>
@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
4.1% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

@mwiebe mwiebe enabled auto-merge (rebase) March 26, 2025 04:51
for err_key, err_value in error_details.items():
if err_key == "loc":
init_error_details["loc"] = (*loc, *err_value) # type: ignore
elif err_key != "msg":
Copy link

Choose a reason for hiding this comment

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

nit, do we care about any other else? Like future "unknown" errors in case they get swallowed ?

Copy link

Choose a reason for hiding this comment

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

From the docs, eg "type" is also a key type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The InitErrorDetails class https://docs.pydantic.dev/latest/api/pydantic_core/#pydantic_core.InitErrorDetails is well-defined. The reason the code is filtering it out because ErrorDetails has that field but InitErrorDetails does not.

@mwiebe mwiebe merged commit 2a8db9d into OpenJobDescription:mainline Mar 28, 2025
18 of 19 checks passed
@mwiebe mwiebe deleted the format-str-parsing-context branch March 28, 2025 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants