Cleaning up the auth (user management) code #136

hiranya911 · 2018-04-21T00:23:23Z

Since we migrated the user management API to use the Google API client (identitytoolkit package), this code has become bit of a readability nightmare:

Unnecessary indirection: We store all user parameters in maps and then "apply" those values to identitytoolkit types (see UserToCreate and UserToUpdate types).
Too many small/redundant helper functions.
Heavy use of reflection.

This PR addresses the above issues by:

Directly applying user parameters to the corresponding identitytoolkit types. Intermediate maps are no longer used.
Removing most of the redundant helpers, and performing validations explicitly where needed.
Removing all usages of reflection (these were mostly present in the helper functions).

bklimt · 2018-04-23T17:28:26Z

auth/user_mgt.go

+}
+
+func (u *UserToCreate) validatedRequest() (*identitytoolkit.IdentitytoolkitRelyingpartySignupNewUserRequest, error) {
+	req := u.request() // it is allowed to create a user without any parameters


Tiny nit:
// creating a user without any parameters is allowed

Otherwise, it's ambiguous what "it" means.

bklimt · 2018-04-23T17:34:25Z

auth/user_mgt.go

@@ -107,83 +97,236 @@ type UserIterator struct {

 // UserToCreate is the parameter struct for the CreateUser function.
 type UserToCreate struct {
-	params map[string]interface{}
+	_req   *identitytoolkit.IdentitytoolkitRelyingpartySignupNewUserRequest


I'm not familiar with prefixing names with underscore in go. Isn't that a python convention?

I wanted to somehow indicate in the code that this field should not be accessed directly. It should always be accessed via request() or validatedRequest() functions. This also works well with IDE assist, since typing u.req auto completes to u.request(). WDYT?

bklimt · 2018-04-23T17:37:52Z

auth/user_mgt.go

@@ -107,83 +97,236 @@ type UserIterator struct {

 // UserToCreate is the parameter struct for the CreateUser function.
 type UserToCreate struct {
-	params map[string]interface{}
+	_req   *identitytoolkit.IdentitytoolkitRelyingpartySignupNewUserRequest
+	fields struct {


Is there a reason these are in a custom struct instead of just at the top level of the UserToCreate struct? If so, maybe a comment here explaining why would be helpful.

I suppose the level of indirection can be removed without taking a hit on the readability. Removed.

bklimt · 2018-04-23T17:40:37Z

auth/user_mgt.go

 		return fmt.Errorf("photo url must be a non-empty string")
 	}
 	return nil
 }

-func validateEmail(val interface{}) error {
-	email := val.(string)
+func validateEmail(email string) error {
 	if email == "" {


It's just slightly strange that you use len(s) == 0 above but use s == "" here. I'm not sure which one would be preferred, but it would be nice to be consistent. Also, the actual error string is worded slightly differently.

Changed to == ""

It's just slightly odd that "photo url must be a non-empty string", but "email must not be empty". Minor nit, though.

hiranya911 · 2018-04-23T18:18:51Z

Made the suggested changes, and left a follow up question for discussion.

hiranya911 · 2018-04-23T19:00:25Z

Thanks for the comments. Made the suggested changes as per comments stated here, and offline chat.

* context signatures * VerifyIDToken context * custom token context * fix context arguments * custom token with claims context * remove TODO * integration_iid_name * propagate ctx * propagate to private functions * snippets and tests ctx * integration test user2 fix * error message * Revert "integration test user2 fix" This reverts commit 57148ce. * add context to AE, fix integration test after merge. * Revert "error message" This reverts commit db7c34a. * Merged with latest dev * Using the 1.6 import for context * Dropping call to WithContext * Cleaning up the auth package (#134) * Cleaning up the auth package * Variable grouping * Removing some punctuation; Changed id to ID in error messages * Adding a minor comment * Cleaning up the auth (user management) code (#136) * Cleaning up the auth (user management) code * Added a few more tests * Removed the fields inner type from UserToCreate and UserToUpdate * Renamed _req variables; More consistent error messages

* Breaking context change (#88) * context signatures * VerifyIDToken context * custom token context * fix context arguments * custom token with claims context * remove TODO * integration_iid_name * propagate ctx * propagate to private functions * snippets and tests ctx * integration test user2 fix * error message * Revert "integration test user2 fix" This reverts commit 57148ce. * add context to AE, fix integration test after merge. * Revert "error message" This reverts commit db7c34a. * Merged with latest dev * Using the 1.6 import for context * Dropping call to WithContext * Cleaning up the auth package (#134) * Cleaning up the auth package * Variable grouping * Removing some punctuation; Changed id to ID in error messages * Adding a minor comment * Cleaning up the auth (user management) code (#136) * Cleaning up the auth (user management) code * Added a few more tests * Removed the fields inner type from UserToCreate and UserToUpdate * Renamed _req variables; More consistent error messages * Removing golint checks from 1.6.x build (#141) * Removing golint checks from 1.6.x build * Minor reformat * Bumped version to 3.0.0 (#142) * Bumped version to 3.0.0 * Updated changelog

* context signatures * VerifyIDToken context * custom token context * fix context arguments * custom token with claims context * remove TODO * integration_iid_name * propagate ctx * propagate to private functions * snippets and tests ctx * integration test user2 fix * error message * Revert "integration test user2 fix" This reverts commit 57148ce. * add context to AE, fix integration test after merge. * Revert "error message" This reverts commit db7c34a. * Merged with latest dev * Using the 1.6 import for context * Dropping call to WithContext * Cleaning up the auth package (#134) * Cleaning up the auth package * Variable grouping * Removing some punctuation; Changed id to ID in error messages * Adding a minor comment * Cleaning up the auth (user management) code (#136) * Cleaning up the auth (user management) code * Added a few more tests * Removed the fields inner type from UserToCreate and UserToUpdate * Renamed _req variables; More consistent error messages

Cleaning up the auth (user management) code

704d6ad

hiranya911 requested a review from bklimt April 21, 2018 00:23

hiranya911 assigned bklimt Apr 21, 2018

Added a few more tests

1160bed

bklimt requested changes Apr 23, 2018

View reviewed changes

hiranya911 assigned hiranya911 and unassigned bklimt Apr 23, 2018

Removed the fields inner type from UserToCreate and UserToUpdate

5cb16b1

hiranya911 assigned bklimt and unassigned hiranya911 Apr 23, 2018

Renamed _req variables; More consistent error messages

2c7deee

bklimt approved these changes Apr 23, 2018

View reviewed changes

hiranya911 assigned hiranya911 and unassigned bklimt Apr 23, 2018

hiranya911 merged commit 8194437 into breaking_context_change Apr 23, 2018

hiranya911 deleted the hkj-auth-cleanup branch April 23, 2018 20:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Cleaning up the auth (user management) code #136

Cleaning up the auth (user management) code #136

Uh oh!

hiranya911 commented Apr 21, 2018

Uh oh!

bklimt Apr 23, 2018

Uh oh!

hiranya911 Apr 23, 2018

Uh oh!

bklimt Apr 23, 2018

Uh oh!

hiranya911 Apr 23, 2018

Uh oh!

bklimt Apr 23, 2018

Uh oh!

hiranya911 Apr 23, 2018

Uh oh!

bklimt Apr 23, 2018

Uh oh!

hiranya911 Apr 23, 2018

Uh oh!

bklimt Apr 23, 2018

Uh oh!

hiranya911 Apr 23, 2018

Uh oh!

hiranya911 commented Apr 23, 2018

Uh oh!

hiranya911 commented Apr 23, 2018

Uh oh!

Uh oh!

Cleaning up the auth (user management) code #136

Cleaning up the auth (user management) code #136

Uh oh!

Conversation

hiranya911 commented Apr 21, 2018

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!

hiranya911 commented Apr 23, 2018

Uh oh!

hiranya911 commented Apr 23, 2018

Uh oh!

Uh oh!