Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 21 additions & 10 deletions crates/openjd-model/src/template/parameters.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,19 +79,30 @@ fn strip_type_field(mut value: serde_json::Value) -> serde_json::Value {
value
}

/// Split a parameter definition into the tag as written, the tag folded for
Comment thread
leongdl marked this conversation as resolved.
/// matching, and the body with `type` removed.
///
/// Both parameter kinds share this so §2's case rule has one home. The fold is
/// ASCII deliberately: `to_uppercase` maps U+0131 to `I`, making `ıNT` a spelling
/// of `INT`.
pub(super) fn split_type_tag<E: serde::de::Error>(
value: serde_json::Value,
missing: &str,
) -> Result<(String, String, serde_json::Value), E> {
let written = value
.get("type")
.and_then(|v| v.as_str())
.ok_or_else(|| E::custom(missing))?
.to_string();
let folded = written.to_ascii_uppercase();
Ok((written, folded, strip_type_field(value)))
}

impl<'de> serde::Deserialize<'de> for JobParameterDefinition {
fn deserialize<D: serde::Deserializer<'de>>(deserializer: D) -> Result<Self, D::Error> {
let value = serde_json::Value::deserialize(deserializer)?;
let type_str = value
.get("type")
.and_then(|v| v.as_str())
.ok_or_else(|| {
serde::de::Error::custom("missing 'type' field in parameter definition")
})?
.to_string();

let normalized = type_str.to_uppercase();
let stripped = strip_type_field(value);
let (type_str, normalized, stripped) =
split_type_tag(value, "missing 'type' field in parameter definition")?;

match normalized.as_str() {
"STRING" => serde_json::from_value(stripped)
Expand Down
130 changes: 127 additions & 3 deletions crates/openjd-model/src/template/parse.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,13 @@

use std::str::FromStr;

use crate::error::{path_field, ModelError, ValidationErrors};
use crate::error::{path_field, path_index, ModelError, PathElement, ValidationErrors};
use crate::template::constrained_strings::ExtensionName;
use crate::template::validation as validate;
use crate::template::{EnvironmentTemplate, JobTemplate};
use crate::types::{
CallerLimits, Extensions, ModelExtension, SpecificationRevision, TemplateSpecificationVersion,
ValidationContext,
CallerLimits, Extensions, JobParameterType, ModelExtension, SpecificationRevision,
TaskParameterType, TemplateSpecificationVersion, ValidationContext,
};

/// Document format.
Expand Down Expand Up @@ -183,6 +183,115 @@ fn validate_extensions_list(
result
}

/// A parameter type name as the template author spelled it, with the error path
/// it should be reported at.
type WrittenTypeName = (Vec<PathElement>, String);

/// Collect the `type` string of every parameter definition, as written, from the
/// raw document.
///
/// The two kinds are collected separately so each can be checked against its own
/// type table. Deserialization matches the tag case-blind (§2 makes type names
/// case-insensitive under EXPR), so the author's spelling does not survive it, and
/// the effective extension set does not exist until after it. This runs before the
/// document is moved into `serde_json::from_value`, and
/// `check_type_name_canonical_case` consumes the result once the extensions are
/// known.
///
/// Anything that is not an object, or whose `type` is absent or not a string, is
/// skipped and left for deserialization to report. This function never errors.
fn collect_written_type_names(
template: &serde_json::Value,
job_names: &mut Vec<WrittenTypeName>,
task_names: &mut Vec<WrittenTypeName>,
) {
fn collect_from(
defs: Option<&serde_json::Value>,
path: &[PathElement],
out: &mut Vec<WrittenTypeName>,
) {
let Some(items) = defs.and_then(|v| v.as_array()) else {
return;
};
for (i, item) in items.iter().enumerate() {
if let Some(written) = item.get("type").and_then(|v| v.as_str()) {
out.push((path_index(path, i), written.to_string()));
}
}
}

let root: Vec<PathElement> = vec![];
collect_from(
template.get("parameterDefinitions"),
&path_field(&root, "parameterDefinitions"),
job_names,
);

let Some(steps) = template.get("steps").and_then(|v| v.as_array()) else {
return;
};
for (i, step) in steps.iter().enumerate() {
let space_path = path_field(
&path_index(&path_field(&root, "steps"), i),
"parameterSpace",
);
collect_from(
step.get("parameterSpace")
.and_then(|s| s.get("taskParameterDefinitions")),
&path_field(&space_path, "taskParameterDefinitions"),
task_names,
);
}
}

/// Reject a parameter type name that names a real type but is not spelled the way
/// the specification spells it, unless the EXPR extension is in effect.
///
/// §2: "When the `EXPR` extension is enabled, job parameter and task parameter type
/// names become case-insensitive." Without it they are case-sensitive, so
/// `string` is not a spelling of `STRING`.
///
/// A spelling that names no type at all is left alone: deserialization already
/// reported it as an unknown type, and it would be reported twice otherwise.
fn check_type_name_canonical_case(
job_names: &[WrittenTypeName],
task_names: &[WrittenTypeName],
extensions: &Extensions,
errors: &mut ValidationErrors,
) {
if extensions.contains(&ModelExtension::Expr) {
return;
}
for (path, written) in job_names {
if let Some(canonical) = JobParameterType::from_spec_str(written).map(|t| t.as_spec_str()) {
if canonical != written {
errors.add(
path,
canonical_case_message("parameter", written, canonical),
);
}
}
}
for (path, written) in task_names {
if let Some(canonical) = TaskParameterType::from_spec_str(written).map(|t| t.as_spec_str())
{
if canonical != written {
errors.add(
path,
canonical_case_message("task parameter", written, canonical),
);
}
}
}
}

fn canonical_case_message(kind: &str, written: &str, canonical: &str) -> String {
format!(
"{kind} type '{written}' is not recognized. Type names are case-sensitive \
without the EXPR extension; expected '{canonical}'."
)
}

/// Decode and validate a job template from a YAML value.
pub fn decode_job_template(
template: serde_json::Value,
Expand Down Expand Up @@ -217,6 +326,11 @@ pub fn decode_job_template(
)));
}

// Collect the type names as written, before `template` is moved below.
let mut job_type_names = Vec::new();
let mut task_type_names = Vec::new();
collect_written_type_names(&template, &mut job_type_names, &mut task_type_names);

let jt: JobTemplate = match version.revision() {
// Future revisions may decode into a different struct layout.
// Making the match explicit now localizes the dispatch point.
Expand All @@ -231,6 +345,7 @@ pub fn decode_job_template(
let mut errors = ValidationErrors::default();
let extensions =
validate_extensions_list(jt.extensions.as_deref(), supported_extensions, &mut errors);
check_type_name_canonical_case(&job_type_names, &task_type_names, &extensions, &mut errors);
errors.into_result("JobTemplate")?;

// Route to the revision-specific validation pipeline via the
Expand Down Expand Up @@ -274,6 +389,14 @@ pub fn decode_environment_template(
)));
}

// An environment template's `parameterDefinitions` is the same
// `JobParameterDefinition` union a job template's is, so §2's casing rule
// applies here too. Collected before `template` is moved below. There are no
// steps on this document, so no task parameter names.
let mut job_type_names = Vec::new();
let mut task_type_names = Vec::new();
collect_written_type_names(&template, &mut job_type_names, &mut task_type_names);

let et: EnvironmentTemplate = match version.revision() {
// Future revisions may decode into a different struct layout.
// Making the match explicit now localizes the dispatch point,
Expand All @@ -288,6 +411,7 @@ pub fn decode_environment_template(
let mut errors = ValidationErrors::default();
let extensions =
validate_extensions_list(et.extensions.as_deref(), supported_extensions, &mut errors);
check_type_name_canonical_case(&job_type_names, &task_type_names, &extensions, &mut errors);
errors.into_result("EnvironmentTemplate")?;

let ctx = ValidationContext::with_extensions(version.revision(), extensions);
Expand Down
42 changes: 39 additions & 3 deletions crates/openjd-model/src/template/task_parameters.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,54 @@ use crate::format_string::FormatString;
use serde::Deserialize;

/// §3.4.1 TaskParameterDefinition — discriminated union on `type`.
#[derive(Debug, Clone, Deserialize)]
#[serde(tag = "type")]
///
/// The `type` tag is matched case-blind, mirroring [`JobParameterDefinition`]:
Comment thread
leongdl marked this conversation as resolved.
/// §2 makes both kinds of type name case-insensitive under the EXPR extension,
/// and a non-canonical spelling without EXPR is rejected during validation,
/// where the effective extension set is known.
///
/// [`JobParameterDefinition`]: super::parameters::JobParameterDefinition
#[derive(Debug, Clone)]
#[allow(non_camel_case_types)]
pub enum TaskParameterDefinition {
INT(IntTaskParameterDefinition),
FLOAT(FloatTaskParameterDefinition),
STRING(StringTaskParameterDefinition),
PATH(PathTaskParameterDefinition),
#[serde(rename = "CHUNK[INT]")]
CHUNK_INT(ChunkIntTaskParameterDefinition),
}

impl<'de> serde::Deserialize<'de> for TaskParameterDefinition {
fn deserialize<D: serde::Deserializer<'de>>(deserializer: D) -> Result<Self, D::Error> {
let value = serde_json::Value::deserialize(deserializer)?;
let (type_str, normalized, stripped) = super::parameters::split_type_tag(
value,
"missing 'type' field in task parameter definition",
)?;

match normalized.as_str() {
"INT" => serde_json::from_value(stripped)
.map(Self::INT)
.map_err(serde::de::Error::custom),
"FLOAT" => serde_json::from_value(stripped)
.map(Self::FLOAT)
.map_err(serde::de::Error::custom),
"STRING" => serde_json::from_value(stripped)
.map(Self::STRING)
.map_err(serde::de::Error::custom),
"PATH" => serde_json::from_value(stripped)
.map(Self::PATH)
.map_err(serde::de::Error::custom),
"CHUNK[INT]" => serde_json::from_value(stripped)
.map(Self::CHUNK_INT)
.map_err(serde::de::Error::custom),
_ => Err(serde::de::Error::custom(format!(
"unknown task parameter type: '{type_str}'"
))),
}
}
}

impl TaskParameterDefinition {
pub fn task_param_type(&self) -> crate::types::TaskParameterType {
use crate::types::TaskParameterType;
Expand Down
2 changes: 2 additions & 0 deletions crates/openjd-model/tests/integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ mod test_merge_job_parameters;
mod test_misc_v2023_09;
#[path = "integration/test_model_profile.rs"]
mod test_model_profile;
#[path = "integration/test_param_type_name_case.rs"]
mod test_param_type_name_case;
#[path = "integration/test_parameter_space.rs"]
mod test_parameter_space;
#[path = "integration/test_parse.rs"]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -805,8 +805,10 @@ fn test_lowercase_range_expr_with_expr() {
));
}

// Note: Python tests for case-insensitive types failing without EXPR are not ported
// because the Rust implementation accepts case-insensitive types by default.
// The Python counterparts that assert case-insensitive type names FAIL without EXPR
// live in test_param_type_name_case.rs, which covers all four combinations of
// extension state by spelling, for job parameters, task parameters and environment
// templates.

// ============================================================
// LIST[FLOAT] parameter — additional tests
Expand Down
Loading
Loading