feat: configurable path param regex - #208
Conversation
|
Claude finished @ceholden's task in 3m 48s —— View job Reviewing PR #208
✅ No blocking issues — safe to merge. Traced the full path-param flow ( Non-blocking observations:
Simplify (ponytail)
💰 Estimated review cost: $0.75 · 3m48s · 30 turns |
43d66bf to
fb5f2ef
Compare
| collections_filter_path: str = r"^/collections(/[^/]+)?$" | ||
| collections_filter_path: Union[str, Sequence[str]] = ( | ||
| r"^/collections(?:/(?P<collection_id>[^/]+))?$", | ||
| ) |
There was a problem hiding this comment.
can the usage of sequence be avoided by using | in regex?
There was a problem hiding this comment.
This would break if we have the same named capture group in each arm of the regex,
>>> parts = [
... r"^/collections/(?P<collection_id>[^/]+)/aggregate$",
... r"^/collections/(?P<collection_id>[^/]+)/aggregations$",
... ]
# Compiles fine
>>> for p in parts:
... re.compile(p)
...
re.compile('^/collections/(?P<collection_id>[^/]+)/aggregate$')
re.compile('^/collections/(?P<collection_id>[^/]+)/aggregations$')
# Boom!
>>> re.compile("|".join(parts))
...
re.PatternError: redefinition of group name 'collection_id' as group 2; was group 1 at position 68| 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))) |
There was a problem hiding this comment.
This is not something we usually see in python 😬
There was a problem hiding this comment.
🙈 I can unroll the loop to avoid getattr/setattr since it's n=2
There was a problem hiding this comment.
Forgotten from Friday... object.__setattr__ is needed since the class is frozen. Worth a comment at least!
I could try removing frozen=True on the class, keep it as it is, or switch this to be a Pydantic dataclass with a "before" validator
| "method": request.method, | ||
| "query_params": dict(request.query_params), | ||
| "path_params": requests.extract_variables(request.url.path), | ||
| "path_params": path_params, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Am I following along here? The way I'm reading this would imply:
path_paramsas it exists now would only be relevant for core (+ filter) STAC API endpoints- Anything beyond that would have to rerun the regex that handles matching on the endpoint in order to extract the pattern
- The consequence is the reuse of the covering regex with the extraction regex in this PR wouldn't be needed, so we could close as not needed
- This stance would consider the queryables / filter extension support as a special case that we're not going to extend further
We do get the request.url.path so it's totally possible if we want to limit the scope here! Or, am I misunderstanding what you meant?
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #208 +/- ##
==========================================
+ Coverage 89.65% 89.90% +0.25%
==========================================
Files 30 30
Lines 1343 1377 +34
Branches 182 191 +9
==========================================
+ Hits 1204 1238 +34
Misses 97 97
Partials 42 42
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Description
Closes #206
This PR extends the
collections_path_filteranditems_path_filterto handle both, (1) the scoping/coverage of auth requirements and (2) the data extraction from those paths.#204 is related to this change since it updated the data extraction to support anyone who added "queryables" from the Filter extension to the
collections_path_filter, but it isn't necessarily replaced by this PR. I think it's good to cover the Filter API extension out of the box since it's so common. This PR is more intended to support any endpoint hosted alongside STAC API routes.How it works
The user can provide named capture groups inside the regular expressions patterns
collections_path_filteranditems_path_filterto control how the data extraction happens. This gives the user an ability to override the hard coded behavior of theutils.requests.extract_variables()function.Including named capture groups implies supporting a plural number of filter patterns. We can match on many endpoint path styles using just one regex, but we cannot have the same named capture group appear multiple times in the same regular expression. Accordingly the type signatures and some of the handling had to be expanded to support more than one pattern. For example,
find_matchmoves to anany(re.match(pattern, path) for pattern in patterns)instead of checking a singlere.match(pattern, path).I took as an assumption that this should be backwards compatible. Anyone who overrides
collections_path_filter/items_path_filterwithout using named capture groups will default to use theutils.requests.extract_variables()function as a fallback. If this fallback didn't exist then regex without named capture groups would provide no data, breaking their evaluations. I can remove this backup if it'd be better to make this a v2 major version breaking change.The use case I have isn't for the STAC API aggregation extension, but I used that as an example in the updated configuration documentation since it's similar enough and a published STAC API extension.
🤖 disclosure: I used a LLM to assist investigating a solution, to write unit tests, and to provide review feedback, but I've reviewed and modified any generated code.