Repository: stravalib/stravalib

Issue:
Method to get stream kinda broken.
The method to get the streams (`get_activity_streams`, `get_effort_streams`, `get_segment_streams`) seems to not match the schema from strava https://developers.strava.com/playground/#/Streams/getActivityStreams

The types are supposed to be passed as a query argument and not as part of the path. Also, this argument which they call `key` in the schema is required.

I suggest we remove the default of None for those functions and fix the way we build the request. I can take care of it if you agree with the proposed changes.

Discussion:
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/stravalib/stravalib/pull/385?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=stravalib) Report
> Merging [#385](https://app.codecov.io/gh/stravalib/stravalib/pull/385?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=stravalib) (d89e02b) into [master](https://app.codecov.io/gh/stravalib/stravalib/commit/dffb9284e56fd344446b9d54ec3ccad157d060be?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=stravalib) (dffb928) will **increase** coverage by `0.47%`.
> The diff coverage is `100.00%`.

```diff
@@            Coverage Diff             @@
##           master     #385      +/-   ##
==========================================
+ Coverage   85.89%   86.36%   +0.47%     
==========================================
  Files          10       10              
  Lines        1666     1665       -1     
  Branches      157      158       +1     
==========================================
+ Hits         1431     1438       +7     
+ Misses        192      187       -5     
+ Partials       43       40       -3     
```


| [Impacted Files](https://app.codecov.io/gh/stravalib/stravalib/pull/385?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=stravalib) | Coverage Δ | |
|---|---|---|
| [stravalib/client.py](https://app.codecov.io/gh/stravalib/stravalib/pull/385?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=stravalib#diff-c3RyYXZhbGliL2NsaWVudC5weQ==) | `74.47% <100.00%> (+1.79%)` | :arrow_up: |

:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=stravalib)

enadeau: I've written some test to bring the coverage to 100% on the method.

I think it is ready for review

The `get_effrot_streams` and `get_segments_streams` still need to be modified, but the change would be identical to the one I did for `get_activity_streams` so best to get the review in first.
enadeau: Applied your suggestions and also did the effort and segment method. 

I've refactored the code to avoid having the same logic 3 time.

Ready for review

Patch:
diff --git a/changelog.md b/changelog.md
index 0475fba..4545dbf 100644
--- a/changelog.md
+++ b/changelog.md
@@ -2,6 +2,8 @@
 
 ## Unreleased
 
+- Update client's stream method to warn when using unofficial parameters (@enadeau, #385)
+
 ## v1.3.3
 
 ### Fixed
diff --git a/stravalib/client.py b/stravalib/client.py
index b55fdac..c00fa93 100644
--- a/stravalib/client.py
+++ b/stravalib/client.py
@@ -18,7 +18,7 @@ from typing import Dict, List, Optional
 import arrow
 import pytz
 
-from stravalib import exc, model, unithelper
+from stravalib import exc, model, strava_model, unithelper
 from stravalib.exc import (
     ActivityPhotoUploadNotSupported,
     warn_attribute_unofficial,
@@ -1361,10 +1361,12 @@ class Client(object):
             for v in raw["segments"]
         ]
 
-    def get_activity_streams(
-        self, activity_id, types=None, resolution=None, series_type=None
+    def _get_streams(
+        self, stream_url, types=None, resolution=None, series_type=None
     ):
-        """Returns a stream for an activity.
+        """
+        Generic method to retrieve stream data for activity, effort or
+        segment.
 
         https://developers.strava.com/docs/reference/#api-Streams-getActivityStreams
 
@@ -1381,56 +1383,102 @@ class Client(object):
 
         Parameters
         ----------
-        activity_id : int
-            The ID of activity.
-        types : list, optional, default=None
+        stream_url : str
+            Resource locator for the streams
+        types : list[str], optional, default=None
             A list of the types of streams to fetch.
-        resolution : str
-            optional, default is 'all') indicates desired number
-            of data points. 'low' (100), 'medium' (1000),
-            'high' (10000) or 'all'.
-        series_type : str, optional, default='distance'
+        resolution : str, optional
+            Indicates desired number of data points. 'low' (100), 'medium'
+            (1000) or 'high' (10000).
+            .. deprecated::
+                This param is not officialy supported by the Strava API and may be
+                removed in the future.
+        series_type : str, optional
             Relevant only if using resolution either 'time' or 'distance'.
             Used to index the streams if the stream is being reduced.
+            .. deprecated::
+                This param is not officialy supported by the Strava API and may be
+                removed in the future.
 
         Returns
         -------
         py:class:`dict`
             An dictionary of :class:`stravalib.model.Stream` from the activity
-            or None if there are no streams.
-
         """
-
-        # Stream is a comma separated list
-        if types is not None:
-            types = ",".join(types)
-
-        params = {}
+        extra_params = {}
         if resolution is not None:
-            params["resolution"] = resolution
-
+            warn_param_unofficial("resolution")
+            extra_params["resolution"] = resolution
         if series_type is not None:
-            params["series_type"] = series_type
-
-        result_fetcher = functools.partial(
-            self.protocol.get,
-            "/activities/{id}/streams/{types}".format(
-                id=activity_id, types=types
-            ),
-            **params,
+            warn_param_unofficial("series_type")
+            extra_params["series_type"] = series_type
+        if not types:
+            types = strava_model.StreamType.schema()["enum"]
+        invalid_types = set(types).difference(
+            strava_model.StreamType.schema()["enum"]
         )
+        if invalid_types:
+            raise ValueError(
+                f"Types {invalid_types} not supported by StravaApi"
+            )
+        types_arg = ",".join(types)
 
-        streams = BatchedResultsIterator(
-            entity=model.Stream,
-            bind_client=self,
-            result_fetcher=result_fetcher,
+        response = self.protocol.get(
+            stream_url, keys=types_arg, key_by_type=True, **extra_params
         )
+        return {
+            stream_type: model.Stream.parse_obj(stream)
+            for stream_type, stream in response.items()
+        }
 
-        # Pack streams into dictionary
-        try:
-            return {i.type: i for i in streams}
-        except exc.ObjectNotFound:
-            return None  # just to be explicit.
+    def get_activity_streams(
+        self, activity_id, types=None, resolution=None, series_type=None
+    ):
+        """Returns a stream for an activity.
+
+        https://developers.strava.com/docs/reference/#api-Streams-getActivityStreams
+
+        Streams represent the raw spatial data for the uploaded file. External
+        applications may only access this information for activities owned
+        by the authenticated athlete.
+
+        Streams are available in 11 different types. If the stream is not
+        available for a particular activity it will be left out of the request
+        results.
+
+        Streams types are: time, latlng, distance, altitude, velocity_smooth,
+                           heartrate, cadence, watts, temp, moving, grade_smooth
+
+        Parameters
+        ----------
+        activity_id : int
+            The ID of activity.
+        types : list[str], optional, default=None
+            A list of the types of streams to fetch.
+        resolution : str, optional
+            Indicates desired number of data points. 'low' (100), 'medium'
+            (1000) or 'high' (10000).
+            .. deprecated::
+                This param is not officialy supported by the Strava API and may be
+                removed in the future.
+        series_type : str, optional
+            Relevant only if using resolution either 'time' or 'distance'.
+            Used to index the streams if the stream is being reduced.
+            .. deprecated::
+                This param is not officialy supported by the Strava API and may be
+                removed in the future.
+
+        Returns
+        -------
+        py:class:`dict`
+            An dictionary of :class:`stravalib.model.Stream` from the activity
+        """
+        return self._get_streams(
+            f"/activities/{activity_id}/streams",
+            types=types,
+            resolution=resolution,
+            series_type=series_type,
+        )
 
     def get_effort_streams(
         self, effort_id, types=None, resolution=None, series_type=None
@@ -1456,12 +1504,18 @@ class Client(object):
             The ID of effort.
         types : list, optional, default=None
             A list of the the types of streams to fetch.
-        resolution : str, optional, default='all'
+        resolution : str, optional
             Indicates desired number of data points. 'low' (100), 'medium'
-            (1000), 'high' (10000) or 'all'.
-        series_type : str, optional, default='distance'
+            (1000) or 'high' (10000).
+            .. deprecated::
+                This param is not officialy supported by the Strava API and may be
+                removed in the future.
+        series_type : str, optional
             Relevant only if using resolution either 'time' or 'distance'.
             Used to index the streams if the stream is being reduced.
+            .. deprecated::
+                This param is not officialy supported by the Strava API and may be
+                removed in the future.
 
         Returns
         -------
@@ -1469,35 +1523,13 @@ class Client(object):
             An dictionary of :class:`stravalib.model.Stream` from the effort.
 
         """
-
-        # Stream are comma separated lists
-        if types is not None:
-            types = ",".join(types)
-
-        params = {}
-        if resolution is not None:
-            params["resolution"] = resolution
-
-        if series_type is not None:
-            params["series_type"] = series_type
-
-        result_fetcher = functools.partial(
-            self.protocol.get,
-            "/segment_efforts/{id}/streams/{types}".format(
-                id=effort_id, types=types
-            ),
-            **params,
-        )
-
-        streams = BatchedResultsIterator(
-            entity=model.Stream,
-            bind_client=self,
-            result_fetcher=result_fetcher,
+        return self._get_streams(
+            f"/segment_efforts/{effort_id}/streams",
+            types=types,
+            resolution=resolution,
+            series_type=series_type,
         )
 
-        # Pack streams into dictionary
-        return {i.type: i for i in streams}
-
     def get_segment_streams(
         self, segment_id, types=None, resolution=None, series_type=None
     ):
@@ -1522,12 +1554,18 @@ class Client(object):
             The ID of segment.
         types : list, optional, default=None
             A list of the the types of streams to fetch.
-        resolution : str, optional, default='all'
+        resolution : str, optional
             Indicates desired number of data points. 'low' (100), 'medium'
-            (1000), 'high' (10000) or 'all'.
-        series_type : str, optional, default='distance'
+            (1000) or 'high' (10000).
+            .. deprecated::
+                This param is not officialy supported by the Strava API and may be
+                removed in the future.
+        series_type : str, optional
             Relevant only if using resolution either 'time' or 'distance'.
             Used to index the streams if the stream is being reduced.
+            .. deprecated::
+                This param is not officialy supported by the Strava API and may be
+                removed in the future.
 
         Returns
         -------
@@ -1535,35 +1573,13 @@ class Client(object):
             An dictionary of :class:`stravalib.model.Stream` from the effort.
 
         """
-
-        # Stream are comma separated lists
-        if types is not None:
-            types = ",".join(types)
-
-        params = {}
-        if resolution is not None:
-            params["resolution"] = resolution
-
-        if series_type is not None:
-            params["series_type"] = series_type
-
-        result_fetcher = functools.partial(
-            self.protocol.get,
-            "/segments/{id}/streams/{types}".format(
-                id=segment_id, types=types
-            ),
-            **params,
+        return self._get_streams(
+            f"/segments/{segment_id}/streams",
+            types=types,
+            resolution=resolution,
+            series_type=series_type,
         )
 
-        streams = BatchedResultsIterator(
-            entity=model.Stream,
-            bind_client=self,
-            result_fetcher=result_fetcher,
-        )
-
-        # Pack streams into dictionary
-        return {i.type: i for i in streams}
-
     def get_routes(self, athlete_id=None, limit=None):
         """Gets the routes list for an authenticated user.
 


Test patch:
diff --git a/stravalib/tests/integration/test_client.py b/stravalib/tests/integration/test_client.py
index fdb17f0..36e3a23 100644
--- a/stravalib/tests/integration/test_client.py
+++ b/stravalib/tests/integration/test_client.py
@@ -81,13 +81,120 @@ def test_get_activity_zones(mock_strava_api, client):
 
 
 def test_get_activity_streams(mock_strava_api, client):
-    # TODO: parameterize test to cover all branching
+    query_params = {"keys": "distance", "key_by_type": True}
     mock_strava_api.get(
-        "/activities/{id}/streams", response_update={"data": [1, 2, 3]}
+        "/activities/{id}/streams",
+        json={
+            "distance": {
+                "data": [1.0, 2.0, 3.0],
+                "series_type": "distance",
+                "original_size": 9,
+                "resolution": "high",
+            }
+        },
+        match=[matchers.query_param_matcher(query_params)],
+    )
+    streams = client.get_activity_streams(42, types=["distance"])
+    assert streams["distance"].data == [1.0, 2.0, 3.0]
+
+
+def test_get_effort_streams(mock_strava_api, client):
+    query_params = {"keys": "distance", "key_by_type": True}
+    mock_strava_api.get(
+        "/segment_efforts/{id}/streams",
+        json={
+            "distance": {
+                "data": [1.0, 2.0, 3.0],
+                "series_type": "distance",
+                "original_size": 9,
+                "resolution": "high",
+            }
+        },
+        match=[matchers.query_param_matcher(query_params)],
+    )
+    streams = client.get_effort_streams(42, types=["distance"])
+    assert streams["distance"].data == [1.0, 2.0, 3.0]
+
+
+def test_get_segment_streams(mock_strava_api, client):
+    query_params = {"keys": "distance", "key_by_type": True}
+    mock_strava_api.get(
+        "/segments/{id}/streams",
+        json={
+            "distance": {
+                "data": [1.0, 2.0, 3.0],
+                "series_type": "distance",
+                "original_size": 9,
+                "resolution": "high",
+            }
+        },
+        match=[matchers.query_param_matcher(query_params)],
+    )
+    streams = client.get_segment_streams(42, types=["distance"])
+    assert streams["distance"].data == [1.0, 2.0, 3.0]
+
+
+def test_get_activity_streams_no_type_specified(mock_strava_api, client):
+    query_params = {
+        "keys": "time,distance,latlng,altitude,velocity_smooth,heartrate,cadence,watts,temp,moving,grade_smooth",
+        "key_by_type": True,
+    }
+    mock_strava_api.get(
+        "/activities/{id}/streams",
+        json={
+            "distance": {
+                "data": [1.0, 2.0, 3.0],
+                "original_size": 9,
+                "resolution": "high",
+            }
+        },
+        match=[matchers.query_param_matcher(query_params)],
     )
-    # the example in swagger.json returns a distance stream
     streams = client.get_activity_streams(42)
-    assert streams["distance"].data == [1, 2, 3]
+    assert streams["distance"].data == [1.0, 2.0, 3.0]
+
+
+def test_get_activity_streams_invalid_type(mock_strava_api, client):
+    with pytest.raises(ValueError):
+        streams = client.get_activity_streams(
+            42, types=["distance", "hhjj", "npt"]
+        )
+
+
+def test_get_activity_streams_resolution_unofficial(mock_strava_api, client):
+    mock_strava_api.get(
+        "/activities/{id}/streams",
+        json={
+            "distance": {
+                "data": [1.0, 2.0, 3.0],
+                "series_type": "distance",
+                "original_size": 9,
+                "resolution": "high",
+            }
+        },
+    )
+    with pytest.warns(FutureWarning):
+        streams = client.get_activity_streams(
+            42, types=["distance"], resolution="high"
+        )
+
+
+def test_get_activity_streams_series_type_unofficial(mock_strava_api, client):
+    mock_strava_api.get(
+        "/activities/{id}/streams",
+        json={
+            "distance": {
+                "data": [1.0, 2.0, 3.0],
+                "series_type": "distance",
+                "original_size": 9,
+                "resolution": "high",
+            }
+        },
+    )
+    with pytest.warns(FutureWarning):
+        streams = client.get_activity_streams(
+            42, types=["distance"], series_type="distance"
+        )
 
 
 @pytest.mark.parametrize(
@@ -704,14 +811,7 @@ def test_upload_activity_photo_fail_activity_upload_not_complete(client):
     assert str(error.value) == "Activity upload not complete"
 
 
-@pytest.mark.parametrize(
-    "photo_metadata",
-    (
-        None,
-        [],
-        [{}],
-    ),
-)
+@pytest.mark.parametrize("photo_metadata", (None, [], [{}]))
 @mock.patch("stravalib.client.ActivityUploader.poll")
 def test_upload_activity_photo_fail_not_supported(client, photo_metadata):
     activity_upload_response = {