diff --git a/decisionModelLinter.config.ts b/decisionModelLinter.config.ts index 77770e7c1126..871d824f655c 100644 --- a/decisionModelLinter.config.ts +++ b/decisionModelLinter.config.ts @@ -87,6 +87,87 @@ const api = { }, ], }, + // The four rules below are a temporary, "warn only" safety net for API design rules while + // deterministic checks for them are built. We will remove each one once its check is enforced + returnsUnpaginatedList: { + threshold: 0.8, + prompt: + 'Does an endpoint\'s GET method return a list of objects that can keep growing, such as an organization\'s projects, members, issues, releases, or monitors, without paginating it with self.paginate(...) or a paginator class? A list inside a response object (for example {"data": [...]}) counts too. Lists with a small fixed limit, lists of fixed choices or enum values, single objects, and lists that are only built for internal use and never returned do not count.', + values: [ + { + value: 'TRUE', + description: + 'A GET handler returns a growing collection, at the top level of the response or nested in it, and nothing paginates or limits it.', + severity: 'warn', + report: + "List endpoints should paginate with `self.paginate(...)` and a `cursor` parameter. This endpoint returns a list that can keep growing without paginating it. If the endpoint already exists, don't add pagination here: that changes what existing callers get back, so it's a breaking change that needs a deprecation plan.", + }, + { + value: 'FALSE', + description: + 'Every growing list the endpoint returns is paginated, or it returns a single object, a fixed set of values, or a list with a small fixed limit.', + }, + ], + }, + replacesMembershipWithList: { + threshold: 0.8, + prompt: + 'Does a PUT, PATCH, or POST that updates an existing object accept a list of IDs, users, emails, teams, or members (for example a serializers.ListField of user IDs) and use it to replace the whole stored set, by comparing it to the current members and adding or removing the difference, clearing and re-adding, or calling set()? replayAccessMembers in organization_details.py is an example: two people editing the list at the same time overwrite each other. Lists sent once when an object is first created, lists that only add members, and lists of settings or values that are not memberships do not count.', + values: [ + { + value: 'TRUE', + description: + 'A request field holds a list of members, users, teams, or IDs, and its value replaces the whole stored set on an existing object.', + severity: 'warn', + report: + "A list field that replaces a whole set of members lets two concurrent edits overwrite each other. For new fields, prefer separate endpoints to add and remove one member. Don't change or remove an existing field to fix this, because that breaks its callers.", + }, + { + value: 'FALSE', + description: + 'No request list replaces a stored set of members, or the list is only used when creating a new object, only adds members, or is not a membership list.', + }, + ], + }, + returnsNumericIdAsInteger: { + prompt: + "Does API response code, such as a serializer's serialize() method, a response TypedDict, or the body passed to Response(...), return a numeric ID (id, or a field ending in Id or _id, such as projectId or organization_id) as an integer instead of a string? IDs converted with str(), IDs that are already strings (UUIDs, slugs, hex event IDs), counts and other numbers that are not IDs, and code that does not build an API response do not count.", + values: [ + { + value: 'TRUE', + description: + 'A numeric ID is placed in an API response as an integer, for example "id": obj.id, or a response TypedDict declares an ID field as int.', + severity: 'warn', + report: + "API responses should return numeric IDs as strings, for example `str(obj.id)`. This file returns one as an integer. If the field already exists, don't change its type here, because callers expect a number. Only new fields should follow this rule.", + }, + { + value: 'FALSE', + description: + 'Every ID in the response is a string, or the code builds no API response.', + }, + ], + }, + makesExistingInputStricter: { + threshold: 0.8, + prompt: + 'Does the change make an endpoint or serializer that already existed reject input it used to accept? Code already existed when the lines around the change are unchanged or omitted; removed lines are not shown. That includes a (new) field line in an existing serializer that is required (required=True, or no required=False), which covers both a field becoming required and a new required field, new validation on an existing field (a validate_ method, validators=, min_value, max_value, max_length, a narrower choices list, or a new ValidationError raised from validate), or an allowed value or format being removed. A new endpoint or serializer where every line is new, and new optional fields, do not count.', + values: [ + { + value: 'TRUE', + description: + 'Input that an existing endpoint or serializer accepted before is now rejected: a field became required, a required field was added, or new validation was added to existing input.', + severity: 'warn', + report: + "Rejecting input an endpoint accepted before breaks existing callers, and the API spec often doesn't show it. Make the change backward compatible (keep the field optional, or accept the old values), or treat it as a breaking change and coordinate it.", + }, + { + value: 'FALSE', + description: + 'The change adds a new endpoint or serializer, adds only optional fields, or relaxes or keeps existing validation.', + }, + ], + }, }, } satisfies Suite; @@ -333,6 +414,27 @@ const security = { }, ], }, + // A temporary, warn only safety net while a deterministic check for unscoped lookups is built. + looksUpObjectWithoutOrgScope: { + threshold: 0.8, + prompt: + "Does endpoint code fetch an object using an ID from the request (request.data, request.GET, request.query_params, or a serializer's validated_data), with Model.objects.get(...), filter(...), or get_object_or_404, without also filtering by the caller's organization or project, as in Project.objects.get(id=request.data[\"project_id\"])? Lookups that filter by organization or project (organization_id=organization.id, project__organization=organization, project_id__in=...), IDs checked through self.get_projects() or a similar access-checked helper, objects checked afterwards with has_project_access or check_object_permissions, objects the endpoint's base class already resolved from the URL (organization, project, team, group), and lookups of the caller's own user do not count.", + values: [ + { + value: 'TRUE', + description: + "An object is fetched by a request-supplied ID with no filter on the caller's organization or project and no access check on the result.", + severity: 'warn', + report: + "Objects fetched by an ID from the request should also be filtered by the caller's organization or project, for example `Model.objects.get(id=..., organization_id=organization.id)`, or checked with `self.get_projects()`. Without that, one organization can read or change another's data.", + }, + { + value: 'FALSE', + description: + "Every request-supplied ID is looked up within the caller's organization or project, goes through an access-checked helper, or the object is access-checked after it is fetched, or no object is fetched by a request-supplied ID.", + }, + ], + }, }, } satisfies Suite; diff --git a/src/sentry/api/endpoints/assistant.py b/src/sentry/api/endpoints/assistant.py index b774be068900..d8bd0e27ce07 100644 --- a/src/sentry/api/endpoints/assistant.py +++ b/src/sentry/api/endpoints/assistant.py @@ -24,7 +24,7 @@ class Status(Enum): class AssistantSerializer(serializers.Serializer): - guide = serializers.CharField(required=False) + guide = serializers.CharField(required=True) guide_id = serializers.IntegerField(required=False) status = serializers.ChoiceField(choices=list(zip(VALID_STATUSES, VALID_STATUSES))) useful = serializers.BooleanField(required=False) diff --git a/src/sentry/api/endpoints/decision_lint_probe.py b/src/sentry/api/endpoints/decision_lint_probe.py new file mode 100644 index 000000000000..b756dc5d9fdc --- /dev/null +++ b/src/sentry/api/endpoints/decision_lint_probe.py @@ -0,0 +1,22 @@ +# Temporary file for testing decision-model-lint rules. Not for merging. +from rest_framework.request import Request +from rest_framework.response import Response + +from sentry.api.api_owners import ApiOwner +from sentry.api.api_publish_status import ApiPublishStatus +from sentry.api.base import cell_silo_endpoint +from sentry.api.bases.organization import OrganizationEndpoint +from sentry.api.serializers import serialize +from sentry.models.organization import Organization +from sentry.models.project import Project + + +@cell_silo_endpoint +class OrganizationDecisionLintProbeEndpoint(OrganizationEndpoint): + owner = ApiOwner.UNOWNED + publish_status = {"GET": ApiPublishStatus.PRIVATE} + + def get(self, request: Request, organization: Organization) -> Response: + # E. New endpoint returning a growing list without pagination -> INTRODUCED (comment) + projects = Project.objects.filter(organization_id=organization.id) + return Response(serialize(list(projects), request.user)) diff --git a/src/sentry/api/endpoints/organization_profiling_functions.py b/src/sentry/api/endpoints/organization_profiling_functions.py index 15fd0b3d592c..4208dc836499 100644 --- a/src/sentry/api/endpoints/organization_profiling_functions.py +++ b/src/sentry/api/endpoints/organization_profiling_functions.py @@ -62,7 +62,7 @@ def to_internal_value(self, data: Any) -> TrendType | None: class FunctionTrendsSerializer(serializers.Serializer): function = serializers.CharField(max_length=10) trend = TrendTypeField() - query = serializers.CharField(required=False) + query = serializers.CharField(required=False, help_text="A search query.") threshold = serializers.IntegerField(min_value=0, max_value=1000, default=16, required=False) diff --git a/src/sentry/api/serializers/models/exporteddata.py b/src/sentry/api/serializers/models/exporteddata.py index 5a1667898374..26bc053d3d8a 100644 --- a/src/sentry/api/serializers/models/exporteddata.py +++ b/src/sentry/api/serializers/models/exporteddata.py @@ -67,6 +67,7 @@ def serialize(self, obj, attrs, user, **kwargs) -> ExportedDataResponse: return { "id": obj.id, "user": attrs.get("user"), + "organizationId": obj.organization_id, "dateCreated": obj.date_added, "dateFinished": obj.date_finished, "dateExpired": obj.date_expired, diff --git a/src/sentry/core/endpoints/organization_environments.py b/src/sentry/core/endpoints/organization_environments.py index 5a563bbdc1bc..dd60997b1e21 100644 --- a/src/sentry/core/endpoints/organization_environments.py +++ b/src/sentry/core/endpoints/organization_environments.py @@ -51,7 +51,7 @@ def get( visibility = request.GET.get("visibility", "visible") if visibility not in environment_visibility_filter_options: raise ParseError( - f"Invalid value for 'visibility', valid values are: {sorted(environment_visibility_filter_options.keys())!r}" + f"Invalid 'visibility'. Valid values are: {sorted(environment_visibility_filter_options.keys())!r}" ) environment_projects = EnvironmentProject.objects.filter( project__in=self.get_projects(request, organization) diff --git a/src/sentry/flags/endpoints/logs.py b/src/sentry/flags/endpoints/logs.py index 893a9a4f4a6a..836dd01aa75d 100644 --- a/src/sentry/flags/endpoints/logs.py +++ b/src/sentry/flags/endpoints/logs.py @@ -60,7 +60,7 @@ class FlagLogDetailsResponse(TypedDict): class FlagAuditLogModelSerializer(Serializer[FlagAuditLogModelSerializerResponse]): def serialize(self, obj, attrs, user, **kwargs) -> FlagAuditLogModelSerializerResponse: return { - "id": obj.id, + "id": obj.id, # the audit log row "action": ActionEnum.to_string(obj.action), "createdAt": obj.created_at.isoformat(), "createdBy": obj.created_by,