Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adds /vehiclestatuses endpoint to fms-server #32

Conversation

eriksven
Copy link
Contributor

This is an initial implementation of the '/vehiclestatuses' endpoint in the fms-server.

@eriksven eriksven force-pushed the feature/vehiclestatuses-endpoint branch 2 times, most recently from 585607f to 03a0660 Compare December 14, 2023 10:46
@eriksven eriksven force-pushed the feature/vehiclestatuses-endpoint branch from 03a0660 to 447f399 Compare December 15, 2023 10:24
@eriksven eriksven requested a review from sophokles73 December 15, 2023 10:31
@@ -30,7 +30,7 @@ readme.workspace = true

[dependencies]
async-trait = "0.1"
axum = "0.6"
axum = "0.7.2"
Copy link
Contributor

Choose a reason for hiding this comment

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

FMPOV we should not pin the version to a patch level but instead to the minor version only. That will make it easier for cargo to find a version that suits multiple components.

Comment on lines 155 to 184
let start_parameter = params.get("starttime");
let stop_parameter = params.get("stoptime");


let latest_only = match parse_latest_only(&params) {
Ok(value) => value,
Err(status) => return Err(status),
};

if start_parameter.is_none() && latest_only.is_none() {
// rFMS makes it mandatory to either supply the starttime or latestOnly
return Err(StatusCode::BAD_REQUEST);
}

if latest_only.is_some() && (start_parameter.is_some() || stop_parameter.is_some()) {
// rFMS does not allow to set latestOnly and and time at the same time
return Err(StatusCode::BAD_REQUEST);
}

let start_time = start_parameter.map_or(0, |text| {
DateTime::<Utc>::from_str(text).map_or(0, |time| time.timestamp())
});

let stop_time = stop_parameter
.map_or_else(|| Utc::now().timestamp(), |text| {
DateTime::<Utc>::from_str(text).map_or_else(|_| Utc::now().timestamp(), |time| time.timestamp())
});

let vin = params.get("vin");
let trigger_filter = params.get("triggerFilter");
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this query parameter checking/parsing something that is needed for the other endpoints as well? If so, can we reuse/share this code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that it is something that can be shared. My idea was to add this sharing in a later PR and limit the scope of this PR to the initial addition of the /vehiclestatuses endpoint.
Should I add the sharing to this PR instead?

@@ -515,4 +515,765 @@ pub struct ErrorObject {

}

#[derive(Debug, Clone, PartialEq, serde::Serialize, serde::Deserialize)]
Copy link
Contributor

Choose a reason for hiding this comment

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

this module seems to be quite large. Can we distribute the type to multiple modules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we could distribute it. However, I was not sure where to split because this is basically the domain model used within rFMS. I will try to split this along the lines of the different endpoints using parts of the model.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I guess that makes sense. Putting common types into one module and then have the endpoint specific types refer to these ...

Comment on lines 78 to 81
let query_parameters = match parse_query_parameters(&params) {
Ok(value) => value,
Err(status) => return Err(status),
};
Copy link
Contributor

Choose a reason for hiding this comment

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

you should be able to write this as just

let query_parameters = parse_query_parameters(&params)?;


influx_server
.get_vehicleposition(start_time, stop_time, vin, trigger_filter, latest_only)
.get_vehicleposition(query_parameters.start_time, query_parameters.stop_time, query_parameters.vin, query_parameters.trigger_filter, query_parameters.latest_only)
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't it make more sense to simply pass in &query_parameters?



let latest_only = match parse_latest_only(&params) {
let query_parameters = match parse_query_parameters(&params) {
Copy link
Contributor

Choose a reason for hiding this comment

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

see above

influx_server
.get_vehiclesstatuses(start_time, stop_time, vin, trigger_filter, latest_only)
.get_vehiclesstatuses(query_parameters.start_time, query_parameters.stop_time, query_parameters.vin, query_parameters.trigger_filter, query_parameters.latest_only)
Copy link
Contributor

Choose a reason for hiding this comment

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

see above

refactors model into separate files too
@eriksven eriksven force-pushed the feature/vehiclestatuses-endpoint branch from 71660c8 to 2ff2497 Compare December 18, 2023 10:51
@eriksven
Copy link
Contributor Author

@sophokles73 I changed your findings

Copy link
Contributor

@sophokles73 sophokles73 left a comment

Choose a reason for hiding this comment

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

LGTM

@eriksven eriksven merged commit d7977d0 into eclipse-sdv-blueprints:main Dec 18, 2023
sophokles73 added a commit to SoftwareDefinedVehicle/fleet-management-fork that referenced this pull request Jan 16, 2024
Updated Cargo.lock and transitive dependencies legal docs.
sophokles73 added a commit to SoftwareDefinedVehicle/fleet-management-fork that referenced this pull request Jan 16, 2024
Updated Cargo.lock and transitive dependencies legal docs.
eriksven pushed a commit that referenced this pull request Jan 16, 2024
Updated Cargo.lock and transitive dependencies legal docs.
vivekpandey02 pushed a commit to vivekpandey02/fleet-management that referenced this pull request Jan 30, 2024
Updated Cargo.lock and transitive dependencies legal docs.
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.

2 participants