ref(users): move user & user_identity_config & auth serializers to new abode #76896

Christinarlong · 2024-09-03T22:24:42Z

Moves

User
User_Identity_Config
Authenticator
and their tests to the user's directory :0) Fixes any additional typing along the journey (mainly serializer response dicts) ~

split of (#76822)
ref(issue # here)

codecov · 2024-09-03T22:30:22Z

Codecov Report

Attention: Patch coverage is 98.93617% with 3 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/sentry/users/api/serializers/user.py	98.17%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #76896       +/-   ##
===========================================
+ Coverage   64.76%   78.25%   +13.49%     
===========================================
  Files        6894     6900        +6     
  Lines      306469   306875      +406     
  Branches    50250    50268       +18     
===========================================
+ Hits       198493   240154    +41661     
+ Misses     101387    60292    -41095     
+ Partials     6589     6429      -160

Christinarlong · 2024-09-03T22:38:24Z

src/sentry/api/serializers/models/user.py

Replaced file with import shim for any getsentry refs

Christinarlong · 2024-09-04T19:02:18Z

src/sentry/users/api/serializers/authenticator.py

 from sentry.api.serializers import Serializer, register
 from sentry.auth.authenticators.base import AuthenticatorInterface
 from sentry.auth.authenticators.recovery_code import RecoveryCodeInterface
 from sentry.auth.authenticators.sms import SmsInterface
 from sentry.auth.authenticators.totp import TotpInterface
 from sentry.auth.authenticators.u2f import U2fInterface

+if TYPE_CHECKING:
+    from django.utils.functional import _StrOrPromise


From: typeddjango/django-stubs#1804
is it ok if we potentially return/serialize a promise?

is it ok if we potentially return/serialize a promise?

If we can we should avoid importing symbols with _ prefixes. We could str() cast these translation related containers to simplify the typed dict definitions.

this is actually fine -- _StrOrPromise is ~kinda public and typing-only

Christinarlong · 2024-09-04T19:06:17Z

src/sentry/users/api/serializers/user.py

+
+
+def manytoone_to_dict(
+    queryset: QuerySet[T], key: str, filter_func: Callable[[Any], bool] | None = None


typed and made this generic-ish

Christinarlong · 2024-09-04T19:07:48Z

src/sentry/users/api/serializers/user.py

+    dateJoined: datetime
+    lastLogin: datetime | None
+    has2fa: bool
+    lastActive: datetime | None


The model says that lastLogin and lastActive are nullable fields but the openAPI spec says these are required fields, not sure if there's a source of truth/who to follow for this response

We could have casting/fallback in the serializer that removes the None values, or the api schema could be wrong.

I definitely interpreted the OpenApi failing job wrong, as I didn't realize the OpenApi spec is generated from the model serializer which I guess didn't exist until now (?????)

That job fails anytime there is a delta which isn't great. I took a look at the output and because we've added more accurate types schema generation has better metadata to use and the schema has changed. The properties were always nullable, but our schema didn't reflect that because of limited information.

Christinarlong · 2024-09-04T19:10:09Z

src/sentry/users/api/serializers/user.py

+            ]
+
+        return d
+


I got rid of the DetailedUserSerializerResponse here as it wasn't really adding anything new

markstory · 2024-09-05T17:54:37Z

src/sentry/api/serializers/models/notification_action.py

 from sentry.models.notificationaction import (
    ActionService,
    ActionTarget,
    ActionTrigger,
    NotificationAction,
    NotificationActionProject,
 )
+from sentry.users.api.serializers.user import manytoone_to_dict


Not entirely related to these changes, but this module seems like a strange place for a very generic query utility. We can move it separately later.

markstory · 2024-09-05T17:55:44Z

src/sentry/users/api/serializers/__init__.py

@@ -0,0 +1,3 @@
+from .authenticator import *  # noqa: F401,F403
+from .user import *  # noqa: F401,F403
+from .user_identity_config import *  # noqa: F401,F403


Do we need this barrel file? It seems like we could avoid it right now.

markstory · 2024-09-05T17:57:17Z

src/sentry/users/api/serializers/authenticator.py

 from sentry.api.serializers import Serializer, register
 from sentry.auth.authenticators.base import AuthenticatorInterface
 from sentry.auth.authenticators.recovery_code import RecoveryCodeInterface
 from sentry.auth.authenticators.sms import SmsInterface
 from sentry.auth.authenticators.totp import TotpInterface
 from sentry.auth.authenticators.u2f import U2fInterface

+if TYPE_CHECKING:
+    from django.utils.functional import _StrOrPromise


is it ok if we potentially return/serialize a promise?

If we can we should avoid importing symbols with _ prefixes. We could str() cast these translation related containers to simplify the typed dict definitions.

markstory · 2024-09-05T17:59:49Z

src/sentry/users/api/serializers/user.py

+    dateJoined: datetime
+    lastLogin: datetime | None
+    has2fa: bool
+    lastActive: datetime | None


We could have casting/fallback in the serializer that removes the None values, or the api schema could be wrong.

Christinarlong · 2024-09-06T19:01:47Z

src/sentry/users/api/serializers/authenticator.py

+def get_interface_serializer(interface: AuthenticatorInterface) -> AuthenticatorInterfaceSerializer:
+    if isinstance(interface, SmsInterface):
+        return SmsInterfaceSerializer()
+    return AuthenticatorInterfaceSerializer()


In the endpoints that are given an ApplicationInterface, interface, to serialize, this 'getter' is used to make sure we're using the right serializer. It's a weird scenario because registering the serializer in line 76 and 94 should solve this for us, but because we also have that bug of the file not being imported and therefore the serializers not being registered, this redundant function is here.....

Maybe other alternatives could be having barrel file for just the authenticator ?

I don't hate the factory function. In the longer term, I think we should consider removing the @register() calls as they contribute to import tangle and import side-effects create hidden dependencies between modules.

It's a weird scenario because registering the serializer in line 76 and 94 should solve this for us, but because we also have that bug of the file not being imported and therefore the serializers not being registered, this redundant function is here.....

If the endpoint imports the sentry.users.api.serializers.authenticators module I would expect that the register() calls would behave correctly. Is that not the case?

yeah that's correct, but if we import the authenticator serializer and don't do anything with it, we'll get shaboinked by mypy & linter. OTOH we can't just pass in AuthenticatorInterface for everything because of SmsInterface :/

markstory

Looks good to me.

markstory · 2024-09-06T21:41:04Z

src/sentry/users/api/serializers/authenticator.py

+class EnrolledAuthenticatorInterfaceSerializerResponse(TypedDict, total=False):
+    authId: str
+    createdAt: datetime
+    lastUsedAt: datetime | None


Another way to solve this in future changes would be to use typing.NotRequired to mark these keys as optional.

actually that's a good point -- is this actually optional and nullable? (rarely something is)

@asottile-sentry like are the "enrolled" fields optional & nullable ? I believe so, because they are some test cases like tests/sentry/users/api/endpoints/test_user_authenticator_enroll.py where the enrollment in an authenticator interface failed, so the returned user doesn't have any of these fields. Or were you talking about a different field (?) ...

as written now there's these possibilities:

{} {"lastUsedAt": some_datetime} {"lastUsedAt": None}

which is usually a bug

yeah I dunno, from what I'm seeing from our tests
{} -> is a real scenario [it seems like this can happen when enrollment fails], so Optional seems to be valid
{"lastUsedAt": some_datetime} -> also is a real scenario when we mark an authenticator as used.
{"lastUsedAt": None} -> also is real when a user is newly enrolled to an authenticator

It looks like the three scenarios all happen so probably not a bug, albeit also somewhat confusing to deal with

markstory · 2024-09-06T21:49:13Z

src/sentry/users/api/serializers/authenticator.py

+def get_interface_serializer(interface: AuthenticatorInterface) -> AuthenticatorInterfaceSerializer:
+    if isinstance(interface, SmsInterface):
+        return SmsInterfaceSerializer()
+    return AuthenticatorInterfaceSerializer()


I don't hate the factory function. In the longer term, I think we should consider removing the @register() calls as they contribute to import tangle and import side-effects create hidden dependencies between modules.

It's a weird scenario because registering the serializer in line 76 and 94 should solve this for us, but because we also have that bug of the file not being imported and therefore the serializers not being registered, this redundant function is here.....

If the endpoint imports the sentry.users.api.serializers.authenticators module I would expect that the register() calls would behave correctly. Is that not the case?

markstory · 2024-09-06T21:50:54Z

src/sentry/users/api/serializers/user.py

+    # OrganizationMemberResponse).
+    identities: list[_Identity]
+    avatar: SerializedAvatarFields
+    authenticators: list[Any]  # TODO: find out what type this is


Interesting that we have a special serialization format for authenticators here that is different from the representation that authenticator endpoints use. 🤔

yeahh..... the user_authenticator_enroll.py endpoint also has it's own serializer /format for each of the interfaces that's different. AuthenticatorInterface serialization in general is like p. inconsistent ;-;

markstory · 2024-09-06T21:56:16Z

src/sentry/utils/serializers.py

+T = TypeVar("T", bound=Model)
+
+
+def manytoone_to_dict(


Christinarlong added 6 commits September 3, 2024 14:59

move user and user_config serializers and tests

a464cb5

apply typing to user and user_config

e1099cf

fix paths

2b5af01

fix typing for serializesr

6a6838b

update paths in test patches

2a63d9f

actually import the serializer lol

ec64d4f

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Sep 3, 2024

vercel bot deployed to Preview September 3, 2024 22:26 View deployment

🔪 regenerate mypy module blocklist

91610e1

vercel bot deployed to Preview September 3, 2024 22:35 View deployment

Christinarlong commented Sep 3, 2024

View reviewed changes

src/sentry/api/serializers/models/user.py Outdated

Copy link

Contributor Author

Christinarlong Sep 3, 2024

Choose a reason for hiding this comment

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

Replaced file with import shim for any getsentry refs

Christinarlong added 2 commits September 3, 2024 17:39

add authenticator

ae5c7eb

added more init.pys for pytest to find

2d6cb77

vercel bot deployed to Preview September 4, 2024 00:57 View deployment

add typing for authenticator

6b49f50

vercel bot deployed to Preview September 4, 2024 01:35 View deployment

Christinarlong added 3 commits September 4, 2024 11:54

revert userip changes

fd588bb

revert userrole changes

641f46c

revert useremail changes

5450c40

vercel bot deployed to Preview September 4, 2024 19:02 View deployment

🔪 regenerate mypy module blocklist

387a467

vercel bot deployed to Preview September 4, 2024 19:08 View deployment

Christinarlong commented Sep 4, 2024

View reviewed changes

Christinarlong requested review from GabeVillalobos and a team September 4, 2024 21:48

Christinarlong changed the title ~~ref(users): move user & user_identity_config serializers to new abode~~ ref(users): move user & user_identity_config & auth serializers to new abode Sep 4, 2024

Christinarlong mentioned this pull request Sep 4, 2024

ref(users): move other users related serializers to new abode #76972

Merged

Merge branch 'master' into christinarlong/move-user-serializers-user

e503e6a

vercel bot deployed to Preview September 4, 2024 23:35 View deployment

actually export the user shim oop

8ad2c1d

vercel bot deployed to Preview September 5, 2024 00:33 View deployment

markstory reviewed Sep 5, 2024

View reviewed changes

Christinarlong added 6 commits September 5, 2024 14:21

remove barrel file

c719035

explicitly use authenticatorinterface serializers

0daef0b

fix typing for interface serializers

3a1fd68

use user serializer explicitly

2926799

explicitly pass in uic serializer

70ea0c2

bring out manytoone to utils

9f883fd

vercel bot deployed to Preview September 6, 2024 18:16 View deployment

Christinarlong commented Sep 6, 2024

View reviewed changes

Christinarlong requested a review from markstory September 6, 2024 19:32

Christinarlong marked this pull request as ready for review September 6, 2024 19:32

Christinarlong requested review from a team as code owners September 6, 2024 19:32

remove unneeded inits

32b6303

markstory approved these changes Sep 6, 2024

View reviewed changes

also bring back str or primseu

91a255b

vercel bot deployed to Preview September 9, 2024 18:23 View deployment

jk those inits were needed

4b2f36a

vercel bot deployed to Preview September 9, 2024 19:22 View deployment

Christinarlong merged commit 1c2e8a6 into master Sep 10, 2024
49 of 50 checks passed

Christinarlong deleted the christinarlong/move-user-serializers-user branch September 10, 2024 17:07

github-actions bot locked and limited conversation to collaborators Sep 26, 2024



		def manytoone_to_dict(
		queryset: QuerySet[T], key: str, filter_func: Callable[[Any], bool] \| None = None

Uh oh!

ref(users): move user & user_identity_config & auth serializers to new abode #76896

ref(users): move user & user_identity_config & auth serializers to new abode #76896

Uh oh!

Conversation

Christinarlong commented Sep 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Sep 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Christinarlong Sep 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markstory left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Christinarlong Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Christinarlong Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Christinarlong commented Sep 3, 2024 •

edited

Loading

codecov bot commented Sep 3, 2024 •

edited

Loading

Christinarlong Sep 4, 2024 •

edited

Loading

Christinarlong Sep 9, 2024 •

edited

Loading

Christinarlong Sep 9, 2024 •

edited

Loading