[mock_uss] Add flight_planning API implementation to mock_uss #287

BenjaminPelletier · 2023-10-25T00:39:29Z

This PR adds an implementation of the flight_planning automated testing API to mock_uss by simply calling the existing flight planning handlers for the legacy scd interface. The interface used in uss_qualifier's CI automated tests is switched to this new API for all but one of the mock_uss instances.

The overall state of mock_uss flight planning handling is still left in need of some attention after this PR, but I hope to follow up with some improvements.

mickmis

LGTM - comments are mostly questions.

mickmis · 2023-10-26T12:38:50Z

monitoring/monitorlib/geo.py

+            kwargs["altitude_lower"] = self.altitude_lower.to_flight_planning_api()
+        if self.altitude_upper:
+            kwargs["altitude_upper"] = self.altitude_upper.to_flight_planning_api()
+        return fp_api.Volume3D(**kwargs)


I'm really not sure of myself here, but shouldn't this use ImplicitDict.parse in order for the fields to be correctly set?

When an ImplicitDict subclass is constructed with keyword arguments, those fields are set to the values specified without type checking (it's basically this). As long as the type for each field is known to be correct (and there's no need for a deep clone), constructing with keyword arguments is the most efficient way to produce an ImplicitDict-subclass instance. Parsing has a number of additional features such as type checking and type conversion (e.g., a string will be automatically wrapped into a StringBasedDateTime), so if the entire tree of fields in your input parameters may have one or more instances of incorrect types, parsing will fix that. But, parsing is much more expensive than direct construction. In case you didn't know, MyClass(foo=1, bar="baz") is equivalent to MyClass(**{"foo": 1, "bar": "baz"})

Thank you for the clarifications!

In case you didn't know, MyClass(foo=1, bar="baz") is equivalent to MyClass(**{"foo": 1, "bar": "baz"})

Indeed I thought those were different. Probably because my IDE performs type checks and highlight issues in the former but not the latter.

mickmis · 2023-10-26T12:39:44Z

monitoring/monitorlib/geotemporal.py

+            kwargs["time_start"] = fp_api.Time(value=self.time_start)
+        if self.time_end:
+            kwargs["time_end"] = fp_api.Time(value=self.time_end)
+        return fp_api.Volume4D(**kwargs)


Same remark here about possible need to use ImplicitDict.parse

mickmis · 2023-10-26T12:42:06Z

monitoring/monitorlib/geotemporal.py

+        if self.time_start:
+            kwargs["time_start"] = fp_api.Time(value=self.time_start)
+        if self.time_end:
+            kwargs["time_end"] = fp_api.Time(value=self.time_end)


Is it better here to set time_start and time_end to None?
I asked this question to myself a couple of times and I could not be sure.

The advantage of declaring a field as "Optional" but providing a default is that it is not necessary to check for field presence; e.g.:

class Foo1(ImplicitDict): bar: Optional[str] = None def baz(f: Foo1): if f.bar is None: # <-- this is ok [...] class Foo2(ImplicitDict): bar: Optional[str] def baz(f: Foo2): if "bar" in f and f.bar is None: # <-- an additional check for field is necessary [...]

When already given an object, setting to None or not controls how the object renders into JSON:

class Foo(ImplicitDict): bar: Optional[int] baz: str json.dumps(Foo(bar=None, baz="biz")) >> {"bar":null,"baz":"biz"} json.dumps(Foo(baz="biz")) >> {"baz":"biz"}

The latter is almost always going to be better than the former. But, the former doesn't hurt unless you're trying to produce a pretty JSON/YAML output.

Alright - thanks for the explanations :)

mickmis · 2023-10-26T12:46:51Z

monitoring/mock_uss/scdsc/routes_scdsc.py

@@ -27,7 +27,7 @@ def scdsc_get_operational_intent_details(entityid: str):
    tx = db.value
    flight = None
    for f in tx.flights.values():
-        if f.op_intent.reference.id == entityid:
+        if f and f.op_intent.reference.id == entityid:


nit: If f does not exist, that means a null object was inserted in the DB, doesn't that mean that we have a bug somewhere?

edit: actually I assume it is for cases where it's a "placeholder for a new flight"

Correct -- None is a placeholder for new flight

mickmis · 2023-10-26T12:50:28Z

monitoring/mock_uss/scdsc/routes_injection.py

+                # FlightRecord was a true existing flight
+                log(f"Releasing placeholder for flight_id {flight_id}")
+                tx.flights[flight_id].locked = False
+            else:
+                # FlightRecord was just a placeholder for a new flight
+                log(f"Releasing lock on existing flight_id {flight_id}")
+                del tx.flights[flight_id]


Suggested change

# FlightRecord was a true existing flight

log(f"Releasing placeholder for flight_id {flight_id}")

tx.flights[flight_id].locked = False

else:

# FlightRecord was just a placeholder for a new flight

log(f"Releasing lock on existing flight_id {flight_id}")

del tx.flights[flight_id]

# FlightRecord was a true existing flight

log(f"Releasing lock on existing flight_id {flight_id}")

tx.flights[flight_id].locked = False

else:

# FlightRecord was just a placeholder for a new flight

log(f"Releasing placeholder for flight_id {flight_id}")

del tx.flights[flight_id]

Done, thanks

mickmis · 2023-10-26T13:02:11Z

monitoring/mock_uss/flight_planning/routes.py

+
+def injection_status() -> Tuple[dict, int]:
+    return (
+        api.StatusResponse(


flask.jsonify(...) isn't needed here? (I'm not too familiar with the framework)

flask.jsonify turns a dict into a Flask Response. This function is just computing the content, and the handler (view function) itself turns it into a response on line 43.

This function is just computing the content, and the handler (view function) itself turns it into a response on line 43.

Missed that - OK

mickmis · 2023-10-26T13:04:31Z

monitoring/mock_uss/flight_planning/routes.py

+def flight_planning_v1_delete_flight(flight_plan_id: str) -> Tuple[str, int]:
+    """Implements flight deletion in SCD automated testing injection API."""
+    scd_resp, code = delete_flight(flight_plan_id)
+    if code != 200:


Why raising an error and not forwarding the error code? E.g. if we get a 404 I assume that we want to respond the same.

In this particular handler, a missing flight is reflected as a Failure to perform the requested operation rather than indicating that a RESTful resource is missing, so 200 is returned (indicating successful attempt of the request accompanied by a nominal indication of the Failure outcome). That's probably not ideal API construction, but I think the code should work for what we currently have. I'll take the note on API construction.

* Add flight_planning API implementation to mock_uss * Address comments 1199553

…ss#287) * Add flight_planning API implementation to mock_uss * Address comments 1199553

Add flight_planning API implementation to mock_uss

80347c8

BenjaminPelletier marked this pull request as ready for review October 25, 2023 01:48

mickmis approved these changes Oct 26, 2023

View reviewed changes

BenjaminPelletier added 2 commits October 26, 2023 15:57

Merge remote-tracking branch 'interuss/main' into flight_planner-impl

ee8b43a

Address comments

299f250

BenjaminPelletier merged commit 1199553 into interuss:main Oct 26, 2023
8 checks passed

BenjaminPelletier deleted the flight_planner-impl branch October 26, 2023 18:03

github-actions bot added a commit that referenced this pull request Oct 26, 2023

[mock_uss] Add flight_planning API implementation to mock_uss (#287)

f035791

* Add flight_planning API implementation to mock_uss * Address comments 1199553

github-actions bot added a commit to openskies-sh/monitoring that referenced this pull request Oct 27, 2023

[mock_uss] Add flight_planning API implementation to mock_uss (interu…

a1fb7e7

…ss#287) * Add flight_planning API implementation to mock_uss * Address comments 1199553

BenjaminPelletier mentioned this pull request Nov 8, 2023

Update flight planning automated testing with updated API #221

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mock_uss] Add flight_planning API implementation to mock_uss #287

[mock_uss] Add flight_planning API implementation to mock_uss #287

BenjaminPelletier commented Oct 25, 2023

mickmis left a comment

mickmis Oct 26, 2023

BenjaminPelletier Oct 26, 2023

mickmis Oct 27, 2023

mickmis Oct 26, 2023

mickmis Oct 26, 2023

BenjaminPelletier Oct 26, 2023

mickmis Oct 27, 2023

mickmis Oct 26, 2023

BenjaminPelletier Oct 26, 2023

mickmis Oct 26, 2023

BenjaminPelletier Oct 26, 2023

mickmis Oct 26, 2023

BenjaminPelletier Oct 26, 2023

mickmis Oct 27, 2023

mickmis Oct 26, 2023

BenjaminPelletier Oct 26, 2023

[mock_uss] Add flight_planning API implementation to mock_uss #287

[mock_uss] Add flight_planning API implementation to mock_uss #287

Conversation

BenjaminPelletier commented Oct 25, 2023

mickmis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment