chore(users): migrate proto backend to native #1805

abcxff · 2025-01-07T15:45:44Z

Changes

cloudflare-workers-and-pages · 2025-01-07T15:45:46Z

Deploying rivet with Cloudflare Pages

Latest commit:	`6b956b2`
Status:	✅ Deploy successful!
Preview URL:	https://fc1263c2.rivet.pages.dev
Branch Preview URL:	https://01-07-chore-users-migrate-pr.rivet.pages.dev

View logs

abcxff · 2025-01-07T15:46:01Z

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

_{An organization admin has enabled the Graphite Merge Queue in this repository.} _{Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.}

This stack of pull requests is managed by Graphite. Learn more about stacking.

greptile-apps

PR Summary

This PR migrates user identity handling from proto backend types to native Rust types across the codebase. Here's a summary of the key changes:

Added new native types in /packages/services/user/src/types/identity.rs and mod.rs to replace proto backend user types
Moved URL generation functions user_profile and user_avatar from util/core/src/route.rs to services/user/src/route.rs
Updated identity creation and validation logic in user/src/ops/identity/create.rs to use native types instead of proto types
Simplified UUID comparisons by removing unnecessary proto type conversions in multiple files
Added new workspace dependencies util and types-proto in user/Cargo.toml to support the migration

Key points to review:

Verify URL construction safety in the new user/src/route.rs functions
Check error handling consistency with :: scope resolution operator in profile_validate.rs
Ensure proper type safety in UUID comparisons after removing proto type conversions
Review test coverage for the new native type implementations
Consider documenting the migration pattern for future similar changes

_{26 file(s) reviewed, 13 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

packages/api/group/src/convert/identity.rs

packages/services/user/src/ops/identity/create.rs

packages/services/user/src/ops/profile_validate.rs

packages/services/user/src/types/mod.rs

packages/services/user/src/route.rs

packages/services/user/tests/identity_get.rs

packages/services/user/src/ops/profile_validate.rs

abcxff · 2025-02-14T22:36:56Z

@greptileai

greptile-apps

PR Summary

Based on the provided files and previous review, I'll focus on the most recent changes in this PR that weren't covered in the previous summary.

This PR continues the migration of user identity handling from proto backend to native Rust types, with several important updates:

Migrated context handling in API routes from ctx.op_ctx() to &*ctx for more idiomatic Rust usage
Updated ApiCtx usage in fetch functions across convert/src/fetch/ modules to align with native implementation
Simplified user ID handling by removing unnecessary unwrapping and proto type conversions
Streamlined operation calls by using ctx.op directly instead of chirp_workflow::compat::op

Key points to review:

Ensure consistent error propagation with the new context dereferencing pattern
Verify that all ApiCtx conversions maintain proper transaction boundaries
Check for potential memory leaks in the new context handling approach
Consider adding documentation for the new context usage patterns

The changes maintain the same functionality while making the code more idiomatic and reducing complexity in the type system.

_{31 file(s) reviewed, 11 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2025-02-14T22:38:00Z

packages/api/cloud/src/route/games/mod.rs

@@ -29,9 +29,9 @@ pub async fn list(
 ) -> GlobalResult<models::CloudGamesGetGamesResponse> {
 	let accessible_games = ctx.auth().accessible_games(ctx.op_ctx()).await?;


style: Consider updating ctx.auth().accessible_games(ctx.op_ctx()) to use the new context style for consistency

greptile-apps · 2025-02-14T22:38:46Z

packages/api/group/src/route/groups.rs

 			let ban_ts = unwrap!(team_bans
 				.banned_users
 				.iter()
-				.find(|ban| user.user_id == ban.user_id)
+				.find(|ban| Some(user.user_id.into()) == ban.user_id)
 			).ban_ts;


style: consider using unwrap_with! here to provide a more descriptive error message if the ban is not found

greptile-apps · 2025-02-14T22:38:47Z

packages/api/identity/src/route/identities.rs

@@ -53,7 +53,7 @@ async fn get_profile(
 	_watch_index: WatchIndexQuery,
 ) -> GlobalResult<(models::IdentityProfile, i64)> {
 	let identities =
-		fetch::identity::profiles(ctx.op_ctx(), current_user_id, vec![identity_id]).await?;
+		fetch::identity::profiles(&*ctx, current_user_id, vec![identity_id]).await?;


style: dereferencing ctx with &* could potentially cause issues if the Deref implementation for Ctx<Auth> changes in the future. Consider using an explicit conversion method instead.

greptile-apps · 2025-02-14T22:38:48Z

packages/api/identity/src/route/identities.rs

@@ -87,7 +87,7 @@ pub async fn handles(
 	);

 	let identities =
-		fetch::identity::handles(ctx.op_ctx(), current_user_id, query.identity_ids).await?;
+		fetch::identity::handles(&*ctx, current_user_id, query.identity_ids).await?;


style: same dereferencing pattern used here - should be consistent with how context is handled across the codebase

greptile-apps · 2025-02-14T22:38:48Z

packages/api/identity/src/route/identities.rs

@@ -120,7 +120,7 @@ pub async fn summaries(
 	// Wait for an update if needed

 	let identities =
-		fetch::identity::summaries(ctx.op_ctx(), current_user_id, query.identity_ids).await?;
+		fetch::identity::summaries(&*ctx, current_user_id, query.identity_ids).await?;


logic: consider adding error handling for the case where identities.into_iter().next() returns None, rather than just using unwrap_with

greptile-apps · 2025-02-14T22:39:29Z

packages/common/convert/src/fetch/game.rs

 	game_ids: Vec<common::Uuid>,
 ) -> GlobalResult<(game::get::Response, HashMap<Uuid, backend::team::Team>)> {


logic: The function accepts Vec<common::Uuid> but returns HashMap<Uuid, ...> - ensure these UUID types are compatible across the codebase

greptile-apps · 2025-02-14T22:39:37Z

packages/common/convert/src/fetch/identity.rs

 	let (users, teams_ctx, linked_accounts) = tokio::try_join!(
 		users(ctx, raw_user_ids.clone()),
-		teams(ctx, user_ids.clone()),
-		linked_accounts(ctx, user_ids.clone()),
+		teams(ctx, raw_user_ids.clone()),
+		linked_accounts(ctx, raw_user_ids.clone()),
 	)?;


style: Cloning raw_user_ids three times could be inefficient. Consider cloning once before the try_join and reusing the cloned vector.

greptile-apps · 2025-02-14T22:42:09Z

packages/services/user/src/types/identity.rs

+pub struct Email {
+    pub email: String,
+}


style: Consider adding validation for email format and length. Email strings should be validated before storage.

greptile-apps · 2025-02-14T22:42:10Z

packages/services/user/src/types/identity.rs

+impl From<&Identity> for backend::user_identity::Identity {
+    fn from(identity: &Identity) -> Self {
+        backend::user_identity::Identity {
+            kind: Some(match &identity.kind {
+                Kind::Email(kind) => {
+                    backend::user_identity::identity::Kind::Email(
+                        backend::user_identity::identity::Email {
+                            email: kind.email.clone()
+                        }
+                    )
+                }
+                Kind::DefaultUser(_) => {
+                    backend::user_identity::identity::Kind::DefaultUser(
+                        backend::user_identity::identity::DefaultUser {}
+                    )
+                }
+            })
+        }
+    }
+}


style: Consider implementing TryFrom instead of From to handle potential validation errors during conversion.

greptile-apps · 2025-02-14T22:42:11Z

packages/services/user/src/types/identity.rs

+            kind: Some(match &identity.kind {
+                Kind::Email(kind) => {
+                    backend::user_identity::identity::Kind::Email(
+                        backend::user_identity::identity::Email {
+                            email: kind.email.clone()
+                        }


style: Consider adding error handling for the case where email is empty or invalid during conversion.

abcxff mentioned this pull request Jan 7, 2025

chore(services/user): migrate user-team-list proto op #1650

Open

abcxff force-pushed the 12-18-chore_services_user_migrate_user_proto_ops branch from ec88581 to d8bc66a Compare January 14, 2025 22:47

abcxff force-pushed the 01-07-chore_users_migrate_proto_backend_to_native branch from 29b15a5 to 73890c0 Compare January 14, 2025 22:47

abcxff mentioned this pull request Jan 14, 2025

chore(users/workflow): setup basic e2e user workflow #1844

Open

abcxff force-pushed the 12-18-chore_services_user_migrate_user_proto_ops branch from d8bc66a to 6fb3d05 Compare January 22, 2025 03:35

abcxff force-pushed the 01-07-chore_users_migrate_proto_backend_to_native branch from 73890c0 to 20d844f Compare January 22, 2025 03:35

This was referenced Jan 22, 2025

feat(user/workflows): profile set signal #1902

Open

feat(users): user wf tests #1935

Open

abcxff force-pushed the 12-18-chore_services_user_migrate_user_proto_ops branch from 6fb3d05 to c1c6586 Compare January 22, 2025 21:10

abcxff force-pushed the 01-07-chore_users_migrate_proto_backend_to_native branch from 20d844f to fd3c4de Compare January 22, 2025 21:11

abcxff force-pushed the 12-18-chore_services_user_migrate_user_proto_ops branch from c1c6586 to 2c153f0 Compare January 22, 2025 22:19

abcxff force-pushed the 01-07-chore_users_migrate_proto_backend_to_native branch from fd3c4de to e3a441c Compare January 22, 2025 22:19

abcxff force-pushed the 12-18-chore_services_user_migrate_user_proto_ops branch from 2c153f0 to b259e38 Compare January 28, 2025 18:49

abcxff force-pushed the 01-07-chore_users_migrate_proto_backend_to_native branch from e3a441c to e3632e7 Compare January 28, 2025 18:50

abcxff force-pushed the 12-18-chore_services_user_migrate_user_proto_ops branch from b259e38 to 9a2121e Compare January 28, 2025 19:50

abcxff force-pushed the 01-07-chore_users_migrate_proto_backend_to_native branch from e3632e7 to 7e75cf3 Compare January 28, 2025 19:50

abcxff mentioned this pull request Jan 28, 2025

feat(user): migrate pub/sub #1953

Open

abcxff force-pushed the 12-18-chore_services_user_migrate_user_proto_ops branch from 9a2121e to 2519b6f Compare January 28, 2025 19:59

abcxff force-pushed the 01-07-chore_users_migrate_proto_backend_to_native branch from 7e75cf3 to 4bfdbf2 Compare January 28, 2025 19:59

abcxff force-pushed the 12-18-chore_services_user_migrate_user_proto_ops branch from 2519b6f to be1bf74 Compare January 28, 2025 22:35

abcxff force-pushed the 01-07-chore_users_migrate_proto_backend_to_native branch from 4bfdbf2 to fe3ab7c Compare January 28, 2025 22:35

abcxff mentioned this pull request Jan 29, 2025

chore(users): delete old worker code #1959

Open

abcxff force-pushed the 12-18-chore_services_user_migrate_user_proto_ops branch from be1bf74 to 936d97d Compare February 6, 2025 21:51

abcxff force-pushed the 01-07-chore_users_migrate_proto_backend_to_native branch 2 times, most recently from 18a7d75 to 764ba21 Compare February 6, 2025 21:57

abcxff changed the base branch from 12-18-chore_services_user_migrate_user_proto_ops to 01-29-chore_users_delete_old_worker_code February 6, 2025 21:57

abcxff force-pushed the 01-29-chore_users_delete_old_worker_code branch from 8b00f39 to a082694 Compare February 10, 2025 18:53

abcxff force-pushed the 01-07-chore_users_migrate_proto_backend_to_native branch from 764ba21 to 31ed4b4 Compare February 10, 2025 18:54

abcxff marked this pull request as ready for review February 10, 2025 18:56

greptile-apps bot reviewed Feb 10, 2025

View reviewed changes

abcxff force-pushed the 01-29-chore_users_delete_old_worker_code branch from a082694 to 6de4e36 Compare February 10, 2025 23:08

abcxff force-pushed the 01-07-chore_users_migrate_proto_backend_to_native branch from 31ed4b4 to 1f6a1b5 Compare February 10, 2025 23:08

NathanFlurry requested changes Feb 11, 2025

View reviewed changes

packages/services/user/src/ops/profile_validate.rs Outdated Show resolved Hide resolved

chore(users): migrate proto backend to native

6b956b2

abcxff force-pushed the 01-29-chore_users_delete_old_worker_code branch from 6de4e36 to a69da5b Compare February 14, 2025 14:31

abcxff force-pushed the 01-07-chore_users_migrate_proto_backend_to_native branch from 1f6a1b5 to 6b956b2 Compare February 14, 2025 14:31

abcxff mentioned this pull request Feb 14, 2025

chore: move loops to oss #2031

Open

greptile-apps bot reviewed Feb 14, 2025

View reviewed changes

abcxff mentioned this pull request Feb 16, 2025

feat(user): user one-off for users w/o wf #2036

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(users): migrate proto backend to native #1805

chore(users): migrate proto backend to native #1805

abcxff commented Jan 7, 2025

cloudflare-workers-and-pages bot commented Jan 7, 2025 •

edited

Loading

abcxff commented Jan 7, 2025 •

edited

Loading

greptile-apps bot left a comment

abcxff commented Feb 14, 2025

greptile-apps bot left a comment

greptile-apps bot Feb 14, 2025

greptile-apps bot Feb 14, 2025

greptile-apps bot Feb 14, 2025

greptile-apps bot Feb 14, 2025

greptile-apps bot Feb 14, 2025

greptile-apps bot Feb 14, 2025

greptile-apps bot Feb 14, 2025

greptile-apps bot Feb 14, 2025

greptile-apps bot Feb 14, 2025

greptile-apps bot Feb 14, 2025

		@@ -29,9 +29,9 @@ pub async fn list(
		) -> GlobalResult<models::CloudGamesGetGamesResponse> {
		let accessible_games = ctx.auth().accessible_games(ctx.op_ctx()).await?;

		game_ids: Vec<common::Uuid>,
		) -> GlobalResult<(game::get::Response, HashMap<Uuid, backend::team::Team>)> {

chore(users): migrate proto backend to native #1805

Are you sure you want to change the base?

chore(users): migrate proto backend to native #1805

Conversation

abcxff commented Jan 7, 2025

Changes

cloudflare-workers-and-pages bot commented Jan 7, 2025 • edited Loading

Deploying rivet with Cloudflare Pages

abcxff commented Jan 7, 2025 • edited Loading

How to use the Graphite Merge Queue

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

abcxff commented Feb 14, 2025

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Feb 14, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 14, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 14, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 14, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 14, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 14, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 14, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 14, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 14, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 14, 2025

Choose a reason for hiding this comment

cloudflare-workers-and-pages bot commented Jan 7, 2025 •

edited

Loading

abcxff commented Jan 7, 2025 •

edited

Loading