feat: Add BulkImport APIs and cron #966

anku255 · 2024-03-20T07:30:33Z

Summary of change

(A few sentences about this PR)

Related issues

Bulk User Import into SuperTokens #912

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

src/main/java/io/supertokens/webserver/api/bulkimport/BulkImportAPI.java

src/main/java/io/supertokens/bulkimport/BulkImportUserUtils.java

src/test/java/io/supertokens/test/bulkimport/apis/AddBulkImportUsersTest.java

src/main/java/io/supertokens/cronjobs/bulkimport/ProcessBulkImportUsers.java

src/main/java/io/supertokens/bulkimport/BulkImport.java

src/main/java/io/supertokens/cronjobs/bulkimport/ProcessBulkImportUsers.java

src/test/java/io/supertokens/test/bulkimport/ProcessBulkImportUsersCronJobTest.java

build.gradle

rishabhpoddar · 2024-05-05T09:33:57Z

src/main/java/io/supertokens/webserver/Webserver.java

@@ -26,6 +26,8 @@
 import io.supertokens.pluginInterface.multitenancy.TenantIdentifier;
 import io.supertokens.pluginInterface.multitenancy.exceptions.TenantOrAppNotFoundException;
 import io.supertokens.webserver.api.accountlinking.*;
+import io.supertokens.webserver.api.bulkimport.BulkImportAPI;
+import io.supertokens.webserver.api.bulkimport.DeleteBulkImportUserAPI;


not related to this file, but you need to update the CDI stuff in WebserverAPI.java as well

src/main/java/io/supertokens/bulkimport/BulkImport.java

src/main/java/io/supertokens/cronjobs/bulkimport/ProcessBulkImportUsers.java

src/main/java/io/supertokens/bulkimport/BulkImportUserUtils.java

rishabhpoddar · 2024-05-05T14:30:21Z

src/main/java/io/supertokens/cronjobs/bulkimport/ProcessBulkImportUsers.java

+        // so we have to use an array.
+        String[] errorMessage = { e.getMessage() };
+
+        if (e instanceof StorageTransactionLogicException) {


we should also add the error stack in the message? So the saved error can look like:
{
errorMsg: "...",
errorStack: "..."
}

There is a utils function somewhere in the code to get the error stack from an exception as a string

I don't think we should provide error stack. We don't expect our users to debug the issue by looking at the code. The error message should be clear enough to understand which field has the issue.

oh yea, but users may ask on support that they are getting this error and then for us, it will be way easier to know from where. This can be skipped only if the error messages are globally unique.

We decided to add error codes in the error message to make them globally unique.

src/main/java/io/supertokens/cronjobs/bulkimport/ProcessBulkImportUsers.java

src/main/java/io/supertokens/webserver/api/bulkimport/BulkImportAPI.java

rishabhpoddar · 2024-05-05T14:44:20Z

src/main/java/io/supertokens/webserver/api/bulkimport/DeleteBulkImportUserAPI.java

+        if (arr.size() == 0) {
+            throw new ServletException(new WebserverAPI.BadRequestException("Field name 'ids' cannot be an empty array"));
+        }


in this case, just return status code 200 ok

in the right JSON output format ofc

I think its better to return an error because calling this API with no ids is not a valid case.

it makes it harder for people to call the API. Cause if they have a loop in which they are calling the API, they will be forced to check if the input is empty before calling the API (as opposed to being flexible enough to allow them to check it after and then exit the loop)

That makes sense. I have updated it.

rishabhpoddar · 2024-05-05T14:45:42Z

src/main/java/io/supertokens/webserver/api/bulkimport/BulkImportAPI.java

+        JsonObject input = InputParser.parseJsonObjectOrThrowError(req);
+        JsonArray users = InputParser.parseArrayOrThrowError(input, "users", false);
+
+        if (users.size() <= 0 || users.size() > BulkImport.MAX_USERS_TO_ADD) {


in case it's 0, you just return 200 status code with correct json output.

I think its better to return an error because calling this API with 0 users is not a valid case.

it makes it harder for people to call the API. Cause if they have a loop in which they are calling the API, they will be forced to check if the input is empty before calling the API (as opposed to being flexible enough to allow them to check it after and then exit the loop)

That makes sense. I have updated it.

rishabhpoddar · 2024-05-07T11:15:26Z

src/main/java/io/supertokens/bulkimport/BulkImportUserUtils.java

+        this.allUserRoles = allUserRoles;
+        this.allExternalUserIds = new HashSet<>();
+    }
+


in the bulkimport user type, we also need to allow plain text password cause it will be used during lazy migration task

This is done.

rishabhpoddar · 2024-05-07T11:23:54Z

src/main/java/io/supertokens/cronjobs/bulkimport/ProcessBulkImportUsers.java

+public class ProcessBulkImportUsers extends CronTask {
+
+    public static final String RESOURCE_KEY = "io.supertokens.ee.cronjobs.ProcessBulkImportUsers";
+    private Map<String, SQLStorage> userPoolToStorageMap = new HashMap<>();


need to allow parallelism:

Allow devs to configure the parallelism. Default is 1.

The prop can be called BULK_MIGRATION_PARALLELISM. This is a SaaS protected prop and can be added to PROTECTED_CONFIGS in CoreConfig.java. It should have the @NotConflictingInApp annotation.

You can make a pool of proxy storages here based on the value of this prop, and create those number of threads as well.

src/main/java/io/supertokens/webserver/api/bulkimport/BulkImportAPI.java

rishabhpoddar · 2024-05-24T05:13:26Z

src/main/java/io/supertokens/webserver/api/bulkimport/ImportUserAPI.java

+          .collect(JsonArray::new, JsonArray::add, JsonArray::addAll);
+
+      JsonObject errorResponseJson = new JsonObject();
+      errorResponseJson.add("errors", errors);


what is an example of how this output looks like?

{ "errors": [ "Role role1 does not exist.", "Role role2 does not exist.", "Invalid tenantId: t1 for emailpassword recipe.", "Invalid tenantId: t1 for thirdparty recipe.", "Invalid tenantId: t1 for passwordless recipe." ] }

rishabhpoddar · 2024-05-24T05:14:23Z

src/main/java/io/supertokens/webserver/api/bulkimport/ImportUserAPI.java

+    }
+
+    JsonObject input = InputParser.parseJsonObjectOrThrowError(req);
+    JsonObject jsonUser = InputParser.parseJsonObjectOrThrowError(input, "user", false);


i dont think the input should have a user prop. Cause then its like:

{ user: {...} }

Instead. it should just have all the details in the root of the json input

rishabhpoddar · 2024-05-24T05:15:44Z

src/main/java/io/supertokens/bulkimport/BulkImport.java

+        return StorageUtils.getBulkImportStorage(storage).getBulkImportUsersCount(appIdentifier, status);
+    }
+
+    public static synchronized AuthRecipeUserInfo importUser(Main main, AppIdentifier appIdentifier,


there is a lot of code repetition between this func and the processUser func in the cronjob. Please refactor.

Refactored it.

rishabhpoddar · 2024-05-24T05:34:29Z

src/main/java/io/supertokens/cronjobs/bulkimport/ProcessBulkImportUsers.java

+                // `BulkImportProxyStorage` uses `BulkImportProxyConnection`, which overrides the `.commit()` method on the Connection object.
+                // The `initStorage()` method runs `select * from table_name limit 1` queries to check if the tables exist but these queries
+                // don't get committed due to the overridden `.commit()`, so we need to manually commit the transaction to remove any locks on the tables.
+
+                // Without this commit, a call to `select * from bulk_import_users limit 1` in `doesTableExist()` locks the `bulk_import_users` table,
+                // causing other queries to stall indefinitely.
+                bulkImportProxyStorage.commitTransactionForBulkImportProxyStorage();


can this be moved to the storage layer? it can be override the initStorage function, call the super impl, and then call commitTransactionForBulkImportProxyStorage?

rishabhpoddar · 2024-05-24T05:40:55Z

src/main/java/io/supertokens/cronjobs/bulkimport/ProcessBulkImportUsers.java

+            StorageTransactionLogicException exception = (StorageTransactionLogicException) e;
+            errorMessage[0] = exception.actualException.getMessage();


StorageQueryException usually means something like the db is down or something. All errors that are due to semantic issues have a different type (like emailalreadyexistsexception). Please double check error types

feat: Add BulkImport APIs and cron

0ea0211

anku255 changed the base branch from master to feat/bulk-import-base March 20, 2024 07:30

anku255 added 2 commits March 20, 2024 13:03

chore: update pull request template

f86211a

fix: Use the correct tenant config to create the proxy storage

05dc4a1

rishabhpoddar requested changes Mar 20, 2024

View reviewed changes

anku255 added 2 commits March 21, 2024 16:17

fix: PR changes

5b4658a

fix: PR changes

d721cd5

sattvikc requested changes Apr 1, 2024

View reviewed changes

anku255 added 2 commits April 2, 2024 14:39

fix: PR changes

769cbb6

fix: PR changes

f443a8c

sattvikc requested changes Apr 4, 2024

View reviewed changes

src/main/java/io/supertokens/bulkimport/BulkImport.java Outdated Show resolved Hide resolved

src/main/java/io/supertokens/cronjobs/bulkimport/ProcessBulkImportUsers.java Outdated Show resolved Hide resolved

anku255 added 4 commits April 4, 2024 19:24

fix: PR changes

73b75a8

fix: PR changes

6d59616

fix: Update version

b6c63f4

fix: PR changes

173e7fc

sattvikc requested changes Apr 17, 2024

View reviewed changes

src/test/java/io/supertokens/test/bulkimport/ProcessBulkImportUsersCronJobTest.java Outdated Show resolved Hide resolved

fix: PR changes

4f6ab13

anku255 requested a review from sattvikc April 18, 2024 11:20

sattvikc requested changes Apr 22, 2024

View reviewed changes

build.gradle Show resolved Hide resolved

anku255 added 4 commits April 25, 2024 15:51

Merge branch 'feat/bulk-import-base' into feat/bulk-import-1

7f8ce62

fix: Rename DeleteBulkImportUser API path

fcdfb54

fix: disable bulk import for in-memory db

ddfaa1c

fix: a bug with createTotpDevices

c7ae1e4

rishabhpoddar requested changes May 5, 2024

View reviewed changes

rishabhpoddar requested changes May 7, 2024

View reviewed changes

fix: PR changes

d24bebf

rishabhpoddar requested changes May 8, 2024

View reviewed changes

src/main/java/io/supertokens/webserver/api/bulkimport/BulkImportAPI.java Show resolved Hide resolved

anku255 added 2 commits May 23, 2024 12:38

feat: Add an api to import user in sync

09c1b85

feat: Add an api to get count of bulk import users

119ca1d

rishabhpoddar requested changes May 24, 2024

View reviewed changes

rishabhpoddar reviewed May 24, 2024

View reviewed changes

anku255 added 2 commits May 27, 2024 17:53

fix: PR changes

79a1e67

fix: Add error codes and plainTextPassword import

77cb57c

anku255 mentioned this pull request May 29, 2024

Bulk User Import into SuperTokens #912

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add BulkImport APIs and cron #966

feat: Add BulkImport APIs and cron #966

anku255 commented Mar 20, 2024 •

edited

rishabhpoddar May 5, 2024

anku255 May 29, 2024

rishabhpoddar May 5, 2024

rishabhpoddar May 5, 2024

anku255 May 23, 2024

rishabhpoddar May 24, 2024

anku255 May 29, 2024

rishabhpoddar May 5, 2024

rishabhpoddar May 5, 2024

anku255 May 23, 2024

rishabhpoddar May 24, 2024

anku255 May 27, 2024

rishabhpoddar May 5, 2024

anku255 May 23, 2024

rishabhpoddar May 24, 2024

anku255 May 27, 2024

rishabhpoddar May 7, 2024

anku255 May 29, 2024

rishabhpoddar May 7, 2024

rishabhpoddar May 24, 2024

anku255 May 27, 2024

rishabhpoddar May 24, 2024

anku255 May 27, 2024

rishabhpoddar May 24, 2024

anku255 May 29, 2024

rishabhpoddar May 24, 2024

anku255 May 27, 2024

rishabhpoddar May 24, 2024

		StorageTransactionLogicException exception = (StorageTransactionLogicException) e;
		errorMessage[0] = exception.actualException.getMessage();

feat: Add BulkImport APIs and cron #966

Are you sure you want to change the base?

feat: Add BulkImport APIs and cron #966

Conversation

anku255 commented Mar 20, 2024 • edited

Summary of change

Related issues

Test Plan

Documentation changes

Checklist for important updates

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anku255 commented Mar 20, 2024 •

edited