Repository: Materials-Consortia/optimade-python-tools (python)

Issue:
ParserError's should not return 500 HTTP status codes
When the Lark parser is unable to parse a filter in an unhandled way, we throw a `ParserError`, which inherits directly from `Exception` and thus the server treats it as a 500 Server Error.

Instead this should raise a 400 Bad Request and return the Lark error message.

This is something that has been biting OPTIMADE implementations that use k8s, as 500 is treated as "down endpoint" status code (cc @shyamd).  

Pull request:
Return 400 Bad Request (not 500) on filter parser errors, plus filterparser module facelift
Closes #812 by changing filter parsing errors from `ParserError`s to `BadRequest`s.

Also adds docstrings, types etc. to the filterparser module.

Patch:
diff --git a/optimade/filterparser/lark_parser.py b/optimade/filterparser/lark_parser.py
index 9a6a5cd1..f1200ed0 100644
--- a/optimade/filterparser/lark_parser.py
+++ b/optimade/filterparser/lark_parser.py
@@ -1,13 +1,35 @@
+"""This submodule implements the [`LarkParser`][optimade.filterparser.LarkParser] class,
+which uses the lark library to parse filter strings with a defined OPTIMADE filter grammar
+into `Lark.Tree` objects for use by the filter transformers.
+
+"""
+
 from pathlib import Path
-from lark import Lark, Tree
+from typing import Dict, Tuple
 from collections import defaultdict
 
+from lark import Lark, Tree
+
+from optimade.server.exceptions import BadRequest
+
+__all__ = ("ParserError", "LarkParser")
+
 
 class ParserError(Exception):
-    pass
+    """Triggered by critical parsing errors that should lead
+    to 500 Server Error HTTP statuses.
+    """
+
+
+def get_versions() -> Dict[Tuple[int, int, int], Dict[str, str]]:
+    """Find grammar files within this package's grammar directory,
+    returning a dictionary broken down by scraped grammar version
+    (major, minor, patch) and variant (a string tag).
 
+    Returns:
+        A mapping from version, variant to grammar file name.
 
-def get_versions():
+    """
     dct = defaultdict(dict)
     for filename in Path(__file__).parent.joinpath("../grammar").glob("*.lark"):
         tags = filename.stem.lstrip("v").split(".")
@@ -17,36 +39,66 @@ def get_versions():
     return dict(dct)
 
 
-available_parsers = get_versions()
+AVAILABLE_PARSERS = get_versions()
 
 
 class LarkParser:
-    def __init__(self, version=None, variant="default"):
+    """This class wraps a versioned OPTIMADE grammar and allows
+    it to be parsed into Lark tree objects.
 
-        version = version if version else max(available_parsers.keys())
+    """
 
-        if version not in available_parsers:
+    def __init__(self, version: Tuple[int, int, int] = None, variant: str = "default"):
+        """For a given version and variant, try to load the corresponding grammar.
+
+        Parameters:
+            version: The grammar version number to use (e.g., `(1, 0, 1)` for v1.0.1).
+            variant: The grammar variant to employ.
+
+        Raises:
+            ParserError: If the requested version/variant of the
+                grammar does not exist.
+
+        """
+
+        version = version if version else max(AVAILABLE_PARSERS.keys())
+
+        if version not in AVAILABLE_PARSERS:
             raise ParserError(f"Unknown parser grammar version: {version}")
 
-        if variant not in available_parsers[version]:
+        if variant not in AVAILABLE_PARSERS[version]:
             raise ParserError(f"Unknown variant of the parser: {variant}")
 
         self.version = version
         self.variant = variant
 
-        with open(available_parsers[version][variant]) as file:
-            self.lark = Lark(file)
+        with open(AVAILABLE_PARSERS[version][variant]) as f:
+            self.lark = Lark(f)
 
         self.tree = None
         self.filter = None
 
-    def parse(self, filter_):
+    def parse(self, filter_: str) -> Tree:
+        """Parse a filter string into a `lark.Tree`.
+
+        Parameters:
+            filter_: The filter string to parse.
+
+        Raises:
+            BadRequest: If the filter cannot be parsed.
+
+        Returns:
+            The parsed filter.
+
+        """
         try:
             self.tree = self.lark.parse(filter_)
             self.filter = filter_
             return self.tree
-        except Exception as e:
-            raise ParserError(e)
+        except Exception as exc:
+            raise BadRequest(
+                detail=f"Unable to parse filter {filter_}. Lark traceback: \n{exc}"
+            ) from exc
 
     def __repr__(self):
         if isinstance(self.tree, Tree):


Test patch:
diff --git a/tests/filterparser/test_filterparser.py b/tests/filterparser/test_filterparser.py
index d4a40374..913fc707 100644
--- a/tests/filterparser/test_filterparser.py
+++ b/tests/filterparser/test_filterparser.py
@@ -5,7 +5,8 @@ import pytest
 
 from lark import Tree
 
-from optimade.filterparser import LarkParser, ParserError
+from optimade.filterparser import LarkParser
+from optimade.server.exceptions import BadRequest
 
 testfile_dir = os.path.join(os.path.dirname(__file__), "testfiles")
 
@@ -22,7 +23,7 @@ class TestParserV0_9_5:
     def test_inputs(self):
         for tf in self.test_filters:
             if tf == "filter=number=0.0.1":
-                with pytest.raises(ParserError):
+                with pytest.raises(BadRequest):
                     self.parser.parse(tf)
             else:
                 tree = self.parser.parse(tf)
@@ -59,13 +60,13 @@ class TestParserV1_0_0:
         assert isinstance(self.parse("cell_length_a = 1"), Tree)
         assert isinstance(self.parse("cell_volume = 1"), Tree)
 
-        with pytest.raises(ParserError):
+        with pytest.raises(BadRequest):
             self.parse("0_kvak IS KNOWN")  # starts with a number
 
-        with pytest.raises(ParserError):
+        with pytest.raises(BadRequest):
             self.parse('"foo bar" IS KNOWN')  # contains space; contains quotes
 
-        with pytest.raises(ParserError):
+        with pytest.raises(BadRequest):
             self.parse("BadLuck IS KNOWN")  # contains upper-case letters
 
         # database-provider-specific prefixes
@@ -96,19 +97,19 @@ class TestParserV1_0_0:
         assert isinstance(self.parse("l = -.1e-12"), Tree)
         assert isinstance(self.parse("m = 1000000000.E1000000000"), Tree)
 
-        with pytest.raises(ParserError):
+        with pytest.raises(BadRequest):
             self.parse("number=1.234D12")
-        with pytest.raises(ParserError):
+        with pytest.raises(BadRequest):
             self.parse("number=.e1")
-        with pytest.raises(ParserError):
+        with pytest.raises(BadRequest):
             self.parse("number= -.E1")
-        with pytest.raises(ParserError):
+        with pytest.raises(BadRequest):
             self.parse("number=+.E2")
-        with pytest.raises(ParserError):
+        with pytest.raises(BadRequest):
             self.parse("number=1.23E+++")
-        with pytest.raises(ParserError):
+        with pytest.raises(BadRequest):
             self.parse("number=+-123")
-        with pytest.raises(ParserError):
+        with pytest.raises(BadRequest):
             self.parse("number=0.0.1")
 
     def test_operators(self):
@@ -268,11 +269,11 @@ class TestParserV1_0_0:
             self.parse('"NOTice"=val'), Tree
         )  # value ("NOTice") = property (val)
 
-        with pytest.raises(ParserError):
+        with pytest.raises(BadRequest):
             self.parse("NOTICE=val")  # not valid property or value (NOTICE)
-        with pytest.raises(ParserError):
+        with pytest.raises(BadRequest):
             self.parse('"NOTICE"=Val')  # not valid property (Val)
-        with pytest.raises(ParserError):
+        with pytest.raises(BadRequest):
             self.parse("NOTICE=val")  # not valid property or value (NOTICE)
 
     def test_parser_version(self):
diff --git a/tests/filtertransformers/test_mongo.py b/tests/filtertransformers/test_mongo.py
index 0f6de6ee..d797e618 100644
--- a/tests/filtertransformers/test_mongo.py
+++ b/tests/filtertransformers/test_mongo.py
@@ -2,7 +2,8 @@ import pytest
 
 from lark.exceptions import VisitError
 
-from optimade.filterparser import LarkParser, ParserError
+from optimade.filterparser import LarkParser
+from optimade.server.exceptions import BadRequest
 
 
 class TestMongoTransformer:
@@ -25,13 +26,13 @@ class TestMongoTransformer:
         assert self.transform("cell_length_a = 1") == {"cell_length_a": {"$eq": 1}}
         assert self.transform("cell_volume = 1") == {"cell_volume": {"$eq": 1}}
 
-        with pytest.raises(ParserError):
+        with pytest.raises(BadRequest):
             self.transform("0_kvak IS KNOWN")  # starts with a number
 
-        with pytest.raises(ParserError):
+        with pytest.raises(BadRequest):
             self.transform('"foo bar" IS KNOWN')  # contains space; contains quotes
 
-        with pytest.raises(ParserError):
+        with pytest.raises(BadRequest):
             self.transform("BadLuck IS KNOWN")  # contains upper-case letters
 
         # database-provider-specific prefixes
@@ -70,19 +71,19 @@ class TestMongoTransformer:
             "m": {"$eq": float("inf")}
         }
 
-        with pytest.raises(ParserError):
+        with pytest.raises(BadRequest):
             self.transform("number=1.234D12")
-        with pytest.raises(ParserError):
+        with pytest.raises(BadRequest):
             self.transform("number=.e1")
-        with pytest.raises(ParserError):
+        with pytest.raises(BadRequest):
             self.transform("number= -.E1")
-        with pytest.raises(ParserError):
+        with pytest.raises(BadRequest):
             self.transform("number=+.E2")
-        with pytest.raises(ParserError):
+        with pytest.raises(BadRequest):
             self.transform("number=1.23E+++")
-        with pytest.raises(ParserError):
+        with pytest.raises(BadRequest):
             self.transform("number=+-123")
-        with pytest.raises(ParserError):
+        with pytest.raises(BadRequest):
             self.transform("number=0.0.1")
 
     def test_simple_comparisons(self):