-
Notifications
You must be signed in to change notification settings - Fork 8
feat: configurable path param regex #208
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,7 @@ | |
| import logging | ||
| import re | ||
| from dataclasses import dataclass | ||
| from typing import Any, Awaitable, Callable, Optional | ||
| from typing import Any, Awaitable, Callable, Optional, Sequence | ||
|
|
||
| from cql2 import Expr, ValidationError | ||
| from fastapi import HTTPException | ||
|
|
@@ -32,12 +32,26 @@ class Cql2BuildFilterMiddleware: | |
|
|
||
| # Filters | ||
| collections_filter: Optional[Callable] = None | ||
| collections_filter_path: str = r"^/collections(/[^/]+)?$" | ||
| collections_filter_path: str | Sequence[str] = ( | ||
| r"^/collections(?:/(?P<collection_id>[^/]+))?$", | ||
| ) | ||
|
ceholden marked this conversation as resolved.
|
||
| items_filter: Optional[Callable] = None | ||
| items_filter_path: str = r"^(/collections/([^/]+)/items(/[^/]+)?$|/search$)" | ||
| items_filter_path: str | Sequence[str] = ( | ||
| r"^(?:/collections/(?P<collection_id>[^/]+)/items(?:/(?P<item_id>[^/]+))?|/search)$", | ||
| ) | ||
|
|
||
| def __post_init__(self): | ||
| """Set required conformances based on the filter functions.""" | ||
| for attr in ("collections_filter_path", "items_filter_path"): | ||
| object.__setattr__(self, attr, requests.as_patterns(getattr(self, attr))) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not something we usually see in python 馃槵
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 馃檲 I can unroll the loop to avoid getattr/setattr since it's n=2
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Forgotten from Friday... I could try removing |
||
| for pattern in getattr(self, attr): | ||
| if not re.compile(pattern).groupindex: | ||
| logger.info( | ||
| "Filter path %r declares no named capture groups, " | ||
| "falling back to built-in path param extraction.", | ||
| pattern, | ||
| ) | ||
|
|
||
| required_conformances = set() | ||
| if self.collections_filter: | ||
| logger.debug("Appending required conformance for collections filter") | ||
|
|
@@ -77,7 +91,7 @@ async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: | |
| logger.debug("Skipping CQL2 filter build for OPTIONS request") | ||
| return await self.app(scope, receive, send) | ||
|
|
||
| filter_builder = self._get_filter(request.url.path) | ||
| filter_builder, path_params = self._get_filter(request.url.path) | ||
| if not filter_builder: | ||
| return await self.app(scope, receive, send) | ||
|
|
||
|
|
@@ -88,7 +102,7 @@ async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: | |
| "path": request.url.path, | ||
| "method": request.method, | ||
| "query_params": dict(request.query_params), | ||
| "path_params": requests.extract_variables(request.url.path), | ||
| "path_params": path_params, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The more I think about it the more I think the parsing for path_params should be done in the filter function, so we don't need customization here
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Am I following along here? The way I'm reading this would imply:
We do get the
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @alukach I think this is a great question on scope of ownership that @vincentsarago raised (and thanks Vincent for self-tagging and reviewing so quickly 馃檱!). I'm happy to go either way because filters/ Conceptually I think it'd be cool if this could work similar to how Starlette/FastAPI use regex to match the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @alukach Yes (to all the points) I'm just afraid that we add too much complexity to a library which is already quite complex. |
||
| "headers": dict(request.headers), | ||
| }, | ||
| **scope["state"], | ||
|
|
@@ -112,13 +126,22 @@ async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: | |
|
|
||
| def _get_filter( | ||
| self, path: str | ||
| ) -> Optional[Callable[..., Awaitable[str | dict[str, Any]]]]: | ||
| """Get the CQL2 filter builder for the given path.""" | ||
| ) -> tuple[Optional[Callable[..., Awaitable[str | dict[str, Any]]]], dict]: | ||
| """Get the CQL2 filter builder for the given path and its path params.""" | ||
| endpoint_filters = [ | ||
| (self.collections_filter_path, self.collections_filter), | ||
| (self.items_filter_path, self.items_filter), | ||
| ] | ||
| for expr, builder in endpoint_filters: | ||
| if re.match(expr, path): | ||
| return builder | ||
| return None | ||
| for patterns, builder in endpoint_filters: | ||
| for expr in patterns: | ||
| match = re.match(expr, path) | ||
| if match: | ||
| return builder, self._path_params(match, path) | ||
| return None, {} | ||
|
|
||
| @staticmethod | ||
| def _path_params(match: re.Match, path: str) -> dict: | ||
| """Get the path params declared by a matched pattern's named groups.""" | ||
| if match.re.groupindex: | ||
| return {k: v for k, v in match.groupdict().items() if v is not None} | ||
| return requests.extract_variables(path) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This documentation is great, but perhaps a bit long for the Configuration section. That section is intentionally very short, with basic config descriptions and maybe some tips/notes. This longer-form best content should probably live in
/tipsorarchitecture/filtering-data/and perhaps be linked-to from the the configuration section