Definition Reader #92

raphael-goetz · 2025-11-26T15:14:53Z

Resolves: #91

…finitions service into its own feature

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Pull request overview

Copilot reviewed 12 out of 14 changed files in this pull request and generated 19 comments.

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

src/flow_service/mod.rs

src/flow_definition/feature/version.rs

Copilot · 2025-11-27T15:15:49Z

src/flow_definition/mod.rs

+                Err(ReaderError::ReadFeatureError {
+                    path: self.path.to_string(),
+                    source: Box::new(err),
+                })


[nitpick] Potential information loss in error propagation. When an error occurs in read_feature_content, it's wrapped in another ReadFeatureError, creating nested error wrapping. This can make the original error harder to diagnose. Consider either:

Returning the error directly without additional wrapping

Flattening the error structure

Providing more context about which specific operation failed

The current structure creates double-logging and redundant error wrapping.

Suggested change

Err(ReaderError::ReadFeatureError {

path: self.path.to_string(),

source: Box::new(err),

})

Err(err)

Copilot · 2025-11-27T15:15:49Z

src/flow_service/mod.rs

+    pub async fn send(&self) {
+        self.update_data_types().await;
+        self.update_runtime_definitions().await;
+        self.update_flow_types().await;
+    }


The send method silently ignores all errors from the individual update methods. This makes it difficult for callers to know if the updates succeeded or failed. Consider:

Returning a Result type to indicate success/failure

Collecting errors and returning them

At minimum, add a return value indicating overall success

Example:

pub async fn send(&self) -> Result<(), Vec<String>> { let mut errors = Vec::new(); // Track errors from each update method // Return Err(errors) if any occurred if errors.is_empty() { Ok(()) } else { Err(errors) } }

Copilot · 2025-11-27T15:15:50Z

src/flow_definition/mod.rs

+                None => continue,
+            };
+
+            let flow_types =
+                match self.load_definitions_for_feature::<FlowType>(&path, "flow_type")? {
+                    Some(v) => v,
+                    None => continue,
+                };
+
+            let functions = match self.load_definitions_for_feature::<RuntimeFunctionDefinition>(
+                &path,
+                "runtime_definition",
+            )? {
+                Some(v) => v,
+                None => continue,


Inconsistent error handling strategy. In load_definitions_for_feature, when should_break is false and an error occurs, the method returns Ok(None). However, this means that if one of the three definition types (data_types, flow_types, or functions) fails to load, the entire feature is skipped (lines 126, 132, 140 all have continue when None is returned).

This could lead to silent data loss - if only data types fail to load but flow types succeed, the entire feature is skipped. Consider either:

Collecting partial features with whatever loaded successfully

Logging warnings more explicitly about which definition types failed

Returning more granular error information

Suggested change

None => continue,

};

let flow_types =

match self.load_definitions_for_feature::<FlowType>(&path, "flow_type")? {

Some(v) => v,

None => continue,

};

let functions = match self.load_definitions_for_feature::<RuntimeFunctionDefinition>(

&path,

"runtime_definition",

)? {

Some(v) => v,

None => continue,

None => {

log::warn!(

"Failed to load data_types for feature '{}', path: {}. Using empty list.",

feature_name,

path.display()

);

Vec::new()

}

};

let flow_types = match self

.load_definitions_for_feature::<FlowType>(&path, "flow_type")?

{

Some(v) => v,

None => {

log::warn!(

"Failed to load flow_types for feature '{}', path: {}. Using empty list.",

feature_name,

path.display()

);

Vec::new()

}

};

let functions = match self

.load_definitions_for_feature::<RuntimeFunctionDefinition>(

&path,

"runtime_definition",

)?

{

Some(v) => v,

None => {

log::warn!(

"Failed to load functions for feature '{}', path: {}. Using empty list.",

feature_name,

path.display()

);

Vec::new()

}

Copilot · 2025-11-27T15:15:53Z

src/flow_definition/feature/mod.rs

+
+#[derive(Deserialize, Debug, Clone)]
+pub struct Feature {
+    pub name: String,
+    pub data_types: Vec<DefinitionDataType>,
+    pub flow_types: Vec<FlowType>,


The Feature struct lacks documentation. Public structs should have doc comments explaining their purpose and fields:

/// Represents a feature containing related flow definitions. /// /// A feature groups together data types, flow types, and runtime function /// definitions that belong to the same logical feature or module. #[derive(Deserialize, Debug, Clone)] pub struct Feature { /// The name of the feature pub name: String, /// Data type definitions associated with this feature pub data_types: Vec<DefinitionDataType>, /// Flow type definitions associated with this feature pub flow_types: Vec<FlowType>, /// Runtime function definitions associated with this feature pub functions: Vec<RuntimeFunctionDefinition>, }

Suggested change

#[derive(Deserialize, Debug, Clone)]

pub struct Feature {

pub name: String,

pub data_types: Vec<DefinitionDataType>,

pub flow_types: Vec<FlowType>,

/// Represents a feature containing related flow definitions.

///

/// A feature groups together data types, flow types, and runtime function

/// definitions that belong to the same logical feature or module.

#[derive(Deserialize, Debug, Clone)]

pub struct Feature {

/// The name of the feature.

pub name: String,

/// Data type definitions associated with this feature.

pub data_types: Vec<DefinitionDataType>,

/// Flow type definitions associated with this feature.

pub flow_types: Vec<FlowType>,

/// Runtime function definitions associated with this feature.

Copilot · 2025-11-27T15:15:53Z

src/flow_service/mod.rs

+pub struct FlowUpdateService {
+    aquila_url: String,
+    data_types: Vec<DataType>,
+    runtime_definitions: Vec<RuntimeFunctionDefinition>,
+    flow_types: Vec<FlowType>,
+}
+
+impl FlowUpdateService {
+    /// Create a new FlowUpdateService instance from an Aquila URL and a definition path.
+    ///
+    /// This will read the definition files from the given path and initialize the service with the data types, runtime definitions, and flow types.
+    pub fn from_url(aquila_url: String, definition_path: &str) -> Self {
+        let mut data_types = Vec::new();
+        let mut runtime_definitions = Vec::new();
+        let mut flow_types = Vec::new();
+
+        let reader = Reader::configure(definition_path.to_string(), true, vec![], None);
+
+        let features = match reader.read_features() {
+            Ok(features) => features,
+            Err(error) => {
+                log::error!("Error occurred while reading definitions: {:?}", error);
+                panic!("Error occurred while reading definitions")
+            }
+        };
+
+        for feature in features {
+            data_types.append(&mut feature.data_types.clone());
+            flow_types.append(&mut feature.flow_types.clone());
+            runtime_definitions.append(&mut feature.functions.clone());
+        }
+
+        Self {
+            aquila_url,
+            data_types,
+            runtime_definitions,
+            flow_types,
+        }
+    }
+
+    pub fn with_flow_types(mut self, flow_types: Vec<FlowType>) -> Self {
+        self.flow_types = flow_types;
+        self
+    }
+
+    pub fn with_data_types(mut self, data_types: Vec<DataType>) -> Self {
+        self.data_types = data_types;
+        self
+    }
+
+    pub fn with_runtime_definitions(
+        mut self,
+        runtime_definitions: Vec<RuntimeFunctionDefinition>,
+    ) -> Self {
+        self.runtime_definitions = runtime_definitions;
+        self
+    }
+
+    pub async fn send(&self) {
+        self.update_data_types().await;
+        self.update_runtime_definitions().await;
+        self.update_flow_types().await;
+    }
+
+    async fn update_data_types(&self) {
+        if self.data_types.is_empty() {
+            log::info!("No data types to update");
+            return;
+        }
+
+        log::info!("Updating the current DataTypes!");
+        let mut client = match DataTypeServiceClient::connect(self.aquila_url.clone()).await {
+            Ok(client) => {
+                log::info!("Successfully connected to the DataTypeService");
+                client
+            }
+            Err(err) => {
+                log::error!("Failed to connect to the DataTypeService: {:?}", err);
+                return;
+            }
+        };
+
+        let request = DataTypeUpdateRequest {
+            data_types: self.data_types.clone(),
+        };
+
+        match client.update(request).await {
+            Ok(response) => {
+                log::info!(
+                    "Was the update of the DataTypes accepted by Sagittarius? {}",
+                    response.into_inner().success
+                );
+            }
+            Err(err) => {
+                log::error!("Failed to update data types: {:?}", err);
+            }
+        }
+    }
+
+    async fn update_runtime_definitions(&self) {
+        if self.runtime_definitions.is_empty() {
+            log::info!("No runtime definitions to update");
+            return;
+        }
+
+        log::info!("Updating the current RuntimeDefinitions!");
+        let mut client =
+            match RuntimeFunctionDefinitionServiceClient::connect(self.aquila_url.clone()).await {
+                Ok(client) => {
+                    log::info!("Connected to RuntimeFunctionDefinitionService");
+                    client
+                }
+                Err(err) => {
+                    log::error!(
+                        "Failed to connect to RuntimeFunctionDefinitionService: {:?}",
+                        err
+                    );
+                    return;
+                }
+            };
+
+        let request = RuntimeFunctionDefinitionUpdateRequest {
+            runtime_functions: self.runtime_definitions.clone(),
+        };
+
+        match client.update(request).await {
+            Ok(response) => {
+                log::info!(
+                    "Was the update of the RuntimeFunctionDefinitions accepted by Sagittarius? {}",
+                    response.into_inner().success
+                );
+            }
+            Err(err) => {
+                log::error!("Failed to update runtime function definitions: {:?}", err);
+            }
+        }
+    }
+
+    async fn update_flow_types(&self) {
+        if self.flow_types.is_empty() {
+            log::info!("No FlowTypes to update!");
+            return;
+        }
+
+        log::info!("Updating the current FlowTypes!");
+        let mut client = match FlowTypeServiceClient::connect(self.aquila_url.clone()).await {
+            Ok(client) => {
+                log::info!("Connected to FlowTypeService!");
+                client
+            }
+            Err(err) => {
+                log::error!("Failed to connect to FlowTypeService: {:?}", err);
+                return;
+            }
+        };
+
+        let request = FlowTypeUpdateRequest {
+            flow_types: self.flow_types.clone(),
+        };
+
+        match client.update(request).await {
+            Ok(response) => {
+                log::info!(
+                    "Was the update of the FlowTypes accepted by Sagittarius? {}",
+                    response.into_inner().success
+                );
+            }
+            Err(err) => {
+                log::error!("Failed to update flow types: {:?}", err);
+            }
+        }
+    }
+}


The FlowUpdateService lacks test coverage. Given that other modules in this repository have comprehensive test suites, this new service should have tests covering:

Successful initialization from a definition path

Handling errors during definition reading

Builder pattern methods (with_flow_types, with_data_types, with_runtime_definitions)

The send method and individual update methods

Connection failures and update failures

Consider adding tests using mock gRPC clients or integration tests.

Copilot · 2025-11-27T15:15:54Z

src/flow_definition/feature/version.rs

+pub trait HasVersion {
+    fn version(&self) -> &Option<Version>;
+    fn version_mut(&mut self) -> &mut Option<Version>;
+
+    fn normalize_version(&mut self) {
+        self.version_mut().get_or_insert(Version {
+            major: 0,
+            minor: 0,
+            patch: 0,
+        });
+    }
+
+    fn is_accepted(&self, filter: &Option<Version>) -> bool {
+        filter
+            .as_ref()
+            .is_none_or(|v| self.version().as_ref() == Some(v))
+    }
+}
+
+impl HasVersion for DefinitionDataType {
+    fn version(&self) -> &Option<Version> {
+        &self.version
+    }
+
+    fn version_mut(&mut self) -> &mut Option<Version> {
+        &mut self.version
+    }
+}
+
+impl HasVersion for FlowType {
+    fn version(&self) -> &Option<Version> {
+        &self.version
+    }
+
+    fn version_mut(&mut self) -> &mut Option<Version> {
+        &mut self.version
+    }
+}
+
+impl HasVersion for RuntimeFunctionDefinition {
+    fn version(&self) -> &Option<Version> {
+        &self.version
+    }
+
+    fn version_mut(&mut self) -> &mut Option<Version> {
+        &mut self.version
+    }
+}


The HasVersion trait and its implementations lack test coverage. Consider adding tests to verify:

normalize_version correctly sets default version (0.0.0) when None

normalize_version doesn't overwrite existing versions

is_accepted correctly filters based on version

is_accepted returns true when filter is None

All three implementations work correctly for their respective types

Add tests in a #[cfg(test)] module.

Copilot · 2025-11-27T15:15:54Z

src/flow_definition/mod.rs

+pub struct Reader {
+    should_break: bool,
+    accepted_features: Vec<String>,
+    accepted_version: Option<Version>,
+    path: String,
 }


The Reader struct and its public methods lack documentation. Public APIs should have doc comments explaining their purpose, parameters, and behavior. Consider adding:

/// A reader for parsing flow definition files from a directory structure. /// /// The Reader walks through a directory containing feature definitions and /// parses JSON files for data types, flow types, and runtime function definitions. pub struct Reader { // ... }

Also add documentation for the public methods configure and read_features.

Copilot · 2025-11-27T15:15:54Z

src/flow_definition/mod.rs

+    pub fn configure(
+        path: String,
+        should_break: bool,
+        accepted_features: Vec<String>,
+        accepted_version: Option<Version>,
+    ) -> Self {


The configure method lacks documentation explaining its parameters. Add doc comments:

/// Creates a new Reader instance with the specified configuration. /// /// # Parameters /// * `path` - The root path to the directory containing feature definitions /// * `should_break` - If true, stops on first error; if false, skips errors and continues /// * `accepted_features` - If not empty, only features in this list will be loaded /// * `accepted_version` - If specified, only definitions matching this version will be loaded pub fn configure(

raphael-goetz added 2 commits November 26, 2025 16:13

feat: added flow_definition as new definition reader and moved the de…

b1e80d2

…finitions service into its own feature

ref: cargo fmt & clippy

8f6bdd9

raphael-goetz requested a review from Copilot November 26, 2025 15:14

Copilot started reviewing on behalf of raphael-goetz November 26, 2025 15:15 View session

Copilot finished reviewing on behalf of raphael-goetz November 26, 2025 15:17

Copilot AI reviewed Nov 26, 2025

View reviewed changes

raphael-goetz added 3 commits November 27, 2025 16:06

dependencies: updated to tucana 0.0.42

cae5b07

feat: cleanup + Version check trait

f31cb1b

ref: cargo clippy

330f066

raphael-goetz requested a review from Copilot November 27, 2025 15:07

Copilot started reviewing on behalf of raphael-goetz November 27, 2025 15:08 View session

Copilot finished reviewing on behalf of raphael-goetz November 27, 2025 15:12

Copilot AI reviewed Nov 27, 2025

View reviewed changes

raphael-goetz merged commit e010490 into main Nov 27, 2025
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Definition Reader #92

Definition Reader #92

Uh oh!

raphael-goetz commented Nov 26, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Nov 27, 2025

Uh oh!

Copilot AI Nov 27, 2025

Uh oh!

Copilot AI Nov 27, 2025

Uh oh!

Copilot AI Nov 27, 2025

Uh oh!

Copilot AI Nov 27, 2025

Uh oh!

Copilot AI Nov 27, 2025

Uh oh!

Copilot AI Nov 27, 2025

Uh oh!

Copilot AI Nov 27, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Definition Reader #92

Definition Reader #92

Uh oh!

Conversation

raphael-goetz commented Nov 26, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants