fix: restore messages not attached to rpc in selective_gapic_generation#16951
fix: restore messages not attached to rpc in selective_gapic_generation#16951
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the selective GAPIC generation logic by replacing the multi-step allowlist and pruning process with a unified with_selective_generation method across the Proto, Service, and Method classes. This consolidation simplifies the code by handling both internal method marking and service/method omission in a single pass. The review feedback highlights the need for docstrings on the new methods and recommends updating return type hints to Optional to correctly handle cases where objects are filtered out (returning None). Specifically, the Proto implementation should be updated to return None if it becomes empty after filtering.
| def with_selective_generation( | ||
| self, | ||
| *, | ||
| address_allowlist: Set["metadata.Address"], | ||
| method_allowlist: Set[str], | ||
| resource_messages: Dict[str, "wrappers.MessageType"], | ||
| ) -> None: | ||
| """Adds to the set of Addresses of wrapper objects to be included in selective GAPIC generation. | ||
|
|
||
| This method is used to create an allowlist of addresses to be used to filter out unneeded | ||
| services, methods, messages, and enums at a later step. | ||
|
|
||
| Args: | ||
| address_allowlist (Set[metadata.Address]): A set of allowlisted metadata.Address | ||
| objects to add to. Only the addresses of the allowlisted methods, the services | ||
| containing these methods, and messages/enums those methods use will be part of the | ||
| final address_allowlist. The set may be modified during this call. | ||
| method_allowlist (Set[str]): An allowlist of fully-qualified method names. | ||
| resource_messages (Dict[str, wrappers.MessageType]): A dictionary mapping the unified | ||
| resource type name of a resource message to the corresponding MessageType object | ||
| representing that resource message. Only resources with a message representation | ||
| should be included in the dictionary. | ||
| Returns: | ||
| None | ||
| """ | ||
| # The method.operation_service for an extended LRO is not fully qualified, so we | ||
| # truncate the service names accordingly so they can be found in | ||
| # method.add_to_address_allowlist | ||
| services_in_proto = { | ||
| service.name: service for service in self.services.values() | ||
| } | ||
| for service in self.services.values(): | ||
| service.add_to_address_allowlist( | ||
| address_allowlist=address_allowlist, | ||
| method_allowlist=method_allowlist, | ||
| resource_messages=resource_messages, | ||
| services_in_proto=services_in_proto, | ||
| ) | ||
|
|
||
| def prune_messages_for_selective_generation( | ||
| self, *, address_allowlist: Set["metadata.Address"] | ||
| ) -> Optional["Proto"]: | ||
| """Returns a truncated version of this Proto. | ||
|
|
||
| Only the services, messages, and enums contained in the allowlist | ||
| of visited addresses are included in the returned object. If there | ||
| are no services, messages, or enums left, and no file level resources, | ||
| return None. | ||
|
|
||
| Args: | ||
| address_allowlist (Set[metadata.Address]): A set of allowlisted metadata.Address | ||
| objects to filter against. Objects with addresses not the allowlist will be | ||
| removed from the returned Proto. | ||
| Returns: | ||
| Optional[Proto]: A truncated version of this proto. If there are no services, messages, | ||
| or enums left after the truncation process and there are no file level resources, | ||
| returns None. | ||
| """ | ||
| # Once the address allowlist has been created, it suffices to only | ||
| # prune items at 2 different levels to truncate the Proto object: | ||
| # | ||
| # 1. At the Proto level, we remove unnecessary services, messages, | ||
| # and enums. | ||
| # 2. For allowlisted services, at the Service level, we remove | ||
| # non-allowlisted methods. | ||
| services = { | ||
| k: v.prune_messages_for_selective_generation( | ||
| address_allowlist=address_allowlist | ||
| ) | ||
| for k, v in self.services.items() | ||
| if v.meta.address in address_allowlist | ||
| } | ||
|
|
||
| all_messages = { | ||
| k: v for k, v in self.all_messages.items() if v.ident in address_allowlist | ||
| } | ||
|
|
||
| all_enums = { | ||
| k: v for k, v in self.all_enums.items() if v.ident in address_allowlist | ||
| } | ||
|
|
||
| if not services and not all_messages and not all_enums: | ||
| return None | ||
|
|
||
| return dataclasses.replace( | ||
| self, services=services, all_messages=all_messages, all_enums=all_enums | ||
| ) | ||
|
|
||
| def with_internal_methods(self, *, public_methods: Set[str]) -> "Proto": | ||
| """Returns a version of this Proto with some Methods marked as internal. | ||
| generate_omitted_as_internal: bool, | ||
| public_methods: Set[str], | ||
| ) -> "Proto": | ||
|
|
||
| The methods not in the public_methods set will be marked as internal and | ||
| services containing these methods will also be marked as internal by extension. | ||
| (See :meth:`Service.is_internal` for more details). | ||
| services = {} | ||
| for k, v in self.services.items(): | ||
| new_v = v.with_selective_generation( | ||
| generate_omitted_as_internal=generate_omitted_as_internal, | ||
| public_methods=public_methods) | ||
| if new_v: | ||
| services[k] = new_v | ||
|
|
||
| Args: | ||
| public_methods (Set[str]): An allowlist of fully-qualified method names. | ||
| Methods not in this allowlist will be marked as internal. | ||
| Returns: | ||
| Proto: A version of this Proto with Method objects corresponding to methods | ||
| not in `public_methods` marked as internal. | ||
| """ | ||
| services = { | ||
| k: v.with_internal_methods(public_methods=public_methods) | ||
| for k, v in self.services.items() | ||
| } | ||
| return dataclasses.replace(self, services=services) |
There was a problem hiding this comment.
The with_selective_generation method is missing a docstring and its return type hint should be Optional["Proto"] to allow skipping empty protos. This is consistent with the logic in Service.with_selective_generation and the check at line 446. Additionally, it should return None if no services or messages remain when not generating omitted methods as internal.
def with_selective_generation(
self,
*,
generate_omitted_as_internal: bool,
public_methods: Set[str],
) -> Optional["Proto"]:
"""Returns a version of this Proto for selective generation.
Args:
generate_omitted_as_internal (bool): Whether to mark omitted methods as internal.
public_methods (Set[str]): The set of fully-qualified method names to keep as public.
Returns:
Optional[Proto]: A version of this Proto with services/methods filtered.
Returns None if the Proto becomes empty and generate_omitted_as_internal is False.
"""
services = {}
for k, v in self.services.items():
new_v = v.with_selective_generation(
generate_omitted_as_internal=generate_omitted_as_internal,
public_methods=public_methods)
if new_v:
services[k] = new_v
if not generate_omitted_as_internal and not services and not self.all_messages and not self.all_enums:
return None
return dataclasses.replace(self, services=services)References
- When finding a precise type hint that satisfies both mypy and unit tests is not cost-effective, using a less specific type (e.g., Any) is an acceptable trade-off, especially if it improves upon the previous state.
| def with_selective_generation( | ||
| self, | ||
| *, | ||
| address_allowlist: Set["metadata.Address"], | ||
| resource_messages: Dict[str, "MessageType"], | ||
| services_in_proto: Dict[str, "Service"], | ||
| ) -> None: | ||
| """Adds to the allowlist of Addresses of wrapper objects to be included in selective GAPIC generation. | ||
|
|
||
| This method is used to create an allowlist of addresses to be used to filter out unneeded | ||
| services, methods, messages, and enums at a later step. | ||
|
|
||
| Args: | ||
| address_allowlist (Set[metadata.Address]): A set of allowlisted metadata.Address | ||
| objects to add to. Only the addresses of the allowlisted methods, the services | ||
| containing these methods, and messages/enums those methods use will be part of the | ||
| final address_allowlist. The set may be modified during this call. | ||
| method_allowlist (Set[str]): An allowlist of fully-qualified method names. | ||
| resource_messages (Dict[str, wrappers.MessageType]): A dictionary mapping the unified | ||
| resource type name of a resource message to the corresponding MessageType object | ||
| representing that resource message. Only resources with a message representation | ||
| should be included in the dictionary. | ||
| services_in_proto (Dict[str, wrappers.Service]): A dictionary mapping the names of Service | ||
| objects in the proto containing this method to the Service objects. This is necessary | ||
| for traversing the operation service in the case of extended LROs. | ||
| Returns: | ||
| None | ||
| """ | ||
|
|
||
| address_allowlist.add(self.ident) | ||
|
|
||
| if self.lro: | ||
| self.lro.add_to_address_allowlist( | ||
| address_allowlist=address_allowlist, resource_messages=resource_messages | ||
| ) | ||
|
|
||
| if self.extended_lro and self.operation_service: | ||
| # We need to add the service/method pointed to by self.operation_service to | ||
| # the allowlist, as it might not have been specified by | ||
| # the methods under selective_gapic_generation. | ||
| # We assume that the operation service lives in the same proto file as this one. | ||
| operation_service = services_in_proto[self.operation_service] | ||
| address_allowlist.add(operation_service.meta.address) | ||
| operation_service.operation_polling_method.add_to_address_allowlist( | ||
| address_allowlist=address_allowlist, | ||
| resource_messages=resource_messages, | ||
| services_in_proto=services_in_proto, | ||
| ) | ||
|
|
||
| self.extended_lro.add_to_address_allowlist( | ||
| address_allowlist=address_allowlist, | ||
| resource_messages=resource_messages, | ||
| ) | ||
|
|
||
| self.input.add_to_address_allowlist( | ||
| address_allowlist=address_allowlist, | ||
| resource_messages=resource_messages, | ||
| ) | ||
|
|
||
| self.output.add_to_address_allowlist( | ||
| address_allowlist=address_allowlist, | ||
| resource_messages=resource_messages, | ||
| ) | ||
|
|
||
| def with_internal_methods(self, *, public_methods: Set[str]) -> "Method": | ||
| """Returns a version of this ``Method`` marked as internal | ||
|
|
||
| The methods not in the public_methods set will be marked as internal and | ||
| this ``Service`` will as well by extension (see :meth:`Service.is_internal`). | ||
| generate_omitted_as_internal: bool, | ||
| public_methods: Set[str], | ||
| ) -> "Method": | ||
|
|
||
| Args: | ||
| public_methods (Set[str]): An allowlist of fully-qualified method names. | ||
| Methods not in this allowlist will be marked as internal. | ||
| Returns: | ||
| Service: A version of this `Service` with `Method` objects corresponding to methods | ||
| not in `public_methods` marked as internal. | ||
| """ | ||
| if self.ident.proto in public_methods: | ||
| return self | ||
|
|
||
| return dataclasses.replace( | ||
| self, | ||
| is_internal=True, | ||
| ) | ||
| # Not public | ||
| if generate_omitted_as_internal: | ||
| return dataclasses.replace( | ||
| self, | ||
| is_internal=True, | ||
| ) | ||
| else: | ||
| return None |
There was a problem hiding this comment.
The with_selective_generation method is missing a docstring and the return type hint should be Optional["Method"] since it returns None when a method is omitted and not marked as internal.
def with_selective_generation(
self,
*,
generate_omitted_as_internal: bool,
public_methods: Set[str],
) -> Optional["Method"]:
"""Returns a version of this Method for selective generation.
Args:
generate_omitted_as_internal (bool): Whether to mark omitted methods as internal.
public_methods (Set[str]): The set of fully-qualified method names to keep as public.
Returns:
Optional[Method]: The method (possibly marked internal), or None if it should be removed.
"""
if self.ident.proto in public_methods:
return self
# Not public
if generate_omitted_as_internal:
return dataclasses.replace(
self,
is_internal=True,
)
else:
return NoneReferences
- When finding a precise type hint that satisfies both mypy and unit tests is not cost-effective, using a less specific type (e.g., Any) is an acceptable trade-off, especially if it improves upon the previous state.
| def with_selective_generation( | ||
| self, | ||
| *, | ||
| address_allowlist: Set["metadata.Address"], | ||
| method_allowlist: Set[str], | ||
| resource_messages: Dict[str, "MessageType"], | ||
| services_in_proto: Dict[str, "Service"], | ||
| ) -> None: | ||
| """Adds to the allowlist of Addresses of wrapper objects to be included in selective GAPIC generation. | ||
|
|
||
| This method is used to create an allowlist of addresses to be used to filter out unneeded | ||
| services, methods, messages, and enums at a later step. | ||
|
|
||
| Args: | ||
| address_allowlist (Set[metadata.Address]): A set of allowlisted metadata.Address | ||
| objects to add to. Only the addresses of the allowlisted methods, the services | ||
| containing these methods, and messages/enums those methods use will be part of the | ||
| final address_allowlist. The set may be modified during this call. | ||
| method_allowlist (Set[str]): An allowlist of fully-qualified method names. | ||
| resource_messages (Dict[str, wrappers.MessageType]): A dictionary mapping the unified | ||
| resource type name of a resource message to the corresponding MessageType object | ||
| representing that resource message. Only resources with a message representation | ||
| should be included in the dictionary. | ||
| services_in_proto (Dict[str, Service]): | ||
| Returns: | ||
| None | ||
| """ | ||
|
|
||
| for method in self.methods.values(): | ||
| if method.ident.proto in method_allowlist: | ||
| # Include this service if there are any types/methods in selective gapic for this service. | ||
| address_allowlist.add(self.meta.address) | ||
| method.add_to_address_allowlist( | ||
| address_allowlist=address_allowlist, | ||
| resource_messages=resource_messages, | ||
| services_in_proto=services_in_proto, | ||
| ) | ||
|
|
||
| def prune_messages_for_selective_generation( | ||
| self, *, address_allowlist: Set["metadata.Address"] | ||
| generate_omitted_as_internal: bool, | ||
| public_methods: Set[str], | ||
| ) -> "Service": | ||
| """Returns a truncated version of this Service. | ||
|
|
||
| Only the methods, messages, and enums contained in the address allowlist | ||
| are included in the returned object. | ||
|
|
||
| Args: | ||
| address_allowlist (Set[metadata.Address]): A set of allowlisted metadata.Address | ||
| objects to filter against. Objects with addresses not the allowlist will be | ||
| removed from the returned Proto. | ||
| Returns: | ||
| Service: A truncated version of this proto. | ||
| """ | ||
| return dataclasses.replace( | ||
| self, | ||
| methods={ | ||
| k: v for k, v in self.methods.items() if v.ident in address_allowlist | ||
| }, | ||
| ) | ||
|
|
||
| def with_internal_methods(self, *, public_methods: Set[str]) -> "Service": | ||
| """Returns a version of this ``Service`` with some Methods marked as internal. | ||
| methods = {} | ||
| for k, v in self.methods.items(): | ||
| new_v = v.with_selective_generation( | ||
| generate_omitted_as_internal=generate_omitted_as_internal, | ||
| public_methods=public_methods) | ||
| if new_v: | ||
| methods[k] = new_v | ||
|
|
||
| The methods not in the public_methods set will be marked as internal and | ||
| this ``Service`` will as well by extension (see :meth:`Service.is_internal`). | ||
| if not generate_omitted_as_internal and not methods: | ||
| return None | ||
|
|
||
| Args: | ||
| public_methods (Set[str]): An allowlist of fully-qualified method names. | ||
| Methods not in this allowlist will be marked as internal. | ||
| Returns: | ||
| Service: A version of this `Service` with `Method` objects corresponding to methods | ||
| not in `public_methods` marked as internal. | ||
| """ | ||
| return dataclasses.replace( | ||
| self, | ||
| methods={ | ||
| k: v.with_internal_methods(public_methods=public_methods) | ||
| for k, v in self.methods.items() | ||
| }, | ||
| ) | ||
| return dataclasses.replace(self, methods=methods) |
There was a problem hiding this comment.
The with_selective_generation method is missing a docstring and the return type hint should be Optional["Service"] since it returns None when a service has no methods left and generate_omitted_as_internal is False.
def with_selective_generation(
self,
*,
generate_omitted_as_internal: bool,
public_methods: Set[str],
) -> Optional["Service"]:
"""Returns a version of this Service for selective generation.
Args:
generate_omitted_as_internal (bool): Whether to mark omitted methods as internal.
public_methods (Set[str]): The set of fully-qualified method names to keep as public.
Returns:
Optional[Service]: The service with filtered methods, or None if it should be removed.
"""
methods = {}
for k, v in self.methods.items():
new_v = v.with_selective_generation(
generate_omitted_as_internal=generate_omitted_as_internal,
public_methods=public_methods)
if new_v:
methods[k] = new_v
if not generate_omitted_as_internal and not methods:
return None
return dataclasses.replace(self, methods=methods)References
- When finding a precise type hint that satisfies both mypy and unit tests is not cost-effective, using a less specific type (e.g., Any) is an acceptable trade-off, especially if it improves upon the previous state.
Towards b/507893758, b/507889482, b/501132869, b/503326310