Address SQL Server review comments

sno/base_dataset.py

@@ -9,6 +9,10 @@ class BaseDataset(ImportSource):
     """
     Common interface for all datasets - mainly Dataset2, but
     there is also Dataset0 and Dataset1 used by `sno upgrade`.
+
+    A Dataset instance is immutable since it is a view of a particular git tree.
+    To get a new version of a dataset, commit the desired changes,
+    then instantiate a new Dataset instance that references the new git tree.
     """
 
     # Constants that subclasses should generally define.

sno/checkout.py

@@ -378,7 +378,7 @@ def create_workingcopy(ctx, discard_changes, wc_path):
         wc_path = WorkingCopy.default_path(repo.workdir_path)
 
     if wc_path != old_wc_path:
-        WorkingCopy.check_valid_creation_path(repo.workdir_path, wc_path)
+        WorkingCopy.check_valid_creation_path(wc_path, repo.workdir_path)
 
     # Finished sanity checks - start work:
     if old_wc and wc_path != old_wc_path:

sno/clone.py

@@ -106,7 +106,7 @@ def clone(
 
     if repo_path.exists() and any(repo_path.iterdir()):
         raise InvalidOperation(f'"{repo_path}" isn\'t empty', param_hint="directory")
-    WorkingCopy.check_valid_creation_path(repo_path, wc_path)
+    WorkingCopy.check_valid_creation_path(wc_path, repo_path)
 
     if not repo_path.exists():
         repo_path.mkdir(parents=True)

sno/geometry.py

@@ -59,6 +59,11 @@ def with_crs_id(self, crs_id):
 
     @property
     def crs_id(self):
+        """
+        Returns the CRS ID as it is embedded in the GPKG header - before the WKB.
+        Note that datasets V2 zeroes this field before committing,
+        so will return zero when called on Geometry where it has been zeroed.
+        """
         wkb_offset, is_le, crs_id = parse_gpkg_geom(self)
         return crs_id
 
@@ -296,6 +301,7 @@ def gpkg_geom_to_ogr(gpkg_geom, parse_crs=False):
 
 
 def wkt_to_gpkg_geom(wkt, **kwargs):
+    """Given a well-known-text string, returns a GPKG Geometry object."""
     if wkt is None:
         return None
 
@@ -304,6 +310,7 @@ def wkt_to_gpkg_geom(wkt, **kwargs):
 
 
 def wkb_to_gpkg_geom(wkb, **kwargs):
+    """Given a well-known-binary bytestring, returns a GPKG Geometry object."""
     if wkb is None:
         return None
 
@@ -312,6 +319,7 @@ def wkb_to_gpkg_geom(wkb, **kwargs):
 
 
 def hex_wkb_to_gpkg_geom(hex_wkb, **kwargs):
+    """Given a hex-encoded well-known-binary bytestring, returns a GPKG Geometry object."""
     if hex_wkb is None:
         return None
 

sno/init.py

@@ -369,7 +369,7 @@ def init(
 
     if repo_path.exists() and any(repo_path.iterdir()):
         raise InvalidOperation(f'"{repo_path}" isn\'t empty', param_hint="directory")
-    WorkingCopy.check_valid_creation_path(repo_path, wc_path)
+    WorkingCopy.check_valid_creation_path(wc_path, repo_path)
 
     if not repo_path.exists():
         repo_path.mkdir(parents=True)

sno/repo.py

@@ -177,7 +177,7 @@ def init_repository(
         repo_root_path = repo_root_path.resolve()
         cls._ensure_exists_and_empty(repo_root_path)
         if not bare:
-            WorkingCopy.check_valid_creation_path(repo_root_path, wc_path)
+            WorkingCopy.check_valid_creation_path(wc_path, repo_root_path)
 
         extra_args = []
         if initial_branch is not None:
@@ -224,7 +224,7 @@ def clone_repository(
         repo_root_path = repo_root_path.resolve()
         cls._ensure_exists_and_empty(repo_root_path)
         if not bare:
-            WorkingCopy.check_valid_creation_path(repo_root_path, wc_path)
+            WorkingCopy.check_valid_creation_path(wc_path, repo_root_path)
 
         if bare:
             sno_repo = cls._create_with_git_command(

sno/sqlalchemy.py

@@ -1,6 +1,6 @@
 import re
 import socket
-from urllib.parse import urlsplit, urlunsplit
+from urllib.parse import urlsplit, urlunsplit, urlencode, parse_qs
 
 
 import sqlalchemy
@@ -11,6 +11,7 @@
 
 from sno import spatialite_path
 from sno.geometry import Geometry
+from sno.exceptions import NotFound
 
 
 def gpkg_engine(path):
@@ -110,7 +111,6 @@ def _on_connect(psycopg2_conn, connection_record):
 
 CANONICAL_SQL_SERVER_SCHEME = "mssql"
 INTERNAL_SQL_SERVER_SCHEME = "mssql+pyodbc"
-SQL_SERVER_DRIVER_LIB = "ODBC+Driver+17+for+SQL+Server"
 
 
 def sqlserver_engine(msurl):
@@ -122,7 +122,7 @@ def sqlserver_engine(msurl):
     url_netloc = re.sub(r"\blocalhost\b", _replace_with_localhost, url.netloc)
 
     url_query = _append_to_query(
-        url.query, {"driver": SQL_SERVER_DRIVER_LIB, "Application+Name": "sno"}
+        url.query, {"driver": get_sqlserver_driver(), "Application Name": "sno"}
     )
 
     msurl = urlunsplit(
@@ -133,18 +133,30 @@ def sqlserver_engine(msurl):
     return engine
 
 
+def get_sqlserver_driver():
+    import pyodbc
+
+    drivers = [
+        d for d in pyodbc.drivers() if re.search("SQL Server", d, flags=re.IGNORECASE)
+    ]
+    if not drivers:
+        drivers = pyodbc.drivers()
+    if not drivers:
+        raise NotFound("SQL Server driver was not found")
+    return sorted(drivers)[-1]  # Latest driver
+
+
 def _replace_with_localhost(*args, **kwargs):
     return socket.gethostbyname("localhost")
 
 
-def _append_query_to_url(uri, query_dict):
+def _append_query_to_url(uri, new_query_dict):
     url = urlsplit(uri)
-    url_query = _append_to_query(url.query, query_dict)
+    url_query = _append_to_query(url.query, new_query_dict)
     return urlunsplit([url.scheme, url.netloc, url.path, url_query, ""])
 
 
-def _append_to_query(url_query, query_dict):
-    for key, value in query_dict.items():
-        if key not in url_query:
-            url_query = "&".join(filter(None, [url_query, f"{key}={value}"]))
-    return url_query
+def _append_to_query(existing_query, new_query_dict):
+    query_dict = parse_qs(existing_query)
+    # ignore new keys if they're already set in the querystring
+    return urlencode({**new_query_dict, **query_dict})