split component sequence validator by xiangu-batw · Pull Request #291 · eclipse-score/tooling

xiangu-batw · 2026-06-24T09:38:59Z

No description provided.

hoe-jo · 2026-06-29T12:42:33Z

            &mut errors,
-            validate_component_sequence(
-                component,
+            validate_component_sequence(component, sequence, Errors::default()),


What is the idea of this Errors::default() ? Do we need it?

// validators: change signature from
pub fn validate_component_internal_api(
component_diagram: &ComponentDiagramArchitecture,
internal_api_diagram: &InternalApiIndex,
errors: Errors, // ← remove this
) -> Errors

// to
pub fn validate_component_internal_api(
component_diagram: &ComponentDiagramArchitecture,
internal_api_diagram: &InternalApiIndex,
) -> Errors

hoe-jo · 2026-06-29T12:44:01Z

+        );
+        ran_validator = true;
+    }
+    if let (Some(component), Some(internal_api)) = (component.as_ref(), internal_api.as_ref()) {


change it here also from:

// before
merge_errors(
&mut errors,
validate_component_internal_api(component, internal_api, Errors::default()),
);

// after — intent is now explicit
merge_errors(&mut errors, validate_component_internal_api(component, internal_api));

hoe-jo · 2026-06-29T13:23:17Z

+    )
+}
+
+fn format_name_list(names: &BTreeSet<String>) -> String {


We use this snipped in each validator, should we move it into a format_helper?

// validation/core/src/validators/format_helpers.rs
use std::collections::BTreeSet;

pub(super) fn format_name_list(names: &BTreeSet) -> String {
if names.is_empty() {
return "".to_string();
}
names.iter().map(|n| format!(""{n}"")).collect::<Vec<_>>().join(", ")
}

pub(super) fn extract_method_name(method: &str) -> &str {
method.split('(').next().unwrap_or(method).trim()
}

the common functions are now in shared/

hoe-jo · 2026-06-29T13:38:01Z

These 3 test files in the test folder include nearly the same fixtures, would it make sense to create a common fixture folder? e.g.

// test/fixtures.rs (only compiled under #[cfg(test)])
pub fn unit(alias: &str, interface_targets: &[&str]) -> LogicComponent { ... }
pub fn interface(alias: &str) -> LogicComponent { ... }
pub fn component_diagrams_with_entities(entities: Vec) -> ComponentDiagramInputs { ... }
pub fn internal_api_index(interfaces: Vec<(&str, Vec<&str>)>) -> InternalApiIndex {
let mut errors = Errors::default();
let idx = InternalApiIndex::build_index(&raw, &mut errors);
assert!(errors.is_empty(), "test fixture construction failed: {:?}", errors.messages);
idx
}

extracted the fixtures in fixtures.rs

hoe-jo · 2026-06-29T13:39:25Z

-            match relation.source_role.as_deref() {
-                Some("Required") => {
+            if relation.relation_type != ComponentRelationType::InterfaceBinding {
+                continue;


Hides inconsistencies, maybe add an debug output?

it filters out the irrelevant cases

hoe-jo · 2026-06-29T13:39:57Z

+                .find(|candidate| candidate.is_interface() && candidate.id == relation.target)
+                .map(|candidate| candidate.id.clone())
+            else {
+                continue;


Hides inconsistencies, maybe add debut output?

it filters out the irrelevant cases

hoe-jo · 2026-06-29T13:59:14Z

+    }
+}
+
+fn build_unit_bindings(


nearly same function is declared in component_sequence_validator as well (only different return type)
Can we move UnitInterfaces move to a shared module:
// shared: fn interface_entity_ids(diagram: &ComponentDiagramArchitecture) -> BTreeSet<&str>
// each validator calls it, builds its own map with correct return type

the common functions are now in shared/

hoe-jo · 2026-06-29T14:02:02Z

    if let (Some(component), Some(sequence)) = (component.as_ref(), sequence.as_ref()) {
        merge_errors(
            &mut errors,
-            validate_component_sequence(
-                component,
+            validate_component_sequence(component, sequence, Errors::default()),
+        );
+        ran_validator = true;
+    }
+    if let (Some(component), Some(internal_api)) = (component.as_ref(), internal_api.as_ref()) {
+        merge_errors(
+            &mut errors,
+            validate_component_internal_api(component, internal_api, Errors::default()),
+        );
+        ran_validator = true;
+    }
+    if let (Some(sequence), Some(internal_api)) = (sequence.as_ref(), internal_api.as_ref()) {
+        merge_errors(
+            &mut errors,
+            validate_sequence_internal_api(
                sequence,
-                internal_api.as_ref(),
+                internal_api,
+                component.as_ref(),
                Errors::default(),
            ),
        );


Shall we use the registry pattern here? So basically each validator registers itself and we don´t have to adapt the logic every time a new validator is added? Maybe something like:

// architectural_design.rs
let validators: &[Box<dyn Fn() -> Option>] = &[
Box::new(|| {
let (c, s) = (component.as_ref()?, sequence.as_ref()?);
Some(validate_component_sequence(c, s))
}),
Box::new(|| {
let (c, i) = (component.as_ref()?, internal_api.as_ref()?);
Some(validate_component_internal_api(c, i))
}),
Box::new(|| {
let (s, i) = (sequence.as_ref()?, internal_api.as_ref()?);
Some(validate_sequence_internal_api(s, i, component.as_ref()))
}),
];

let mut ran_validator = false;
for validator in validators {
if let Some(errs) = validator() {
merge_errors(&mut errors, errs);
ran_validator = true;
}
}

updated with registry pattern.

hoe-jo · 2026-06-29T14:12:07Z

-                _ => {}
+                EndpointRole::None => {}
            }
        }


nested loop, also used in the sequence_internal_api_validation. We could think of creating an "ID Map" (pre-built BTreeSet of interface IDs)

so maybe something like:

// Built once before outer loop — O(E):
let interface_ids: BTreeSet<&str> = diagram.entities.iter()
.filter(|e| e.is_interface())
.map(|e| e.id.as_str())
.collect();
// Contains: {"pkg.InterfaceA", "pkg.InterfaceB"}

// Inside per-unit, per-relation loop — O(log E):
if !interface_ids.contains(relation.target.as_str()) {
continue;
}
// relation.target is already the interface ID — use directly, no .find() needed
bindings.insert(relation.target.clone()); // single clone, not double

hoe-jo · 2026-07-02T14:39:48Z

+            )
+            .is_empty()
+        {
+            eprintln!("Warning: sequence call between units \"{}\" and \"{}\" for method \"{}\" has shared interface(s) {:?} but no matching required/provided roles in the component diagram",


route through log::

hoe-jo · 2026-07-02T15:12:31Z

+) {
+    if let Some(component_context) = component_context.as_ref() {
+        diagnostics.debug(|| {
+            "DEBUG: Sequence calls checked against related internal API interfaces:".to_string()


Double Debug (debug already prints debug)

hoe-jo · 2026-07-02T15:14:03Z

@@ -649,177 +313,10 @@ fn build_observed_call_contexts<'a>(
        .collect()
 }


Duplicate to sequence_internal_api_validator:403 -> Move to shared utils?

hoe-jo · 2026-07-02T15:14:50Z

+) {
+    diagnostics.debug(|| "Component interfaces checked against internal API:".to_string());
+    for interface_id in component_interface_ids {
+        diagnostics.debug(|| format!("  {interface_id}\n"));


Is trainling\n working here?

hoe-jo · 2026-07-02T15:14:54Z

+
+    diagnostics.debug(|| "Internal API interfaces available for component interfaces:".to_string());
+    for interface_id in internal_api_interface_ids {
+        diagnostics.debug(|| format!("  {interface_id}\n"));


Is trainling\n working here?

hoe-jo · 2026-07-02T15:17:07Z

-use crate::models::{
-    ComponentDiagramArchitecture, InternalApiIndex, InternalApiInterface, SequenceDiagramIndex,
+use super::shared::{
+    all_interfaces_for_alias, build_unit_bindings, format_name_list, intersect_interfaces,


all_interfaces_for_alias clones BTreeSet on every call:
could we return &BTreeSet with lifetime of UnitBindigs, and work with references only?

Good point. This would require changing SequenceCallContext to borrow interface sets instead of owning them, which in turn requires restructuring the current data model rather than a local optimization. Otherwise it will end up with a self-referential ownership pattern, which isn't supported by ordinary Rust ownership. I'd prefer to keep this PR as it is, and refactor the data model when adding public API support.

xiangu-batw requested review from LittleHuba, castler, hoe-jo, limdor and ramceb as code owners June 24, 2026 09:39

github-project-automation Bot added this to INF - Infrastructure Community Jun 24, 2026

xiangu-batw force-pushed the xian_split_validator branch 2 times, most recently from 0086ab4 to f60f7e6 Compare June 29, 2026 09:42

hoe-jo reviewed Jun 29, 2026

View reviewed changes

xiangu-batw force-pushed the xian_split_validator branch 4 times, most recently from b0c50bc to 6b0ec47 Compare July 2, 2026 02:46

hoe-jo reviewed Jul 2, 2026

View reviewed changes

xiangu-batw added 3 commits July 3, 2026 08:54

split component sequence validator

0a99e4a

split integration test

21dfff7

fix comments

8f19bd1

xiangu-batw force-pushed the xian_split_validator branch from 6b0ec47 to 8f19bd1 Compare July 3, 2026 05:48

		@@ -649,177 +313,10 @@ fn build_observed_call_contexts<'a>(
		.collect()
		}

Uh oh!

Conversation

xiangu-batw commented Jun 24, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants