From 2b2163122678add1a4e34ae1f52b39b9a23558a0 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Tue, 14 May 2024 14:08:51 -0400 Subject: [PATCH 1/2] Revert "Revert "feat(hc): Introduce CI job to check for RPC backwards compatibility (#68337)"" This reverts commit 945acb5a9cbd2595f0f40754043edc813ea7b50d. --- .github/workflows/openapi-diff.yml | 30 +++++- requirements-dev-frozen.txt | 1 + requirements-dev.txt | 1 + src/sentry/runner/commands/rpcschema.py | 118 ++++++++++++++++++++++++ src/sentry/runner/main.py | 1 + src/sentry/services/hybrid_cloud/rpc.py | 50 ++++++++-- src/sentry/services/hybrid_cloud/sig.py | 10 ++ tests/sentry/hybridcloud/test_sig.py | 30 ++++++ 8 files changed, 231 insertions(+), 10 deletions(-) create mode 100644 src/sentry/runner/commands/rpcschema.py create mode 100644 tests/sentry/hybridcloud/test_sig.py diff --git a/.github/workflows/openapi-diff.yml b/.github/workflows/openapi-diff.yml index 31f0444287643b..9efea3dc88d5ca 100644 --- a/.github/workflows/openapi-diff.yml +++ b/.github/workflows/openapi-diff.yml @@ -19,10 +19,6 @@ jobs: timeout-minutes: 90 steps: - uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1 - with: - # Avoid codecov error message related to SHA resolution: - # https://github.com/codecov/codecov-bash/blob/7100762afbc822b91806a6574658129fe0d23a7d/codecov#L891 - fetch-depth: '2' - name: Check for python file changes uses: dorny/paths-filter@0bc4621a3135347011ad047f9ecf449bf72ce2bd # v3.0.0 @@ -58,3 +54,29 @@ jobs: if: steps.changes.outputs.api_docs == 'true' run: | npx json-diff@0.5.4 --color sentry-api-schema/openapi-derefed.json tests/apidocs/openapi-derefed.json + + # Do RPC compatibility checks. Combined with public API audits + # to avoid workflow/job setup overhead. + - name: Dump RPC schema for new version + id: newschema + if: steps.changes.outputs.api_docs == 'true' + run: | + mkdir schemas + sentry rpcschema --partial > schemas/new_schema.json + + - name: Dump RPC schema for master + id: oldschema + if: steps.changes.outputs.api_docs == 'true' + run: | + git fetch origin + git switch master + sentry rpcschema --partial > schemas/old_schema.json + + - name: Invoke oasdiff on RPC schemas + uses: oasdiff/oasdiff-action/breaking@fc826b9f0d21b85b085842521c7a8cc445412c08 # v0.0.19 + if: steps.changes.outputs.api_docs == 'true' + with: + base: ./schemas/old_schema.json + revision: ./schemas/new_schema.json + format: 'text' + fail-on-diff: true diff --git a/requirements-dev-frozen.txt b/requirements-dev-frozen.txt index 47270c6d094104..8104d19ee857c6 100644 --- a/requirements-dev-frozen.txt +++ b/requirements-dev-frozen.txt @@ -106,6 +106,7 @@ nodeenv==1.8.0 oauthlib==3.1.0 openai==1.3.5 openapi-core==0.18.2 +openapi-pydantic==0.4.0 openapi-schema-validator==0.6.2 openapi-spec-validator==0.7.1 orjson==3.10.0 diff --git a/requirements-dev.txt b/requirements-dev.txt index da2ddb786e2802..cafafdcaeb2ae4 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -7,6 +7,7 @@ docker>=6 time-machine>=2.13.0 honcho>=1.1.0 openapi-core>=0.18.2 +openapi-pydantic>=0.4.0 pytest>=8 pytest-cov>=4.0.0 pytest-django>=4.8.0 diff --git a/src/sentry/runner/commands/rpcschema.py b/src/sentry/runner/commands/rpcschema.py new file mode 100644 index 00000000000000..e0e9044e4430c9 --- /dev/null +++ b/src/sentry/runner/commands/rpcschema.py @@ -0,0 +1,118 @@ +import sys +import traceback +from collections.abc import Iterable +from dataclasses import dataclass +from typing import Any + +import click +from django.urls import reverse +from openapi_pydantic import OpenAPI +from openapi_pydantic.util import PydanticSchema, construct_open_api_with_schema_class + +from sentry.runner.decorators import configuration +from sentry.utils import json + + +@click.command("rpcschema") +@click.option( + "--partial", + is_flag=True, + default=False, + help="Ignore RPC methods that produce errors.", +) +@click.option( + "--diagnose", + is_flag=True, + default=False, + help="List RPC methods that produce errors and suppress all other output.", +) +@configuration +def rpcschema(diagnose: bool, partial: bool) -> None: + from sentry.services.hybrid_cloud.rpc import ( + RpcMethodSignature, + list_all_service_method_signatures, + ) + + @dataclass + class RpcSchemaEntry: + sig: RpcMethodSignature + + @property + def api_path(self) -> str: + return reverse( + "sentry-api-0-rpc-service", args=(self.sig.service_key, self.sig.method_name) + ) + + def build_api_entry(self) -> dict[str, Any]: + param_schema, return_schema = self.sig.get_schemas() + return { + "post": { + "description": "Execute an RPC", + "requestBody": { + "content": { + "application/json": { + "schema": PydanticSchema(schema_class=param_schema) + } + }, + }, + "responses": { + "200": { + "description": "Success", + "content": { + "application/json": { + "schema": PydanticSchema(schema_class=return_schema) + } + }, + } + }, + } + } + + def create_spec(signatures: Iterable[RpcMethodSignature]) -> dict[str, Any]: + entries = [RpcSchemaEntry(sig) for sig in signatures] + path_dict = {entry.api_path: entry.build_api_entry() for entry in entries} + + spec = OpenAPI.parse_obj( + dict( + info=dict( + title="Sentry Internal RPC APIs", + version="0.0.1", + ), + servers=[dict(url="https://sentry.io/")], # TODO: Generify with setting value + paths=path_dict, + ) + ) + spec = construct_open_api_with_schema_class(spec) + return spec.dict(by_alias=True, exclude_none=True) + + def create_partial_spec( + signatures: Iterable[RpcMethodSignature], + ) -> tuple[dict[str, Any], list[str]]: + stable_signatures: list[RpcMethodSignature] = [] + error_reports: list[str] = [] + for sig in signatures: + try: + create_spec([sig]) + except Exception as e: + last_line = str(e).split("\n")[-1].strip() + error_reports.append(f"{sig!s}: {last_line}") + if not diagnose: + traceback.print_exc() + else: + stable_signatures.append(sig) + + return create_spec(stable_signatures), error_reports + + all_signatures = list_all_service_method_signatures() + + if diagnose or partial: + spec, error_reports = create_partial_spec(all_signatures) + if diagnose: + print(f"Error count: {len(error_reports)}") # noqa + for bad_sig in error_reports: + print("- " + bad_sig) # noqa + else: + spec = create_spec(all_signatures) + + if not diagnose: + json.dump(spec, sys.stdout) diff --git a/src/sentry/runner/main.py b/src/sentry/runner/main.py index 5ef63248ab7c95..b35827f5ebd0bf 100644 --- a/src/sentry/runner/main.py +++ b/src/sentry/runner/main.py @@ -57,6 +57,7 @@ def cli(config: str) -> None: "sentry.runner.commands.plugins.plugins", "sentry.runner.commands.queues.queues", "sentry.runner.commands.repair.repair", + "sentry.runner.commands.rpcschema.rpcschema", "sentry.runner.commands.run.run", "sentry.runner.commands.start.start", "sentry.runner.commands.tsdb.tsdb", diff --git a/src/sentry/services/hybrid_cloud/rpc.py b/src/sentry/services/hybrid_cloud/rpc.py index 2c5e06d865c587..9df5fc085f3d9f 100644 --- a/src/sentry/services/hybrid_cloud/rpc.py +++ b/src/sentry/services/hybrid_cloud/rpc.py @@ -5,6 +5,7 @@ import hmac import inspect import logging +import pkgutil from abc import abstractmethod from collections.abc import Callable, Iterable, Iterator, Mapping, MutableMapping, Sequence from contextlib import contextmanager @@ -67,8 +68,26 @@ def _setup_exception(self, message: str) -> RpcServiceSetupException: self.base_service_cls.__name__, self.base_function.__name__, message ) + @property + def service_key(self) -> str: + return self.base_service_cls.key + + @property + def service_name(self) -> str: + return self.base_service_cls.__name__ + + @property + def method_name(self) -> str: + return self.base_function.__name__ + + def __repr__(self) -> str: + return f"{type(self).__name__}({self.service_name!r}, {self.method_name!r})" + + def __str__(self) -> str: + return f"{self.service_name}.{self.method_name}" + def get_name_segments(self) -> Sequence[str]: - return (self.base_service_cls.__name__, self.base_function.__name__) + return self.service_name, self.method_name def _extract_region_resolution(self) -> RegionResolutionStrategy | None: region_resolution = getattr(self.base_function, _REGION_RESOLUTION_ATTR, None) @@ -123,6 +142,9 @@ def __init__( def local_mode(self) -> SiloMode: return self._base_service_cls.local_mode + def __repr__(self): + return f"{type(self).__name__}({self._base_service_cls.__name__})" + def deserialize_rpc_arguments( self, method_name: str, serial_arguments: ArgumentDict ) -> pydantic.BaseModel: @@ -133,6 +155,9 @@ def deserialize_rpc_response(self, method_name: str, serial_response: Any) -> An signature = self._signatures[method_name] return signature.deserialize_return_value(serial_response) + def get_all_signatures(self) -> Iterable[RpcMethodSignature]: + return self._signatures.values() + def rpc_method(method: Callable[..., _T]) -> Callable[..., _T]: """Decorate methods to be exposed as part of the RPC interface. @@ -200,7 +225,7 @@ def __init_subclass__(cls) -> None: raise RpcServiceSetupException( cls.key, None, "`local_mode` class attribute (SiloMode) is required" ) - cls._signatures = cls._create_signatures() + cls._signatures = {sig.method_name: sig for sig in cls._create_signatures()} @classmethod def _get_all_rpc_methods(cls) -> Iterator[Callable[..., Any]]: @@ -236,8 +261,7 @@ def get_local_implementation(cls) -> RpcService: raise NotImplementedError @classmethod - def _create_signatures(cls) -> Mapping[str, RpcMethodSignature]: - model_table = {} + def _create_signatures(cls) -> Iterable[RpcMethodSignature]: for base_method in cls._get_all_rpc_methods(): try: signature = RpcMethodSignature(cls, base_method) @@ -246,8 +270,7 @@ def _create_signatures(cls) -> Mapping[str, RpcMethodSignature]: cls.key, base_method.__name__, "Error on parameter model" ) from e else: - model_table[base_method.__name__] = signature - return model_table + yield signature @classmethod def _get_and_validate_local_implementation(cls) -> RpcService: @@ -350,6 +373,21 @@ def create_delegation(cls, use_test_client: bool | None = None) -> Self: return service # type: ignore[return-value] +def list_all_service_method_signatures() -> Iterable[RpcMethodSignature]: + """List signatures of all RPC methods in the global registry.""" + + from sentry.services import hybrid_cloud as hybrid_cloud_service_pkg + + # Forcibly import all service packages to ensure the global registry is fully populated + for _, name, _ in pkgutil.walk_packages( + hybrid_cloud_service_pkg.__path__, prefix=f"{hybrid_cloud_service_pkg.__name__}." + ): + __import__(name) + + for service_obj in _global_service_registry.values(): + yield from service_obj.get_all_signatures() + + class RpcResolutionException(Exception): """Indicate that an RPC service or method name could not be resolved.""" diff --git a/src/sentry/services/hybrid_cloud/sig.py b/src/sentry/services/hybrid_cloud/sig.py index 3d74f9d8ef756d..6f45741e93bca1 100644 --- a/src/sentry/services/hybrid_cloud/sig.py +++ b/src/sentry/services/hybrid_cloud/sig.py @@ -138,3 +138,13 @@ def deserialize_arguments(self, serial_arguments: ArgumentDict) -> pydantic.Base def deserialize_return_value(self, value: Any) -> Any: parsed = self._return_model.parse_obj({self._RETURN_MODEL_ATTR: value}) return getattr(parsed, self._RETURN_MODEL_ATTR) + + def get_schemas(self) -> tuple[type[pydantic.BaseModel], type[pydantic.BaseModel]]: + """Access the schema representations directly. + + This generally should be needed only for reflective operations such as + checking for cross-version compatibility. Routine operations on the parameter + and return values should be done through the "serialize" and "deserialize" + methods. + """ + return self._parameter_model, self._return_model diff --git a/tests/sentry/hybridcloud/test_sig.py b/tests/sentry/hybridcloud/test_sig.py new file mode 100644 index 00000000000000..516d4adcbd0169 --- /dev/null +++ b/tests/sentry/hybridcloud/test_sig.py @@ -0,0 +1,30 @@ +import pydantic + +from sentry.services.hybrid_cloud.sig import SerializableFunctionSignature +from sentry.testutils.cases import TestCase + + +class SerializableFunctionSignatureTest(TestCase): + def test_signature(self): + class AnObject(pydantic.BaseModel): + a: int + b: str + + def a_function(arg1: AnObject, arg2: AnObject) -> AnObject: + return AnObject(a=arg1.a + arg2.a, b=".".join((arg1.b, arg2.b))) + + sig = SerializableFunctionSignature(a_function) + arg_values = dict(arg1=AnObject(a=1, b="foo"), arg2=AnObject(a=2, b="bar")) + serialized_arguments = sig.serialize_arguments(arg_values) + assert serialized_arguments == {"arg1": {"a": 1, "b": "foo"}, "arg2": {"a": 2, "b": "bar"}} + + deserialized_arguments = sig.deserialize_arguments(serialized_arguments) + assert isinstance(deserialized_arguments, pydantic.BaseModel) + assert set(deserialized_arguments.__dict__.keys()) == {"arg1", "arg2"} + assert hasattr(deserialized_arguments, "arg1") + assert deserialized_arguments.arg1 == AnObject(a=1, b="foo") + assert hasattr(deserialized_arguments, "arg2") + assert deserialized_arguments.arg2 == AnObject(a=2, b="bar") + + deserialized_return_value = sig.deserialize_return_value(dict(a=3, b="qux")) + assert deserialized_return_value == AnObject(a=3, b="qux") From 320eca996fbc66f16722f09d787b887017262ce4 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Tue, 14 May 2024 14:20:35 -0400 Subject: [PATCH 2/2] Make imports for openapi_pydantic safe --- src/sentry/runner/commands/rpcschema.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/sentry/runner/commands/rpcschema.py b/src/sentry/runner/commands/rpcschema.py index e0e9044e4430c9..8ed54bb7a63bfb 100644 --- a/src/sentry/runner/commands/rpcschema.py +++ b/src/sentry/runner/commands/rpcschema.py @@ -6,8 +6,6 @@ import click from django.urls import reverse -from openapi_pydantic import OpenAPI -from openapi_pydantic.util import PydanticSchema, construct_open_api_with_schema_class from sentry.runner.decorators import configuration from sentry.utils import json @@ -28,6 +26,16 @@ ) @configuration def rpcschema(diagnose: bool, partial: bool) -> None: + # Defered imports because openapi_pydantic is only installed as a dev dependency + try: + from openapi_pydantic import OpenAPI + from openapi_pydantic.util import PydanticSchema, construct_open_api_with_schema_class + except ImportError: + click.echo( + "Could not import openapi_pydantic. You must install requirements-dev to run this command" + ) + return + from sentry.services.hybrid_cloud.rpc import ( RpcMethodSignature, list_all_service_method_signatures, @@ -108,9 +116,9 @@ def create_partial_spec( if diagnose or partial: spec, error_reports = create_partial_spec(all_signatures) if diagnose: - print(f"Error count: {len(error_reports)}") # noqa + click.echo(f"Error count: {len(error_reports)}") # noqa for bad_sig in error_reports: - print("- " + bad_sig) # noqa + click.echo("- " + bad_sig) # noqa else: spec = create_spec(all_signatures)