Repository: stravalib/stravalib
Pull request #516 (2024-06-01 22:19:04)

Add: test for latlon values
@jsamoocha can you give a tip or help me trouble shoot this? i think having this test is good and am totally open to writing it in a better way. 

✅ Everything validates for summary activity if we have valid lat long numbers. 
✅ Nones are also acceptable and ingested into the model properly. 
❌ When stravalib gets an empty list, it fails. I understand generally why it's failing. but that means our validator is not working as expected and/or things are typed incorrectly. (when i debug the validator seems to work) 

A few notes:

▶️  we have [this validator here](https://github.com/stravalib/stravalib/blob/pydantic-v2/src/stravalib/model.py#L145) that lives in the LatLon object. 
▶️  then we [have this](https://github.com/stravalib/stravalib/blob/pydantic-v2/src/stravalib/model.py#L145) -- a method that lives in the model.py module.  

This method is called 4 times in module.py 

☑️  [in ActivityPhoto here](https://github.com/stravalib/stravalib/blob/pydantic-v2/src/stravalib/model.py#L605)
☑️  [in activityPhoto here](https://github.com/stravalib/stravalib/blob/pydantic-v2/src/stravalib/model.py#L690)
☑️  [In summarize segment here](https://github.com/stravalib/stravalib/blob/pydantic-v2/src/stravalib/model.py#L755)
☑️  [In ActivityPhoto here](https://github.com/stravalib/stravalib/blob/pydantic-v2/src/stravalib/model.py#L935)

It seems to me that we could create a single method and use it several times as a validator (if needed). 

## Type weirdness
But here is the real issue - When i look at the [generated model -- it expects ](https://github.com/stravalib/stravalib/blob/pydantic-v2/src/stravalib/strava_model.py#L227) a list[float] - None is not an option nor is an empty list. But we know Strava can return an empty list or have missing data.

In the LatLon object in [the model module](https://github.com/stravalib/stravalib/blob/pydantic-v2/src/stravalib/model.py#L254) we ingest lat/lon but there is no type override allowing for None values. But i think we had discussed wanting to avoid type overrides where possible? 

regardless, I tried to override the field like this:

```
root: list[float] | None = Field(..., max_length=2, min_length=2)
```

But the test still doesn't pass. i'm sure i'm missing something. I was hoping you could help me as i've spent a good bit of time trying to troubleshoot and i must be missing something. many many thanks!




lwasser: so sorry - i keep opening pr's against main by mistake 🙈 
lwasser: NOTE: i just updated the pr summary so hopefully it makes more sense and is more specific! 
lwasser: huh - things are working now.

* I'd still like a review to ensure my fix was valid. as i did override the default model generated type. 
* Maybe i can merge the two validator methods. 
* mypy is not happy about the override 
jsamoocha: My bad, and I totally understand your confusion. In #505, I created the `SummaryActivity` type and moved some fields there from our (detailed-) `Activity`, among which `start_latlng`. However, I forgot to move the _validator_ for `latlng` fields to that superclass. So whenever we try to deserialize a `SummaryActivity` (instead of a detailed `Activity`), the latlng fields are not validated properly, leading to validation errors in some edge cases.

The following validator in `model.Activity` just needs to be moved to the `model.SummaryActivity` superclass:

```python
_latlng_check = field_validator(
    "start_latlng", "end_latlng", mode="before"
)(check_valid_location)
```

I don't think any of the other changes in this PR are necessary to make lat/lon handling work again. The added test is ok, but we can also add a few cases to the `test_deserialization_edge_cases()` test in `test_model`.
lwasser: ok great. i'll move the validator, remove the other field change i made and add some tests later today! i also want to test what happens to the .lat and .lon properties. 
lwasser: ok everything is running here. gosh that small thing reallyl threw me off. next time i'll check the previous merged pr's more closely.

@jsamoocha only question i have is - we aren't explicitly checking .lat and .lon attrs. should we be? thanks for the help on this one!

Patch:
diff --git a/src/stravalib/model.py b/src/stravalib/model.py
index e9c8cff..237d727 100644
--- a/src/stravalib/model.py
+++ b/src/stravalib/model.py
@@ -28,7 +28,13 @@ from typing import (
 )
 
 from dateutil import parser
-from pydantic import BaseModel, Field, field_validator, model_validator
+from pydantic import (
+    AliasChoices,
+    BaseModel,
+    Field,
+    field_validator,
+    model_validator,
+)
 
 from stravalib import exc, strava_model
 from stravalib.strava_model import (
@@ -39,7 +45,6 @@ from stravalib.strava_model import (
     Comment,
     DetailedClub,
     DetailedGear,
-    DetailedSegmentEffort,
     ExplorerSegment,
     Lap,
     LatLng,
@@ -49,8 +54,6 @@ from stravalib.strava_model import (
     SportType,
     SummaryClub,
     SummaryGear,
-    SummaryPRSegmentEffort,
-    SummarySegmentEffort,
     TimedZoneRange,
 )
 
@@ -141,7 +144,7 @@ def lazy_property(fn: Callable[[U], T]) -> Optional[T]:
 # Custom validators for some edge cases:
 
 
-# This is the first valid location check.
+# This method checks a list of floats - ie a stream not just a single lat/lon
 def check_valid_location(
     location: Optional[Union[list[float], str]]
 ) -> Optional[list[float]]:
@@ -253,13 +256,11 @@ class RelaxedSportType(SportType):
 
 class LatLon(LatLng):
     """
-    Enables backward compatibility for legacy namedtuple
+    Stores lat / lon values or None.
     """
 
-    # It seems like we are validating LatLon twice which is a bit confusing.
-    # Once here and once when we ingest lat long into the Activity Object.
     @model_validator(mode="before")
-    def check_valid_latlng(cls, values: list[float]) -> Optional[list[float]]:
+    def check_valid_latlng(cls, values: list[float]) -> list[float] | None:
         """Validate that Strava returned an actual lat/lon rather than an empty
         list. If list is empty, return None
 
@@ -274,12 +275,8 @@ class LatLon(LatLng):
         list or None
             list of lat/lon values or None
 
-        Notes
-        ------
-        This is the second validation of lat lon but it's a validator
-        the first is check is def check_valid_location above.
-
         """
+
         # Strava sometimes returns empty list in case of activities without GPS
         return values if values else None
 
@@ -708,24 +705,19 @@ class SegmentExplorerResult(
         return self.bound_client.get_segment(self.id)
 
 
-class AthleteSegmentStats(
-    SummarySegmentEffort,
-):
-    """
-    A structure being returned for segment stats for current athlete.
-    """
-
-    # Undocumented attributes:
-    effort_count: Optional[int] = None
-    pr_elapsed_time: Optional[timedelta] = None
-    pr_date: Optional[date] = None
-
-    _naive_local = field_validator("start_date_local")(naive_datetime)
-
-
 class AthletePrEffort(
-    SummaryPRSegmentEffort,
+    strava_model.SummaryPRSegmentEffort,
 ):
+    # Override fields from superclass to match actual responses by Strava API:
+    activity_id: Optional[int] = Field(
+        validation_alias=AliasChoices("pr_activity_id", "activity_id"),
+        default=None,
+    )
+    elapsed_time: Optional[int] = Field(
+        validation_alias=AliasChoices("pr_elapsed_time", "elapsed_time"),
+        default=None,
+    )
+
     # Undocumented attributes:
     distance: Optional[float] = None
     start_date: Optional[datetime] = None
@@ -734,21 +726,16 @@ class AthletePrEffort(
 
     _naive_local = field_validator("start_date_local")(naive_datetime)
 
-    @property
-    def elapsed_time(self) -> Optional[int]:
-        """A property that supports backwards compatibility with the
-        elapsed_time method used in previous versions of stravalib"""
-
-        return self.pr_elapsed_time
-
 
 class SummarySegment(strava_model.SummarySegment, BoundClientEntity):
     # Field overrides from superclass for type extensions:
     start_latlng: Optional[LatLon] = None
     end_latlng: Optional[LatLon] = None
-    athlete_segment_stats: Optional[AthleteSegmentStats] = None
     athlete_pr_effort: Optional[AthletePrEffort] = None
     activity_type: Optional[RelaxedActivityType] = None  # type: ignore[assignment]
+    athlete_segment_stats: Optional[AthleteSegmentStats] = (
+        None  # Actually, this is only part of a (detailed) segment response
+    )
 
     _latlng_check = field_validator(
         "start_latlng", "end_latlng", mode="before"
@@ -801,8 +788,23 @@ class SegmentEffortAchievement(BaseModel):
     effort_count: Optional[int] = None
 
 
+class SummarySegmentEffort(strava_model.SummarySegmentEffort):
+    # Override superclass fields to match actual Strava API responses
+    activity_id: Optional[int] = Field(
+        validation_alias=AliasChoices("pr_activity_id", "activity_id"),
+        default=None,
+    )
+    elapsed_time: Optional[int] = Field(
+        validation_alias=AliasChoices("pr_elapsed_time", "elapsed_time"),
+        default=None,
+    )
+
+    _naive_local = field_validator("start_date_local")(naive_datetime)
+
+
 class BaseEffort(
-    DetailedSegmentEffort,
+    SummarySegmentEffort,
+    strava_model.DetailedSegmentEffort,
     BoundClientEntity,
 ):
     """
@@ -811,10 +813,8 @@ class BaseEffort(
 
     # Field overrides from superclass for type extensions:
     segment: Optional[SummarySegment] = None
-    activity: Optional[Activity] = None
-    athlete: Optional[Athlete] = None
-
-    _naive_local = field_validator("start_date_local")(naive_datetime)
+    activity: Optional[MetaActivity] = None
+    athlete: Optional[MetaAthlete] = None
 
 
 class BestEffort(BaseEffort):
@@ -833,6 +833,18 @@ class SegmentEffort(BaseEffort):
     achievements: Optional[list[SegmentEffortAchievement]] = None
 
 
+class AthleteSegmentStats(
+    SummarySegmentEffort,
+):
+    """
+    A structure being returned for segment stats for current athlete.
+    """
+
+    # Undocumented attributes:
+    effort_count: Optional[int] = None
+    pr_date: Optional[date] = None
+
+
 class MetaActivity(strava_model.MetaActivity, BoundClientEntity):
     @lazy_property
     def comments(self) -> BatchedResultsIterator[ActivityComment]:
@@ -871,14 +883,17 @@ class MetaActivity(strava_model.MetaActivity, BoundClientEntity):
 class SummaryActivity(MetaActivity, strava_model.SummaryActivity):
     # field overrides from superclass for type extensions:
     athlete: Optional[MetaAthlete] = None
-    start_latlng: Optional[LatLon] = (
-        None  # Do we still need these overrides for latlon?
-    )
+    # These force validator to run on lat/lon
+    start_latlng: Optional[LatLon] = None
     end_latlng: Optional[LatLon] = None
     map: Optional[Map] = None
     type: Optional[RelaxedActivityType] = None
     sport_type: Optional[RelaxedSportType] = None
 
+    _latlng_check = field_validator(
+        "start_latlng", "end_latlng", mode="before"
+    )(check_valid_location)
+
 
 class Activity(
     SummaryActivity,
@@ -930,9 +945,6 @@ class Activity(
     segment_leaderboard_opt_out: Optional[bool] = None
     perceived_exertion: Optional[int] = None
 
-    _latlng_check = field_validator(
-        "start_latlng", "end_latlng", mode="before"
-    )(check_valid_location)
     _naive_local = field_validator("start_date_local")(naive_datetime)
 
 
@@ -1063,8 +1075,3 @@ class SubscriptionUpdate(BaseModel):
     aspect_type: Optional[str] = None
     event_time: Optional[datetime] = None
     updates: Optional[dict[str, Any]] = None
-
-
-SegmentEffort.update_forward_refs()
-ActivityLap.update_forward_refs()
-BestEffort.update_forward_refs()


Test patch:
diff --git a/src/stravalib/tests/unit/test_model.py b/src/stravalib/tests/unit/test_model.py
index 3fdd6dd..d3331c5 100644
--- a/src/stravalib/tests/unit/test_model.py
+++ b/src/stravalib/tests/unit/test_model.py
@@ -18,11 +18,12 @@ from stravalib.model import (
     RelaxedSportType,
     Route,
     Segment,
+    SegmentEffort,
     SegmentExplorerResult,
     Split,
     SubscriptionCallback,
+    SummarySegmentEffort,
 )
-from stravalib.strava_model import LatLng
 from stravalib.tests import TestBase
 
 
@@ -30,16 +31,31 @@ from stravalib.tests import TestBase
     "model_class,raw,expected_value",
     (
         (
-            # Removing root
+            Activity,
+            {"start_latlng": []},
+            None,
+        ),
+        (
+            Activity,
+            {"end_latlng": []},
+            None,
+        ),
+        (
+            Activity,
+            {"end_latlng": "5.4,4.3"},
+            LatLon([5.4, 4.3]),
+        ),
+        (
             Activity,
             {"start_latlng": "5.4,4.3"},
             LatLon([5.4, 4.3]),
-        ),  # pydantic 2.x uses root and may
+        ),
         (Activity, {"start_latlng": []}, None),
         (Segment, {"start_latlng": []}, None),
         (SegmentExplorerResult, {"start_latlng": []}, None),
         (ActivityPhoto, {"location": []}, None),
-        # (Activity, {"timezone": "foobar"}, None),  TODO re-add this test when custom types are implemented
+        # TODO re-add this Activity test when custom types are implemented
+        # (Activity, {"timezone": "foobar"}, None),
         (
             Activity,
             {"start_date_local": "2023-01-17T11:06:07Z"},
@@ -55,6 +71,16 @@ from stravalib.tests import TestBase
             {"start_date_local": "2023-01-17T11:06:07Z"},
             datetime(2023, 1, 17, 11, 6, 7),
         ),
+        (
+            SummarySegmentEffort,
+            {"start_date_local": "2023-01-17T11:06:07Z"},
+            datetime(2023, 1, 17, 11, 6, 7),
+        ),
+        (
+            SegmentEffort,
+            {"start_date_local": "2023-01-17T11:06:07Z"},
+            datetime(2023, 1, 17, 11, 6, 7),
+        ),
     ),
 )
 def test_deserialization_edge_cases(model_class, raw, expected_value):
@@ -62,6 +88,46 @@ def test_deserialization_edge_cases(model_class, raw, expected_value):
     assert getattr(obj, list(raw.keys())[0]) == expected_value
 
 
+@pytest.mark.parametrize(
+    "model_class,raw,parsed_attr,expected_parsed_attr_value",
+    (
+        (SummarySegmentEffort, {"pr_activity_id": 42}, "activity_id", 42),
+        (SummarySegmentEffort, {"activity_id": 42}, "activity_id", 42),
+        (
+            SummarySegmentEffort,
+            {"pr_elapsed_time": 42},
+            "elapsed_time",
+            42,
+        ),
+        (
+            SummarySegmentEffort,
+            {"elapsed_time": 42},
+            "elapsed_time",
+            42,
+        ),
+        (AthletePrEffort, {"pr_activity_id": 42}, "activity_id", 42),
+        (AthletePrEffort, {"activity_id": 42}, "activity_id", 42),
+        (
+            AthletePrEffort,
+            {"pr_elapsed_time": 42},
+            "elapsed_time",
+            42,
+        ),
+        (
+            AthletePrEffort,
+            {"elapsed_time": 42},
+            "elapsed_time",
+            42,
+        ),
+    ),
+)
+def test_strava_api_field_name_inconsistencies(
+    model_class, raw, parsed_attr, expected_parsed_attr_value
+):
+    obj = model_class.model_validate(raw)
+    assert getattr(obj, parsed_attr) == expected_parsed_attr_value
+
+
 def test_subscription_callback_field_names():
     sub_callback_raw = {
         "hub.mode": "subscribe",
@@ -73,11 +139,6 @@ def test_subscription_callback_field_names():
     assert sub_callback.hub_verify_token == "STRAVA"
 
 
-class ExtLatLng(LatLng):
-    def foo(self):
-        return f"[{self[0], self[1]}]"
-
-
 # TODO: do we want to continue to support type?
 @pytest.mark.parametrize(
     "klass,attr,given_type,expected_type",