FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix: Normalize intstring/floatstring task parameter range elements by leongdl · Pull Request #342 · OpenJobDescription/openjd-model-for-python · GitHub

fix: Normalize intstring/floatstring task parameter range elements - #342

Open
leongdl wants to merge 4 commits into
OpenJobDescription:mainlinefrom
leongdl:fix/range-list-string-element-normalization
Open

fix: Normalize intstring/floatstring task parameter range elements#342
leongdl wants to merge 4 commits into
OpenJobDescription:mainlinefrom
leongdl:fix/range-list-string-element-normalization

Conversation

leongdl commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

What

A task parameter range element written in the <intstring> / <floatstring> string form now carries the number it denotes instead of its source text.

- name: Frame
  type: INT
  range: ['1', '02', '003']     # was FRAME:1 FRAME:02 FRAME:003, now FRAME:1 FRAME:2 FRAME:3

- name: Weight
  type: FLOAT
  range: ['1.5', '02.50']       # was W:1.5 W:02.50, now W:1.5 W:2.5

The spec reading being adopted

Template Schemas 2023-09 §3.4.1.1 (L1110) makes an <IntRangeList> element <integer> | <intstring>, and §3.4.1.2 (L1184) makes a <FloatRangeList> element <float> | <floatstring>. Neither is behind an extension gate. §2.3 (L318) and §2.4 (L375) define <intstring> / <floatstring> as "a string whose value is the string representation of" a number, so such an element denotes the number, not the text it was written with.

The reading is scoped to the string forms, and deliberately so:

  • Range-list elements in string form normalise. '02' is the task value 2.
  • Numeric range literals keep the scale they were written with. A FLOAT range: [1.0] still renders 1.0. openjd-rs renders an integral float the same way, and the suite pins it in 2023-09/base/jobs/3.4--float-parameter.
  • Job parameter default values do not normalise. Defaults live on the job-parameter definitions and ranges on the task-parameter definitions — separate models, and this change touches only the latter. openjd-rs makes exactly the same split. 2023-09/EXPR/jobs/expr1.3.4--float-passthrough, which pins a FLOAT default: "3.500" rendering verbatim as PARAM:3.500, still passes.
  • STRING and PATH ranges are text by definition. §3.4.1.3/§3.4.1.4 give them no numeric element form, so there is nothing to normalise.

openjd-rs already behaves this way and passes both conformance fixtures. It parses every range element into an i64/f64 at create_job (crates/openjd-model/src/job/create_job/ranges.rs), so it has no source text to carry in the first place.

Why Python was keeping the text

The pre-validator on the template models already computes the value — the element validator evaluates int('02') == 2 in validate_int_fmtstring_field — but validate_list_field returns the original list object, discarding every coercion. Nothing downstream recovered it.

The fix is applied one layer later, on RangeListTaskParameterDefinition, the instantiation target that all three inbound range paths funnel through: a literal list, a range-expression expansion, and an RFC 0006 typed whole-field resolution. Keying off the already-validated type field, an INT or CHUNK[INT] element becomes an int and a FLOAT element becomes a normalised Decimal.

Decimal.normalize() needs one guard: it rewrites Decimal('100') as Decimal('1E+2'), so a positive exponent is quantized away. Exponent notation must never reach a task command line.

An element that does not parse as a number is carried through unchanged rather than rejected here. A literal range is already checked against its element type at template parse time, and adding a rejection path for values arriving from a resolved format string is a separate change.

A latent containment bug this exposed

Normalising the stored elements turned six existing tests red, and the cause was already a defect on mainline.

RangeListIdentifierNode.validate_containment compared ParameterValue.value — the rendered form of a range element — against a set built from the raw elements. It only matched when a range happened to be written as strings. An INT range written as [1, 2, 3] reported every one of its own values as not contained, before this change:

mainline:  range=[10, 11]     contains(value='10') -> False   # wrong
mainline:  range=['10', '11'] contains(value='10') -> True
this PR:   both forms                              -> True

The containment set now holds rendered elements. TestRangeListElementValues::test_containment_matches_the_rendered_values pins both forms.

Verification

Model test suite: 5483 passed before, 5505 passed after (22 added), coverage 94.10% against the 94% gate. Every new assertion was checked for falsifiability by reverting each half of the fix separately: 8 of the 15 model-layer tests fail without the normalisation, and both containment cases fail without the range_set repair.

Conformance fixtures, run by direct file path against a venv built from this branch:

Fixture Before After
base/jobs/3.4.1.1--int-range-intstring-elements-normalized fail pass
base/jobs/3.4.1.2--float-range-floatstring-elements-normalized fail pass
base/jobs/3.4--float-parameter (numeric-literal control) pass pass
EXPR/jobs/expr1.3.4--float-passthrough (default scope guard) pass pass

These two fixtures were the only conformance failures the Python implementation had. A full 2023-09/* run on this branch is 1173 passed, 1 failed, and that one failure is 3.6--let-step-bindings-in-step-env, which is unrelated and waits on #341.

Sweep before writing the fix. Across the whole suite (1,193 files) there are 1,046 FloatRangeList elements and 2,420 IntRangeList elements. Exactly two FLOAT elements and three INT elements use the string form, and all five are in the two fixtures above — so nothing else in the suite asserts a rendering that this change moves.

The sweep also found the constraint that scoped the fix: 1,030 FLOAT elements are numeric literals whose rendering would change if numeric literals were normalised too, and one of them is asserted by an executing fixture (3.4--float-parameter asserts TASK:Scale=1.0 from a literal 1.0). Restricting normalisation to the string forms avoids that conflict without touching any existing assertion.

ruff, black and mypy clean at the CI-pinned versions.

Related

  • openjd-specifications#179 promotes both fixtures out of proposed/. They were parked there pending a ruling on whether <floatstring> normalises; this PR adopts the normalising reading for range elements while leaving parameter defaults verbatim, which is the split that lets the landed expr1.3.4--float-passthrough fixture and these two coexist.
  • fix: Evaluate step-level let bindings in template scope #341 (let bindings in template scope) is independent; neither touches the other's files.

Known remaining divergence

An integral <floatstring> still renders differently from openjd-rs: range: ['1.0'] on a FLOAT parameter renders 1 here and 1.0 there, because openjd-rs formats every integral f64 with a trailing .0 while this implementation preserves the scale a Decimal carries. No conformance fixture covers it, and closing it would change how numeric FLOAT literals render, which 3.4--float-parameter pins. Left alone deliberately.

Template Schemas 2023-09 §3.4.1.1 (L1110) makes an <IntRangeList> element
`<integer> | <intstring>`, and §3.4.1.2 (L1184) makes a <FloatRangeList>
element `<float> | <floatstring>`, neither behind an extension gate. §2.3 and
§2.4 define <intstring>/<floatstring> as "a string whose value is the string
representation of" a number, so such an element denotes the number rather than
its source text.

Python was keeping the source text. A `range: ['1', '02', '003']` on an INT
parameter rendered FRAME:02 and FRAME:003, and a `range: ['1.5', '02.50']` on
a FLOAT parameter rendered W:02.50. These values reach a task command line, so
a renderer was invoked with `--frame 02`.

The pre-validator on the template models did compute the value -- the element
validator evaluates int('02') == 2 -- but validate_list_field returns the
original list object, discarding every coercion, and nothing downstream
recovered it.

Normalize on RangeListTaskParameterDefinition instead, the instantiation
target that all three inbound range paths funnel through: a literal list, a
range-expression expansion, and an RFC 0006 typed whole-field resolution.
Keying off the already-validated `type` field, an INT or CHUNK[INT] element
becomes an int and a FLOAT element becomes a normalized Decimal. Decimal's
normalize() rewrites Decimal('100') as Decimal('1E+2'), so a positive exponent
is quantized away -- exponent notation must never reach a task command line.

Only string-form elements are touched. A <float> literal keeps the scale it was
written with, so a FLOAT `range: [1.0]` still renders 1.0; openjd-rs renders an
integral float the same way and the conformance suite pins it
(2023-09/base/jobs/3.4--float-parameter). STRING and PATH ranges are text by
definition and are left alone. Job parameter defaults are unaffected: they live
on the job-parameter definitions, not the task-parameter definitions, so the
verbatim FLOAT default behaviour that
2023-09/EXPR/jobs/expr1.3.4--float-passthrough pins is untouched.

Normalizing the stored elements exposed a latent bug in
StepParameterSpaceIterator containment. RangeListIdentifierNode compared
ParameterValue.value -- the rendered form of an element -- against a set built
from the raw elements, so it only ever matched when a range happened to be
written as strings. An INT range written as `[1, 2, 3]` already reported every
one of its own values as not contained, before this change. The containment set
now holds rendered elements.

openjd-rs already behaves this way and passes both conformance fixtures; it
parses every range element into an i64/f64 and so has no source text to carry.

Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
leongdl requested a review from a team as a code owner August 29, 2026 17:12
Comment thread src/openjd/model/v2023_09/_model.py Outdated
# them as text rather than changing how they render.
return elem
exponent = value.as_tuple().exponent
if isinstance(exponent, int) and exponent > 0:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The guard only undoes exponent notation for positive exponents, but Decimal.__str__ also uses scientific notation for sufficiently negative exponents (adjusted exponent below -6). So small <floatstring> elements now render in exponent notation on a task command line -- the exact thing the comment two lines below says must never happen:

  • "0.0000001" becomes Decimal("1E-7") and renders as 1E-7 (previously the source text 0.0000001 reached the renderer verbatim)
  • "0.00000025" becomes Decimal("2.5E-7") and renders as 2.5E-7

This is a behaviour regression for these values, not merely a missed normalization: before this validator the raw string was carried through unchanged.

The parametrized cases only cover exponents that shift upward ("100", "1E+2"), which is why it is not caught. Consider handling both directions: when the normalized exponent is negative and value.adjusted() < -6, return Decimal(format(value, "f")) to force plain notation -- and add a "0.0000001" case to test_floatstring_elements_carry_their_value.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Confirmed and fixed. Measured before/after on "0.0000001": the landed code rendered 1E-7 (str(Decimal) switches to exponent notation once the adjusted exponent drops below -6, which the positive-exponent guard never covered), and it is a regression, since keeping the source text rendered 0.0000001. It now renders 0.0000001 via format(value, "f"), which is plain at every magnitude — note this is a deliberate divergence from openjd-rs at the extremes, whose f64 Display emits 1e-07 here, because exponent notation must not reach a task command line.

Comment thread src/openjd/model/v2023_09/_model.py Outdated
try:
if to_int:
return int(elem)
value = Decimal(elem).normalize()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Decimal.normalize() runs under the ambient decimal context, so it rounds to getcontext().prec significant digits (28 by default). That makes this a silent, lossy rewrite of a task parameter value:

  • "1.1234567890123456789012345678901234567890" is rounded to 28 significant digits instead of being carried through exactly, and
  • because the context is process-global mutable state, an embedding application that has done getcontext().prec = 5 anywhere changes what range values this library produces — "1.234567" would become 1.2346.

Before this change the source text reached the renderer unchanged, so no precision could be lost. Since the only goal here is to strip redundant leading/trailing zeros, the operation does not need the default context: consider value.normalize(context=Context(prec=...)) with an explicitly large precision, or drop normalize() and strip the zeros without a context-sensitive arithmetic operation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Confirmed and fixed. Measured: normalize() cut a 41-significant-digit element to 1.234567890123456789012345679 at the default prec=28, and with an embedding app setting getcontext().prec = 5 the same template rendered 1.2346 — library output was a function of host process state. Replaced with format(value, "f") plus text-level zero stripping, neither of which consults the context; verified byte-identical output under prec=5, and pinned by a new localcontext() regression test.

# Numeric elements are left exactly as parsed. A FLOAT range of [1.0]
# renders 1.0, which openjd-rs also does and the conformance suite pins
# (base/jobs/3.4--float-parameter). STRING and PATH ranges are text by
# definition and are never touched.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This leaves the model rendering the same denoted number two different ways on a FLOAT parameter, depending only on how the element was spelled in the template:

  • range: [1.0] (a <float> literal) renders 1.0 — deliberately preserved, per this comment.
  • range: ["1.0"] (a <floatstring>) renders 1 — normalize() strips the fractional zero, and the new test pins this ("0.0" -> "0").

But §2.4 defines a <floatstring> as "a string whose value is the string representation of" a float, and "1.0" is the string representation of the float 1.0. By the PR's own reasoning the element denotes 1.0, so a FLOAT parameter emitting --frame 0 where the template said "0.0" looks like the same class of bug being fixed here, in the opposite direction. It also means the two forms disagree even though the spec treats them as alternate spellings of one value.

Worth confirming against openjd-rs / the conformance suite before pinning "0.0" -> "0" in a test: if openjd-rs renders 0.0 for a "0.0" floatstring, then trailing-zero stripping should be limited to zeros beyond the first fractional digit (i.e. keep at least one), rather than reducing an integral float to bare digits.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

You are right, and I probed openjd-rs to settle it rather than argue from the fixture, which does not cover an integral float (3.4.1.2 uses ['1.5', '02.50']). The Rust CLI renders "1.0" as 1.0, "0.0" as 0.0, "007" as 7.0 and "100" as 100.0, so my normalize() was too aggressive and produced a spelling-dependent split against the <float> literal 1.0. FLOAT elements now always keep one fractional digit, "-0.0" renders 0.0 since the denoted number is unsigned (matching Rust), and the test that pinned "0.0" -> "0" is updated along with 007, 100 and 1E+2.

Comment thread src/openjd/model/v2023_09/_model.py Outdated
if isinstance(exponent, int) and exponent > 0:
# normalize() rewrites Decimal('100') as Decimal('1E+2'); undo the
# shift so a range element never renders in exponent notation.
value = value.quantize(Decimal(1))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

quantize(Decimal(1)) raises InvalidOperation whenever the result would need more than getcontext().prec digits, and that exception is swallowed by the except (ValueError, ArithmeticError) below — which returns the original string. So for a large-exponent element the "never renders in exponent notation" guarantee falls back to whatever the template wrote:

  • range: ["1E+30"] on a FLOAT parameter -> normalize() gives Decimal("1E+30"), exponent 30 > 0, quantize needs 31 digits > prec 28 -> InvalidOperation -> the function returns the string "1E+30", which renders as 1E+30.

Not a regression (the raw text was passed through before), but the fallback silently defeats the stated invariant rather than reporting anything. If exponent notation genuinely must never reach a task command line, formatting with format(value, "f") avoids the precision-bounded quantize entirely and handles arbitrary exponents in one step.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Confirmed exactly as described, and fixed. Measured: Decimal("1E+30").normalize().quantize(Decimal(1)) raises InvalidOperation because the result needs 31 digits against prec=28, the except (ValueError, ArithmeticError) swallowed it, and range: ["1E+30"] fell back to the raw string 1E+30 — defeating the invariant the guard existed to hold; "123456.75" under a narrowed context hit the same path. quantize is gone entirely: format(value, "f") has no precision bound, so "1E+30" now renders 1000000000000000000000000000000.0 and there is no fallback path left for a finite value.

leongdl commented Aug 29, 2026

Copy link
Copy Markdown
Contributor Author

CI note: the THIRD-PARTY-LICENSES check fails on upstream dependency drift, not on anything in this change. The diff is four Python source and test files (git diff --name-only upstream/mainline..HEAD) and touches no dependency manifest, so it cannot move a dependency version. The report diff is a pydantic patch bump that landed between #341's run and this one:

-** pydantic; version 2.13.4        +** pydantic; version 2.13.5
-** pydantic_core; version 2.46.4   +** pydantic_core; version 2.46.5

Regenerating THIRD-PARTY-LICENSES.txt belongs in its own commit, since it will re-drift on the next pydantic release. Every other check is green: all 21 Python matrix jobs (macOS/Ubuntu/Windows, 3.9-3.14), all Rust jobs, cargo-deny, CodeQL and both Analyze legs.

the element unchanged when it does not denote one."""
try:
if to_int:
return int(elem)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The docstring says the element is returned unchanged when it "does not denote" a number, but int() / Decimal() accept a strictly wider grammar than <intstring> / <floatstring>, so some non-conforming elements are silently rewritten instead of passed through:

  • "1_0" -> 10 (PEP 515 underscores)
  • " 5 " -> 5 (surrounding whitespace)
  • "+7" -> 7 (explicit sign dropped from the rendered value)
  • non-ASCII decimal digits, e.g. "٣" -> 3

These are reachable from a literal template range: the template-layer check (validate_int_fmtstring_field via validate_list_field) also uses int(), and validate_list_field discards its validator's return value, so today the raw text is what reaches the renderer. After this change the rendered task value differs for these inputs.

Edge-case severity, but since the point of the change is to make the rendered value match what the element denotes per the spec, it may be worth matching against the spec's grammar (e.g. a regex for <intstring> / <floatstring>) rather than delegating to Python's more permissive parsers.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Accurate on the facts — int() and Decimal() do accept more than <intstring>/<floatstring>, and I reproduced "1_0" -> 10, " 5 " -> 5 and "+7" -> 7 — but I am declining this one. Every input that reaches it is a template already malformed under the spec grammar, so the only behaviour change would be swapping one undefined outcome for another, and a regex gate on both grammars is disproportionate to that. If you want the grammar enforced I would rather do it as validation at the template layer where the element type is already checked, as its own change with its own conformance fixture.

Regenerated versions for pydantic (2.13.4 -> 2.13.5) and pydantic_core
(2.46.4 -> 2.46.5) to match the resolved dependency set, clearing the
THIRD-PARTY-LICENSES check.

Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
return value
to_int = param_type != TaskParameterType.FLOAT
return [
_normalized_range_element(elem, to_int) if isinstance(elem, str) else elem

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The else elem branch skips every non-str element, so the "a range element never renders in exponent notation" invariant that _normalized_range_element establishes only holds for the <intstring>/<floatstring> spelling. A numeric FLOAT element takes this branch and can still render in exponent notation:

  • _coerce_typed_range_elements (the typed_resolve_coerce hook) appends elem.item() for the engine float variant, i.e. a native Python float. TaskRangeList = list[Union[TaskParameterStringValueAsJob, int, float, Decimal]] lists float, so pydantic's smart union keeps it as float on an exact-type match. A LIST[FLOAT] job parameter resolved whole-field into a FLOAT range (range: "{{Param.Values}}") with an element like 1e16 or 1e-7 therefore reaches RangeListIdentifierNode.__getitem__, which does str(self.range[index]) — str(1e+16) is '1e+16' and str(1e-07) is '1e-07', so the task command line gets --frame 1e+16.
  • A YAML <float> literal written in exponent form (range: [1.0e+16]) lands as a Decimal whose str() is likewise 1E+16.

So the guard covers range: ["1E+2"] (there is a test for it) but not the numerically-typed forms of the same value, which is the asymmetry the quantize branch two functions up exists to prevent.

If exponent notation must never reach a task command line, the guard belongs after the str/non-str split — e.g. normalize the exponent for any Decimal/float element as well, rather than only for elements that arrived as text.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Correct on the facts, but pre-existing rather than a regression, so I am declining it here rather than ignoring it. Checked on mainline (f7ca58f): a Decimal("1.0e+16") element already rendered 1.0E+16 there, a float 1e16 rendered 1e+16 and 1e-7 rendered 1e-07, and this branch leaves all three byte-identical — numeric elements were passed through untouched before it and still are. Normalizing them is deliberately out of scope for this PR because that pass-through is what keeps 3.4--float-parameter passing (it pins TASK:Scale=1.0 from a literal 1.0), so it needs its own conformance decision about what a numerically-typed <float> element renders as; worth its own issue.

Review found three defects that share one root cause: Decimal.normalize()
plus a positive-exponent quantize() guard is both context-sensitive and
notation-unstable.

- normalize() rounds to getcontext().prec. A 41-significant-digit element
  came back as 1.234567890123456789012345679, and an embedding application
  that sets getcontext().prec = 5 changed what this library rendered for the
  same template (measured: '1.2345678901234567890123456789012345678901' ->
  '1.2346'). Template output must not depend on host process state.
- str(Decimal) switches to exponent notation once the adjusted exponent is
  below -6, which the positive-exponent guard did not cover, so the element
  '0.0000001' rendered 1E-7. That was a regression: before normalization
  was introduced the source text was kept and it rendered 0.0000001.
- quantize(Decimal(1)) raises InvalidOperation once the result needs more
  digits than getcontext().prec, and the except clause swallowed it and
  returned the raw string. '1E+30' therefore rendered as 1E+30, silently
  defeating the very invariant the guard existed to hold. '123456.75' under
  a narrowed context hit the same path.

Render with format(value, 'f') instead. With no precision in the format spec
it is exact and plain at every magnitude, so it needs neither the rounding
of normalize() nor the precision-bounded quantize(): verified identical
output under getcontext().prec = 5. Redundant leading and trailing zeros are
then stripped from the text, which is not an arithmetic operation and cannot
consult the context.

An integral <floatstring> now keeps one fractional digit, so '1.0' renders
1.0 and not 1. openjd-rs was measured as the reference for this: it renders
'1.0' as 1.0, '0.0' as 0.0, '007' as 7.0 and '100' as 100.0. Dropping the
fraction made the same denoted number render two ways depending on whether
it was spelled as a <float> or a <floatstring>, since a <float> literal 1.0
renders 1.0. The conformance fixture does not settle it -- 3.4.1.2 uses
['1.5', '02.50'], neither of them integral -- so the Rust implementation
was probed directly. '-0.0' renders 0.0, since the number it denotes is
zero, which openjd-rs also does.

Plain notation is a deliberate divergence from openjd-rs at the extremes,
where its f64 Display gives 1e-07 and 1e+30. Exponent notation must not
reach a task command line at any magnitude, and Decimal carries the written
value exactly where f64 cannot.

Scope is unchanged. Numeric literals are still untouched, STRING and PATH
ranges are still text, and job parameter defaults are not involved.
2023-09/base/jobs/3.4--float-parameter and
2023-09/EXPR/jobs/expr1.3.4--float-passthrough both still pass, as do both
proposed 3.4.1.1/3.4.1.2 normalization fixtures; the full 2023-09
conformance suite is 1172 passed, 0 failed.

Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
# ambient precision, str() switches to exponent notation below 1e-6,
# and quantize() raises InvalidOperation once the result needs more
# digits than the ambient precision allows.
text = format(value, "f")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

format(value, "f") is unbounded in output size, and the input is attacker-controlled template text, so this looks like a memory-exhaustion hazard on job creation.

Decimal("1e999999999") constructs fine — Emax/Emin from the context constrain arithmetic results, not construction from a string — and is_finite() is True, so the guard above does not stop it. format(d, "f") then has to materialize the full plain-notation expansion, i.e. on the order of 10^9 characters for that exponent (and likewise for a large negative exponent such as "1e-999999999"). That is a multi-gigabyte allocation, or an effectively unbounded CPU/memory spike well before that.

It is reachable from a plain template: a FLOAT task parameter range: ["1e999999999"] passes the template-layer check, since validate_float_fmtstring_field only does Decimal(str(value)) and imposes no exponent bound. The element then arrives here as a str on the instantiation target and gets expanded. The previous str()/normalize() formulations did not have this property — str(Decimal) keeps exponent notation and is O(digits) — so this is specific to the format(..., "f") approach.

Worth bounding the exponent before expanding, e.g. reject (or leave as text) when value.adjusted() or -value.as_tuple().exponent exceeds some sane limit, so the plain-notation length is capped by construction rather than by the input.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Confirmed and fixed. Measured: Decimal("1e999999999") is finite exactly as you say, and the expansion is exponent+1 characters — parsing range: ["1e100000000"] on the instantiation target cost 0.37s and 266 MiB peak RSS from 11 characters of template text, and the 1e999999999 case killed the probe process outright. The length is now computed from adjusted()/as_tuple().exponent before anything is materialized, bounded at the 1024-character cap TaskParameterStringValueAsJob already enforces on this field, and an element over it keeps its source text — "1e999999999" now parses in 0.01 ms at 42 MiB.

# The number denoted by '-0.0' is zero, which has no sign; openjd-rs
# renders it `0.0`.
negative = False
return f"{'-' if negative else ''}{integer}.{fraction}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Normalization can make an element longer than the 1024-char cap the field type enforces, so an input that validated before this change can now fail (or silently change type) at the same point.

Elements of TaskRangeList are Union[TaskParameterStringValueAsJob, int, float, Decimal] and TaskParameterStringValueAsJob carries StringConstraints(max_length=1024). format(value, "f") turns a short source text into its full plain-notation expansion, so a FLOAT element written as "1E+1030" — 7 characters, well under every existing limit and accepted by the template-layer check — becomes a 1033-character string here. That fails the str member of the union, and rather than erroring cleanly pydantic then falls through to the numeric members in lax mode, where float("1000…0.0") overflows to inf. Either way the result is wrong for a template that was previously valid: an error on a conforming template, or a rendered task value of inf.

This is distinct from the unbounded-expansion concern on the format(..., "f") line above (which needs an exponent in the billions to hurt): here the threshold is around 1e1024, so it is well within what a template can plausibly contain and it costs nothing to trigger. A single exponent/length bound before expanding would cover both.

Also worth a test: the existing coverage stops at "1E+30", which stays comfortably under the cap.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Confirmed and fixed, with one correction to the predicted mechanism. Measured on the instantiation target: the threshold is a 1025-character expansion, and "1E+1022" fell through the str member to int and rendered 1023 digits rather than inf, while "1E-1023" fell through to float, underflowed and rendered 0.0 — silently losing the value; mainline rendered both as their source text, so both were previously valid. Both directions are now bounded by that same 1024-character cap before expansion, "1E+1021" and "1E-1022" expand to exactly 1024 and are still normalized, and there are tests one past the cap and at it.

Rendering a <floatstring> range element with format(value, 'f') is unbounded
in the element's exponent, which review found has two reachable faces.

Unbounded expansion. Decimal construction from a string is not bounded by the
context -- Emax/Emin constrain arithmetic results, not construction -- so
Decimal('1e999999999') is finite and the is_finite() guard did not stop it.
The plain-notation length is exponent + 1, measured: 10**7 gives 10,000,001
characters. Parsing a FLOAT range of ['1e100000000'] on the instantiation
target took 0.37s and 266 MiB peak RSS from 11 characters of template text;
the 1e999999999 case is 10x that and killed the probe process. It is reachable
from an ordinary template, because the template layer accepts the element and
keeps it as a TaskParameterStringValue.

Silently exceeding the field cap, far lower down. A TaskRangeList element's str
member is TaskParameterStringValueAsJob, capped at 1024 characters. Measured:
'1E+1022' -- 7 characters, accepted before this branch -- expands to 1025 and
so fails that member, and pydantic then falls through to the numeric members,
rendering the element as a 1023-digit int. In the other direction '1E-1023'
expands to 1025, fails the same way, and renders 0.0, silently losing the
value. Both were valid text-rendering elements on mainline, which rendered
'1E+1030' as 1E+1030.

Bound the length from value.adjusted() and the exponent before materializing
anything. The limit is the field's own 1024-character cap rather than a new
number, because an expansion past it cannot be carried here as text at all, so
there is nothing to gain by producing one. An element over the bound keeps its
source text -- what this function already does for anything it cannot
normalize, and how such an element rendered before normalization was
introduced -- so there is no new rejection path and no new error type. The
bound is exact at the boundary: '1E+1021' and '1E-1022' expand to exactly 1024
and are still normalized.

After the fix '1e999999999' parses in 0.01ms at 42 MiB peak RSS.

Model suite 5521 -> 5527 passed (6 new tests, 24 skipped, 3 xfailed). The two
boundary tests fail against 2109e2f, rendering 1000...000 and 0.0. Conformance
3.4.1.1, 3.4.1.2, 3.4--float-parameter and expr1.3.4--float-passthrough all
pass.

Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant


Back | FazBrowse Home | New Git URL