Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .generator/requirements.in
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
click
gapic-generator==1.30.13 # https://github.com/googleapis/gapic-generator-python/releases/tag/v1.30.13
-e /usr/local/google/home/omairn/git/googleapis/google-cloud-python/packages/gapic-generator
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The use of a hardcoded local absolute path (-e /usr/local/google/...) will break the build for other developers and in CI/CD environments. Please revert this to a versioned dependency or a relative path if necessary for the experiment.

gapic-generator==1.30.13 # https://github.com/googleapis/gapic-generator-python/releases/tag/v1.30.13

nox
starlark-pyo3>=2025.1
build
Expand Down
25 changes: 25 additions & 0 deletions .librarian/generate-request.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
{
"id": "google-cloud-dialogflow",
"version": "2.47.0",
"apis": [
{
"path": "google/cloud/dialogflow/v2beta1",
"service_config": "dialogflow_v2beta1.yaml"
},
{
"path": "google/cloud/dialogflow/v2",
"service_config": "dialogflow_v2.yaml"
}
],
"source_roots": [
"packages/google-cloud-dialogflow"
],
"preserve_regex": [
"packages/google-cloud-dialogflow/CHANGELOG.md",
"docs/CHANGELOG.md"
],
"remove_regex": [
"packages/google-cloud-dialogflow/"
],
"tag_format": "{id}-v{version}"
}
9 changes: 6 additions & 3 deletions packages/gapic-generator/gapic/schema/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -170,9 +170,12 @@ def resource_messages(self) -> Mapping[str, wrappers.MessageType]:
if msg.options.Extensions[resource_pb2.resource].type
)
return collections.OrderedDict(
itertools.chain(
file_resource_messages,
resource_messages,
sorted(
itertools.chain(
file_resource_messages,
resource_messages,
),
key=lambda item: item[0]
)
)
Comment on lines 172 to 180
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

In modern Python (3.7+), the standard dict preserves insertion order. Following the general rules for this repository, you should use dict(sorted(...)) instead of collections.OrderedDict to ensure determinism more concisely.

Suggested change
return collections.OrderedDict(
itertools.chain(
file_resource_messages,
resource_messages,
sorted(
itertools.chain(file_resource_messages, resource_messages),
key=lambda item: item[0]
)
)
return dict(
sorted(
itertools.chain(file_resource_messages, resource_messages),
key=lambda item: item[0]
)
)
References
  1. To ensure dictionary keys remain sorted without manual effort, programmatically sort the dictionary before returning it (e.g., using dict(sorted(metadata.items()))) instead of relying on manual ordering in the code.


Expand Down
41 changes: 37 additions & 4 deletions packages/gapic-generator/gapic/schema/wrappers.py
Original file line number Diff line number Diff line change
Expand Up @@ -685,10 +685,36 @@ def resource_path(self) -> Optional[str]:
If there are multiple paths, returns the first one."""
return next(iter(self.options.Extensions[resource_pb2.resource].pattern), None)

def _apply_domain_heuristic(self, raw_type: str) -> str:
"""Determines if a resource is foreign and adds a prefix to prevent
[no-redef] AST collisions."""
if not raw_type:
return ""

if "/" not in raw_type:
return raw_type

# Extract the root domain and final resource name, bypassing any nested paths.
# (e.g., "ces.googleapis.com/Project/Location/Tool" -> "ces.googleapis.com" and "Tool")
resource_parts = raw_type.split('/')
domain, short_name = resource_parts[0], resource_parts[-1]
domain_prefix = domain.split('.', 1)[0]

try:
native_package = self.meta.address.package
# 2. If the domain prefix isn't natively in the package namespace, it's foreign
if domain_prefix and native_package and domain_prefix not in native_package:
return f"{domain_prefix}_{short_name}"
except (AttributeError, TypeError):
# 3. Safe fallback if meta, address, or package are missing/None on this wrapper
pass

return short_name

@property
def resource_type(self) -> Optional[str]:
resource = self.options.Extensions[resource_pb2.resource]
return resource.type[resource.type.find("/") + 1 :] if resource else None
return self._apply_domain_heuristic(resource.type) if resource else None

@property
def resource_type_full_path(self) -> Optional[str]:
Expand Down Expand Up @@ -2278,9 +2304,9 @@ def names(self) -> FrozenSet[str]:
return frozenset(answer)

@utils.cached_property
def resource_messages(self) -> FrozenSet[MessageType]:
def resource_messages(self) -> Sequence['MessageType']:
"""Returns all the resource message types used in all
request and response fields in the service."""
request and response fields in the service, deterministically sorted."""

def gen_resources(message):
if message.resource_path:
Expand All @@ -2301,7 +2327,7 @@ def gen_indirect_resources_used(message):
if resource:
yield resource

return frozenset(
unique_messages = frozenset(
msg
for method in self.methods.values()
for msg in chain(
Expand All @@ -2316,6 +2342,13 @@ def gen_indirect_resources_used(message):
)
)

return tuple(
sorted(
unique_messages,
key=lambda m: m.resource_type_full_path or m.name
)
)

@utils.cached_property
def resource_messages_dict(self) -> Dict[str, MessageType]:
"""Returns a dict from resource reference to
Expand Down
16 changes: 8 additions & 8 deletions packages/gapic-generator/tests/unit/schema/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -1740,17 +1740,17 @@ def test_file_level_resources():
expected = collections.OrderedDict(
(
(
"nomenclature.linnaen.com/Species",
"nomenclature.linnaen.com/Phylum",
wrappers.CommonResource(
type_name="nomenclature.linnaen.com/Species",
pattern="families/{family}/genera/{genus}/species/{species}",
type_name="nomenclature.linnaen.com/Phylum",
pattern="kingdoms/{kingdom}/phyla/{phylum}",
).message_type,
),
(
"nomenclature.linnaen.com/Phylum",
"nomenclature.linnaen.com/Species",
wrappers.CommonResource(
type_name="nomenclature.linnaen.com/Phylum",
pattern="kingdoms/{kingdom}/phyla/{phylum}",
type_name="nomenclature.linnaen.com/Species",
pattern="families/{family}/genera/{genus}/species/{species}",
).message_type,
),
)
Expand All @@ -1767,7 +1767,7 @@ def test_file_level_resources():
# The service doesn't own any method that owns a message that references
# Phylum, so the service doesn't count it among its resource messages.
expected.pop("nomenclature.linnaen.com/Phylum")
expected = frozenset(expected.values())
expected = tuple(expected.values())
actual = service.resource_messages

assert actual == expected
Expand Down Expand Up @@ -1822,7 +1822,7 @@ def test_resources_referenced_but_not_typed(reference_attr="type"):
name_resource_opts.child_type = species_resource_opts.type

api_schema = api.API.build([fdp], package="nomenclature.linneaen.v1")
expected = {api_schema.messages["nomenclature.linneaen.v1.Species"]}
expected = (api_schema.messages["nomenclature.linneaen.v1.Species"],)
actual = api_schema.services[
"nomenclature.linneaen.v1.SpeciesService"
].resource_messages
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ def test_resource_path():
resource.pattern.append("kingdoms/{kingdom}/phyla/{phylum}/classes/{klass}")
resource.pattern.append("kingdoms/{kingdom}/divisions/{division}/classes/{klass}")
resource.type = "taxonomy.biology.com/Class"
message = make_message("Squid", options=options)
message = make_message("Squid", options=options, package="taxonomy.biology.v1")

assert message.resource_path == "kingdoms/{kingdom}/phyla/{phylum}/classes/{klass}"
assert message.resource_path_args == ["kingdom", "phylum", "klass"]
Expand All @@ -201,7 +201,7 @@ def test_resource_path_with_wildcard():
resource.pattern.append("kingdoms/{kingdom}/phyla/{phylum}/classes/{klass=**}")
resource.pattern.append("kingdoms/{kingdom}/divisions/{division}/classes/{klass}")
resource.type = "taxonomy.biology.com/Class"
message = make_message("Squid", options=options)
message = make_message("Squid", options=options, package="taxonomy.biology.v1")

assert (
message.resource_path == "kingdoms/{kingdom}/phyla/{phylum}/classes/{klass=**}"
Expand Down Expand Up @@ -230,7 +230,7 @@ def test_resource_path_pure_wildcard():
resource = options.Extensions[resource_pb2.resource]
resource.pattern.append("*")
resource.type = "taxonomy.biology.com/Class"
message = make_message("Squid", options=options)
message = make_message("Squid", options=options, package="taxonomy.biology.v1")

# Pure wildcard resource names do not really help construct resources
# but they are a part of the spec so we need to support them, which means at
Expand Down Expand Up @@ -473,3 +473,60 @@ def test_extended_operation_request_response_fields():

actual = poll_request.extended_operation_response_fields
assert actual == expected


@pytest.mark.parametrize(
"raw_type, package_tuple, expected",
[
# 1. Empty or malformed inputs
("", ("google", "cloud", "dialogflow", "v2"), ""),
("Tool", ("google", "cloud", "dialogflow", "v2"), "Tool"),

# 2. Native Resources (Prefix 'dialogflow' IS in the package tuple)
("dialogflow.googleapis.com/Tool", ("google", "cloud", "dialogflow", "v2"), "Tool"),
("dialogflow.googleapis.com/Project/Location/Tool", ("google", "cloud", "dialogflow", "v2"), "Tool"),

# 3. Foreign Resources (Prefix 'ces' is NOT in the package tuple)
("ces.googleapis.com/Tool", ("google", "cloud", "dialogflow", "v2"), "ces_Tool"),
("ces.googleapis.com/Project/Location/Tool", ("google", "cloud", "dialogflow", "v2"), "ces_Tool"),
]
)
def test_apply_domain_heuristic(raw_type, package_tuple, expected):
meta = metadata.Metadata(
address=metadata.Address(
name="TestMessage",
package=package_tuple,
module="test",
)
)
message = make_message("TestMessage", meta=meta)

actual = message._apply_domain_heuristic(raw_type)
assert actual == expected


def test_apply_domain_heuristic_none_package():
"""Test the EAFP failsafe if package is explicitly None (TypeError)."""
meta = metadata.Metadata(
address=metadata.Address(
name="TestMessage",
package=None,
module="test",
)
)
message = make_message("TestMessage", meta=meta)

actual = message._apply_domain_heuristic("ces.googleapis.com/Tool")
assert actual == "Tool"


def test_apply_domain_heuristic_missing_meta():
"""Test the EAFP failsafe if meta or address are missing (AttributeError)."""
# To test the AttributeError fallback cleanly without mocks, we can just
# pass a bare-bones Python class to the unbound method. This perfectly
# proves the try/except block safely handles totally missing attributes.
class EmptyMessage:
pass

actual = wrappers.MessageType._apply_domain_heuristic(EmptyMessage(), "ces.googleapis.com/Tool")
assert actual == "Tool"
Original file line number Diff line number Diff line change
Expand Up @@ -267,12 +267,12 @@ def test_resource_messages():
),
)

expected = {
squid_resource,
expected = (
clam_resource,
whelk_resource,
squamosa_message,
}
squid_resource,
whelk_resource,
)
actual = service.resource_messages
assert expected == actual

Expand Down Expand Up @@ -557,7 +557,7 @@ def test_resource_response():
),
)

expected = {squid_resource, clam_resource}
expected = (clam_resource, squid_resource)
actual = mollusc_service.resource_messages
assert expected == actual

Expand Down
Loading