feat: adds dynamic routing. #1135
Conversation
|
This PR is almost ready, but misses the following:
I will make changes for 1) after I get an approval of the method used in regular templates. As far as 2), I'm currently running into |
gapic/schema/wrappers.py
Outdated
| # Only 1 named segment is allowed and so only 1 key. | ||
| return list(regex.groupindex)[0] if list(regex.groupindex) else self.field |
There was a problem hiding this comment.
Nit: maybe lift list(regex.groupindex) before the conditional
There was a problem hiding this comment.
I don't know a good variable name.
There was a problem hiding this comment.
What about group_names
gapic/schema/wrappers.py
Outdated
| Args: | ||
| path_template (str): A path template corresponding to a resource name. | ||
| It can only have 0 or 1 named segments. It can not contain complex resource ID path segments. | ||
| See https://google.aip.dev/122 and https://google.aip.dev/client-libraries/4231 for more details. |
There was a problem hiding this comment.
I believe aip/4222 is also relevant
| assert args[0] == request | ||
|
|
||
| _, _, kw = call.mock_calls[0] | ||
| # This test doesn't assert anything useful. |
There was a problem hiding this comment.
Do we just want to make sure that the metadata is there?
There was a problem hiding this comment.
No, ideally, we want to assert correct x-goog-parameters are set. However, it's impossible to do it since the headers are set using complicated process difficult to mock.
| ([wrappers.RoutingParameter("table_name", "{project_id=projects/*}/instances/*/**")], | ||
| RoutingTestRequest(table_name="projects/100/instances/200/tables/300"), | ||
| {"project_id": "projects/100"}), | ||
| ([wrappers.RoutingParameter("table_name", "{project_id=projects/*}/instances/*/**"), | ||
| wrappers.RoutingParameter( | ||
| "table_name", "projects/*/{instance_id=instances/*}/**"), | ||
| ], | ||
| RoutingTestRequest(table_name="projects/100/instances/200/tables/300"), | ||
| {"project_id": "projects/100", "instance_id": "instances/200"}) |
There was a problem hiding this comment.
This is a little hard to eyeball parse. Can you reformat it using black?
There was a problem hiding this comment.
I ran black, but it looks like style_check CI test fails. So, looks like the file needs to be formatted using find gapic tests -name "*.py" -not -path 'tests/**/goldens/*' | xargs autopep8 --in-place
| if self.path_template == "": | ||
| return self.field |
There was a problem hiding this comment.
Is this code path ever tested?
There was a problem hiding this comment.
Yes, the coverage is 100%
Can you elaborate? I think path template in routing parameter always uses '/'. |
gapic/schema/wrappers.py
Outdated
| def _convert_to_regex(self, path_template): | ||
| if self._how_many_named_segments(path_template) > 1: | ||
| # This also takes care of complex patterns (i.e. {foo}~{bar}) | ||
| raise ValueError("There should be exactly one named segment.") |
There was a problem hiding this comment.
Please add some more context when raising the exception
gapic/schema/wrappers.py
Outdated
| # Only 1 named segment is allowed and so only 1 key. | ||
| return list(regex.groupindex)[0] if list(regex.groupindex) else self.field |
There was a problem hiding this comment.
What about group_names
| assert isinstance( | ||
| method.routing_rule.routing_parameters[0], wrappers.RoutingParameter) |
There was a problem hiding this comment.
I don't think this check is necessary; the check above tests equality, which should check the type since you haven't overridden __equal__.
Adds dynamic routing feature.