-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
chore(hybrid-cloud): Break queries that depend on cross silo FKs #45095
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
22f3973
62d82ea
83753e0
786887e
1f6109f
cab3e82
8b99905
0bc0e16
e4b00c9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,25 +1,31 @@ | ||
| from sentry.api.serializers import Serializer, register, serialize | ||
| from sentry.api.serializers import Serializer, register | ||
| from sentry.data_export.base import ExportQueryType | ||
| from sentry.data_export.models import ExportedData | ||
| from sentry.models import User | ||
| from sentry.services.hybrid_cloud.user import user_service | ||
|
|
||
|
|
||
| @register(ExportedData) | ||
| class ExportedDataSerializer(Serializer): | ||
| def get_attrs(self, item_list, user, **kwargs): | ||
| attrs = {} | ||
| users = User.objects.filter(id__in={item.user_id for item in item_list}) | ||
| user_lookup = {user.id: user for user in users} | ||
| serialized_users = { | ||
| u["id"]: u | ||
| for u in user_service.serialize_many( | ||
| filter=dict(user_ids=[item.user_id for item in item_list]) | ||
| ) | ||
| } | ||
|
Comment on lines
+11
to
+16
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We seem to do this a bunch. Should the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In, general, I think the I'd still prefer to keep the service interfaces thing (one way of doing things, not 3), but in this case returning by dictionary is actually preferable to the list, so I may replace it that way.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll make some follow up work to consider refactoring all serialize_many call sites into this convention. |
||
| for item in item_list: | ||
| user = user_lookup[item.user_id] | ||
| serialized_user = serialize(user) | ||
| attrs[item] = { | ||
| "user": { | ||
| "id": serialized_user["id"], | ||
| "email": serialized_user["email"], | ||
| "username": serialized_user["username"], | ||
| if str(item.user_id) in serialized_users: | ||
| serialized_user = serialized_users[str(item.user_id)] | ||
| attrs[item] = { | ||
| "user": { | ||
| "id": serialized_user["id"], | ||
| "email": serialized_user["email"], | ||
| "username": serialized_user["username"], | ||
| } | ||
| } | ||
| } | ||
| else: | ||
| attrs[item] = {} | ||
| return attrs | ||
|
|
||
| def serialize(self, obj, attrs, user, **kwargs): | ||
|
|
@@ -33,7 +39,7 @@ def serialize(self, obj, attrs, user, **kwargs): | |
|
|
||
| return { | ||
| "id": obj.id, | ||
| "user": attrs["user"], | ||
| "user": attrs.get("user"), | ||
| "dateCreated": obj.date_added, | ||
| "dateFinished": obj.date_finished, | ||
| "dateExpired": obj.date_expired, | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -15,6 +15,7 @@ | |||||
| Optional, | ||||||
| Protocol, | ||||||
| Sequence, | ||||||
| Set, | ||||||
| Tuple, | ||||||
| TypedDict, | ||||||
| Union, | ||||||
|
|
@@ -35,7 +36,6 @@ | |||||
| from sentry.constants import LOG_LEVELS | ||||||
| from sentry.issues.grouptype import GroupCategory | ||||||
| from sentry.models import ( | ||||||
| ActorTuple, | ||||||
| Commit, | ||||||
| Environment, | ||||||
| Group, | ||||||
|
|
@@ -183,19 +183,32 @@ def __init__( | |||||
| self.collapse = collapse | ||||||
| self.expand = expand | ||||||
|
|
||||||
| def _serialize_assigness( | ||||||
| self, actor_dict: Mapping[int, ActorTuple] | ||||||
| ) -> Mapping[int, Union[Team, Any]]: | ||||||
| actors_by_type: MutableMapping[Any, List[ActorTuple]] = defaultdict(list) | ||||||
| for actor in actor_dict.values(): | ||||||
| actors_by_type[actor.type].append(actor) | ||||||
| def _serialize_assignees(self, item_list: Sequence[Group]) -> Mapping[int, Union[Team, Any]]: | ||||||
| gas = GroupAssignee.objects.filter(group__in=item_list) | ||||||
| result: MutableMapping[int, Union[Team, Any]] = {} | ||||||
| all_team_ids: MutableMapping[int, Set[int]] = {} | ||||||
| all_user_ids: MutableMapping[int, Set[int]] = {} | ||||||
|
Comment on lines
+189
to
+190
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: You could make these defaultdict() to save the conditions on 193 and 199 |
||||||
|
|
||||||
| for g in gas: | ||||||
| if g.team_id: | ||||||
| if g.team_id not in all_team_ids: | ||||||
| all_team_ids[g.team_id] = {g.group_id} | ||||||
| else: | ||||||
| all_team_ids[g.team_id].add(g.group_id) | ||||||
| if g.user_id: | ||||||
| if g.team_id not in all_team_ids: | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| all_user_ids[g.user_id] = {g.group_id} | ||||||
| else: | ||||||
| all_user_ids[g.user_id].add(g.group_id) | ||||||
|
|
||||||
| for team in Team.objects.filter(id__in=all_team_ids.keys()): | ||||||
| for group_id in all_team_ids[team.id]: | ||||||
| result[group_id] = team | ||||||
| for user in user_service.get_many(filter=dict(user_ids=list(all_user_ids.keys()))): | ||||||
| for group_id in all_user_ids[user.id]: | ||||||
| result[group_id] = user | ||||||
|
|
||||||
| resolved_actors = {} | ||||||
| for t, actors in actors_by_type.items(): | ||||||
| serializable = ActorTuple.resolve_many(actors) | ||||||
| resolved_actors[t] = {actor.id: actor for actor in serializable} | ||||||
|
|
||||||
| return {key: resolved_actors[value.type][value.id] for key, value in actor_dict.items()} | ||||||
| return result | ||||||
|
|
||||||
| def get_attrs( | ||||||
| self, item_list: Sequence[Group], user: Any, **kwargs: Any | ||||||
|
|
@@ -223,11 +236,7 @@ def get_attrs( | |||||
| seen_groups = {} | ||||||
| subscriptions = defaultdict(lambda: (False, False, None)) | ||||||
|
|
||||||
| assignees: Mapping[int, ActorTuple] = { | ||||||
| a.group_id: a.assigned_actor() | ||||||
| for a in GroupAssignee.objects.filter(group__in=item_list) | ||||||
| } | ||||||
| resolved_assignees = self._serialize_assigness(assignees) | ||||||
| resolved_assignees = self._serialize_assignees(item_list) | ||||||
|
|
||||||
| ignore_items = {g.group_id: g for g in GroupSnooze.objects.filter(group__in=item_list)} | ||||||
|
|
||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we get around to building RPC calls we should make sure we only send unique userids in the request body.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, ideally our service interfaces use "wide" types like
Sequenceso that I don't have to force a list. This is on my mind, too.