fix: isolation level and primary user reservation #288
Conversation
| GeneralQueries.linkAccounts_Transaction(this, sqlCon, appIdentifier, recipeUserId, primaryUserId); | ||
| AccountInfoQueries.reserveAccountInfoForLinking_Transaction(this, sqlCon, appIdentifier, recipeUserId, primaryUserId); |
There was a problem hiding this comment.
Is the idea here that we can just drop the GeneralQueries one sooner or later?
| { | ||
| io.supertokens.storage.postgresql.queries.Utils.takeAdvisoryLock( | ||
| sqlCon, tenantConfig.tenantIdentifier.getConnectionUriDomain() + "~" + tenantConfig.tenantIdentifier.getAppId() + "~" + tenantConfig.tenantIdentifier.getTenantId()); | ||
| } |
There was a problem hiding this comment.
Do we need this? What do we want to achieve with this here?
| // Insert row for third party id | ||
| AccountInfoQueries.addRecipeUserAccountInfo_Transaction(start, sqlCon, tenantIdentifier, id, | ||
| THIRD_PARTY.toString(), ACCOUNT_INFO_TYPE.THIRD_PARTY, "", "", | ||
| thirdParty.id + "::" + thirdParty.userId); |
There was a problem hiding this comment.
Do we validate the thirdParty.id cannot have "::"?
| + "account_info_value TEXT NOT NULL," | ||
| + "third_party_id VARCHAR(28)," | ||
| + "third_party_user_id VARCHAR(256)," |
There was a problem hiding this comment.
We could also add a check constraint about "valid combinations" (i.e., tp+email or email or phone), but I don't think we actually need that. Please think it through and document the decision.
| List<Object> parameters = new ArrayList<>(); | ||
|
|
||
| // Add app_id parameter | ||
| parameters.add(appIdentifier.getAppId()); |
There was a problem hiding this comment.
Please move these right below/above where we add the ? this will replace.
| Connection sqlCon = (Connection) con.getConnection(); | ||
|
|
||
| // Build the query dynamically based on which values are not null | ||
| StringBuilder QUERY = new StringBuilder("SELECT primary_user_id, account_info_type FROM " + getConfig(start).getPrimaryUserTenantsTable()); |
There was a problem hiding this comment.
Couldn't we do this similarly to how the addPrimaryUserAccountInfo_Transaction, using a subquery based on the recipe user id?
| } | ||
| } | ||
|
|
||
| public static void checkIfLoginMethodsCanBeLinked_Transaction(Start start, TransactionConnection con, AppIdentifier appIdentifier, Set<String> tenantIds, Set<String> emails, |
There was a problem hiding this comment.
Please see if it makes sense to use a subquery here instead of the collected emails
| if (thirdParties != null && !thirdParties.isEmpty()) { | ||
| List<String> thirdPartyValues = new ArrayList<>(); | ||
| for (LoginMethod.ThirdParty tp : thirdParties) { | ||
| thirdPartyValues.add(tp.id + "::" + tp.userId); |
There was a problem hiding this comment.
We should extract the concatenation into a single util function, just to ensure this stays consistent.
| StringBuilder QUERY = new StringBuilder("SELECT primary_user_id, account_info_type, account_info_value FROM "); | ||
| QUERY.append(getConfig(start).getPrimaryUserTenantsTable()); | ||
| QUERY.append(" WHERE app_id = ? AND tenant_id IN ("); | ||
| for (int i = 0; i < tenantIdsList.size(); i++) { | ||
| QUERY.append("?"); | ||
| if (i != tenantIdsList.size() - 1) { | ||
| QUERY.append(","); | ||
| } | ||
| } |
There was a problem hiding this comment.
Please group this together with adding the params.
| // 1) recipe user's account info -> all tenants of primary user | ||
| String QUERY_1 = "INSERT INTO " + primaryUserTenantsTable | ||
| + " (app_id, tenant_id, account_info_type, account_info_value, primary_user_id)" | ||
| + " SELECT ?, primary_tenants.tenant_id, recipe_ai.account_info_type, recipe_ai.account_info_value, ?" |
There was a problem hiding this comment.
We may want to add explicit types when using a parameter in the subselect "result" directly
Summary of change
(A few sentences about this PR)
Related issues
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your
changes work. Bonus points for screenshots and videos!)
Documentation changes
(If relevant, please create a PR in our docs repo, or create a checklist here
highlighting the necessary changes)
Checklist for important updates
pluginInterfaceSupported.jsonfile has been updated (if needed)build.gradlebuild.gradle, please make sure to add themin
implementationDependencies.json.git tag) in the formatvX.Y.Z, and then find thelatest branch (
git branch --all) whoseX.Yis greater than the latest released tag.OneMillionUsersTestRemaining TODOs for this PR