Repository: pydap/pydap

Issue:
ParseError:`requests_cache.Session` can have unexpected behaviors : Add best practices to documentation
I ran into an initially unexpected behavior /  problem when caching sessions. Caching URLs when the cached session object is not reused, can lead the session to ignore constraint expressions and pydap's open_url to produce xml Parse Errors when attempting to download dap data.


## minimal example
```python
from pydap.client import open_url
data_url = "dap4://opendap.earthdata.nasa.gov/collections/C2491756351-POCLOUD/granules/sss_rc20140912.v5.0cap"

pyds = open_url(data_url, use_cache=True) # under the hood, use_cache=True creates a CachedSession that is passed around, but not reused by user
pyds.tree()
>>>
.sss_rc20140912.v5.0cap.nc
├──lon
├──lat
├──sss_cap
├──ice_frac
└──Equirectangular
```
I can request some data and works
```python
pyds['/lat'][:]
```
However, if I recreate the pyds object
```python
pyds = open_url(data_url, use_cache=True)
```
Now the following errs:
```python
pyds['lon'][:]
>>>
  File <string>
ParseError: syntax error: line 1, column 0
```

Any variable that was downloaded before re-creating the `pyds` object, such as `lon` still works, because it was stored on a cached file. But no new variable can be downloaded from this URL. Creating a new session and passing it when creating the `pyds` object will not work either... At this point, the only alternative is to manually erase the cache file.

## Solution
* Option 1: Deleting/Clearing the cached session will make it work This highlights the need to document this observed behavior, and perhaps include in the documentation a warning and a "best practices"
* Option 2: Define a session object and re-use it when caching the session. As long as the user does not loose this object, one can create requests and the urls will be cached. This also needs to be documented. 
* Option 3: Reduce the expiration time of the cached session. I originally set it to 1 day by default It should be much shorted by default (~ 1-5 minutes). NOTE: The user can always specify this time.

Overall, I think that including best practices will make a big difference in terms of user experience. This behavior with the errors makes sense, and is not a bug. 

In addition to that, it would be good to add a user warning when setting `use_cache=True` and `session=None`. Or perhaps we should also prevent such scenario to occur in the first place...


## Some useful approaches with CachedSessions:
### cached URLs
```python
session.cache.urls()
>>>['http://test.opendap.org/opendap/data/nc/coads_climatology.nc.dap?dap4.ce=COADSX%5B0%3A1%3A179%5D',
 'http://test.opendap.org/opendap/data/nc/coads_climatology.nc.dap?dap4.ce=COADSY%5B0%3A1%3A89%5D',
 'http://test.opendap.org/opendap/data/nc/coads_climatology.nc.dmr',
 'https://opendap.earthdata.nasa.gov/collections/C2491756351-POCLOUD/granules/sss_rc20140912.v5.0cap.dap?dap4.ce=lat%5B0%3A1%3A179%5D',
 'https://opendap.earthdata.nasa.gov/collections/C2491756351-POCLOUD/granules/sss_rc20140912.v5.0cap.dap?dap4.ce=lon%5B0%3A1%3A359%5D',
 'https://opendap.earthdata.nasa.gov/collections/C2491756351-POCLOUD/granules/sss_rc20140912.v5.0cap.dap?dap4.ce=sss_cap%5B0%3A1%3A179%5D%5B0%3A1%3A359%5D',
 'https://opendap.earthdata.nasa.gov/collections/C2491756351-POCLOUD/granules/sss_rc20140912.v5.0cap.dmr']
```
and
### location of cached session
```python
session.cache.db_path
>>> PosixPath('/var/folders/hc/tkfpclz952n091r0k5b2t9jr0000gn/T/http_cache.sqlite')
```

Then one can manually delete the `http_cache.sqlite document`, in the case the expriration time of the cached session is very long, or when something went wrong / unexpected behavior.








Discussion:

Patch:
diff --git a/.gitignore b/.gitignore
index f4844b6..5ef2383 100644
--- a/.gitignore
+++ b/.gitignore
@@ -3,6 +3,8 @@
 # C extensions
 *.so
 
+# no chached sessions
+*.sqlite
 # Packages
 *.egg
 *.egg-info
diff --git a/src/pydap/client.py b/src/pydap/client.py
index 5f72dae..9eba02d 100644
--- a/src/pydap/client.py
+++ b/src/pydap/client.py
@@ -135,6 +135,7 @@ def open_url(
         get_kwargs=get_kwargs,
     )
     dataset = handler.dataset
+    dataset._session = session
 
     # attach server-side functions
     dataset.functions = Functions(url, application, session, timeout=timeout)
diff --git a/src/pydap/model.py b/src/pydap/model.py
index 5909666..f89702e 100644
--- a/src/pydap/model.py
+++ b/src/pydap/model.py
@@ -174,8 +174,11 @@ import warnings
 from collections import OrderedDict
 from collections.abc import Mapping
 from functools import reduce
+from typing import Optional, Union
 
 import numpy as np
+import requests
+import requests_cache
 
 from .lib import _quote, decode_np_strings, tree, walk
 
@@ -571,6 +574,43 @@ class DatasetType(StructureType):
         'B'
     """
 
+    def __init__(
+        self,
+        name="nameless",
+        attributes=None,
+        session: Optional[Union[requests.Session, requests_cache.CachedSession]] = None,
+        **kwargs,
+    ):
+        super().__init__(name, attributes, **kwargs)
+        # Explicit type checking to enforce the allowed types
+        if session is not None and not isinstance(
+            session, (requests.Session, requests_cache.CachedSession)
+        ):
+            raise TypeError(
+                "`session` must be a `requests.Session` or "
+                "`requests_cache.CachedSession` instance"
+            )
+        self._session = session
+
+    @property
+    def session(self):
+        """Read-only property for session."""
+        return self._session
+
+    @session.setter
+    def session(self, value):
+        """Prevent re-assignment of session."""
+        if self.session:
+            raise AttributeError("Cannot modify `session` after it has been set.")
+        else:
+            if isinstance(value, (requests.Session, requests_cache.CachedSession)):
+                self._session = value
+            else:
+                raise TypeError(
+                    "`session` must be a `requests.Session` or "
+                    "`requests_cache.CachedSession` instance"
+                )
+
     def __setitem__(self, key, item):
         # key a path-like only in DAP4
         split_by = " "


Test patch:
diff --git a/src/pydap/tests/test_client.py b/src/pydap/tests/test_client.py
index 0e73cc0..fc7fbbc 100644
--- a/src/pydap/tests/test_client.py
+++ b/src/pydap/tests/test_client.py
@@ -30,6 +30,7 @@ def test_open_url(sequence_app):
     """Open an URL and check dataset keys."""
     dataset = open_url("http://localhost:8001/", sequence_app)
     assert list(dataset.keys()) == ["cast"]
+    assert isinstance(dataset.session, requests.Session)
 
 
 @pytest.fixture
diff --git a/src/pydap/tests/test_model.py b/src/pydap/tests/test_model.py
index d86afcd..32c79fd 100644
--- a/src/pydap/tests/test_model.py
+++ b/src/pydap/tests/test_model.py
@@ -6,6 +6,8 @@ from collections.abc import Mapping
 
 import numpy as np
 import pytest
+import requests
+import requests_cache
 
 from pydap.model import (
     BaseType,
@@ -678,3 +680,31 @@ def test_GridType_maps(gridtype_example):
 def test_GridType_dimensions(gridtype_example):
     """Test ``dimensions`` property."""
     assert gridtype_example.dimensions == ("x", "y")
+
+
+@pytest.mark.parametrize(
+    "session",
+    [None, requests.Session(), requests_cache.CachedSession(), "session"],
+)
+def test_error_session(session):
+    """test that trying to set a session with other than a `None`,
+    a `requests.Session` or a `requests_cached.CachedSession()` object
+    returns a TypeError
+    """
+    if not session:
+        # is session is None (default) then session can be changed later
+        pyds = DatasetType("name")
+        assert pyds.session == session  # asserts default
+        pyds.session = (
+            requests_cache.CachedSession()
+        )  # sets the session to a new cached session
+        assert isinstance(pyds.session, requests_cache.CachedSession)
+    elif isinstance(session, (requests.Session, requests_cache.CachedSession)):
+        pyds = DatasetType("name", session=session)
+        assert pyds.session == session
+        with pytest.raises(AttributeError):
+            pyds.session = None  # session cannot be modify once set
+    else:
+        # test attemps to set the session object with a string type - not allowed
+        with pytest.raises(TypeError):
+            DatasetType("name", session=session)