[#13691] Migrate storage + logic layer from google id by Nsohko · Pull Request #13755 · TEAMMATES/teammates

Nsohko · 2026-04-11T13:27:40Z

Copilot

Pull request overview

This PR is part of issue #13691 and continues the migration away from using googleId as the stable account key by introducing OIDC concepts (issuer + subject) and shifting internal lookups toward Account UUIDs.

Changes:

Introduces LoginIssuer (+ CRUD db/logic) to map OIDC issuer URLs to provider names.
Refactors SQL storage/logic APIs and tests to use accountId (UUID) instead of googleId for linking and queries.
Replaces Google ID field validation/tests with OIDC issuer/subject validation.

Reviewed changes

Copilot reviewed 25 out of 25 changed files in this pull request and generated 15 comments.

Show a summary per file

File	Description
src/test/java/teammates/test/BaseTestCase.java	Updates typical `Account` construction to include issuer/subject/loginIdentifier.
src/test/java/teammates/storage/sqlapi/LoginIssuerDbTest.java	Adds unit tests for `LoginIssuerDb` CRUD behaviors.
src/test/java/teammates/storage/sqlapi/AccountsDbTest.java	Updates account tests to use UUID-based APIs; adds null-delete coverage.
src/test/java/teammates/sqllogic/core/UsersLogicTest.java	Updates reset flows to accountId and adds tests for attaching accounts to instructors.
src/test/java/teammates/sqllogic/core/DataBundleLogicTest.java	Updates data bundle deletion assertions to delete accounts by UUID.
src/test/java/teammates/sqllogic/core/AccountsLogicTest.java	Updates delete/join tests for UUID-based account APIs and new join flow.
src/test/java/teammates/common/util/FieldValidatorTest.java	Replaces Google ID validation tests with OIDC issuer/subject tests.
src/main/java/teammates/storage/sqlentity/User.java	Replaces `getGoogleId` helper with `getAccountId` helper.
src/main/java/teammates/storage/sqlentity/LoginIssuer.java	Adds new SQL entity for issuer→provider mapping.
src/main/java/teammates/storage/sqlentity/Account.java	Refactors `Account` to store `LoginIssuer`, OIDC `subject`, and a `loginIdentifier`; adds uniqueness constraint.
src/main/java/teammates/storage/sqlapi/UsersDb.java	Refactors user lookup helpers from googleId to accountId; updates search to use `loginIdentifier`.
src/main/java/teammates/storage/sqlapi/LoginIssuerDb.java	Adds persistence layer for `LoginIssuer`.
src/main/java/teammates/storage/sqlapi/AccountsDb.java	Removes googleId natural-id lookup; updates create logic to UUID-based access.
src/main/java/teammates/sqllogic/core/UsersLogic.java	Refactors joins/resets and helper methods to accountId; adds attach-account helper.
src/main/java/teammates/sqllogic/core/LoginIssuerLogic.java	Adds logic wrapper for `LoginIssuerDb`.
src/main/java/teammates/sqllogic/core/LogicStarter.java	Wires `LoginIssuerLogic` into dependency initialization.
src/main/java/teammates/sqllogic/core/DataBundleLogic.java	Updates account deletion in bundle removal to UUID-based deletion.
src/main/java/teammates/sqllogic/core/CoursesLogic.java	Updates course creation and student course listing to accountId-based APIs.
src/main/java/teammates/sqllogic/core/AccountsLogic.java	Refactors delete/join flows to accountId and delegates instructor linking to UsersLogic.
src/main/java/teammates/sqllogic/api/Logic.java	Updates public SQL logic API surface to accountId + adds LoginIssuer APIs.
src/main/java/teammates/common/util/SanitizationHelper.java	Replaces googleId sanitizer with loginIdentifier sanitizer.
src/main/java/teammates/common/util/HibernateUtil.java	Registers `LoginIssuer` as a managed Hibernate entity.
src/main/java/teammates/common/util/FieldValidator.java	Adds OIDC issuer/subject validation; removes Google ID validation.
src/main/java/teammates/common/util/Const.java	Adds OIDC-related constants used by validation.
src/main/java/teammates/common/datatransfer/OidcProviderNameType.java	Adds provider name enum for `LoginIssuer`.

Comments suppressed due to low confidence (1)

src/main/java/teammates/storage/sqlentity/Account.java:159

Account#getInvalidityInfo does not validate required linkage fields like loginIssuer (non-null) or loginIdentifier (non-null), so Account.isValid() may return true while persistence fails due to NOT NULL / FK constraints. Consider adding explicit validation for loginIssuer (non-null and/or loginIssuer.isValid()) and loginIdentifier (non-null/length/format as appropriate) to keep errors consistent with other entities.

    @Override
    public List<String> getInvalidityInfo() {
        List<String> errors = new ArrayList<>();
        addNonEmptyError(FieldValidator.getInvalidityInfoForOidcSubject(subject), errors);
        addNonEmptyError(FieldValidator.getInvalidityInfoForPersonName(name), errors);
        addNonEmptyError(FieldValidator.getInvalidityInfoForEmail(email), errors);

        return errors;

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/main/java/teammates/storage/sqlapi/AccountsDb.java

src/main/java/teammates/common/util/FieldValidator.java

src/main/java/teammates/storage/sqlentity/LoginIssuer.java

src/main/java/teammates/sqllogic/api/Logic.java

src/main/java/teammates/sqllogic/core/UsersLogic.java

iZUMi-kyouka · 2026-04-13T17:11:14Z

src/main/java/teammates/common/util/Const.java

Issuer scheme is always HTTPS as per the spec. So what's the use of this constant

iZUMi-kyouka · 2026-04-13T17:11:37Z

src/main/java/teammates/common/datatransfer/OidcProviderNameType.java

+
+        return null;
+    }
+}


Please add newline at EOF

iZUMi-kyouka · 2026-04-13T17:21:22Z

src/main/java/teammates/common/util/FieldValidator.java

+                    && !issuerUri.isOpaque();
+
+            if (!isValid) {
+                return getPopulatedErrorMessage(OIDC_ISSUER_ERROR_MESSAGE, issuer, OIDC_ISSUER_FIELD_NAME,


Can this be simplified to reduce duplication?

iZUMi-kyouka · 2026-04-13T17:23:24Z

src/main/java/teammates/common/util/FieldValidator.java

+     * contains scheme, host, and optionally, port number and path components and no query or fragment components.
+     *
+     * @return An explanation of why the {@code issuer} is not acceptable.
+     *         Returns an empty string if the {@code issuer} is acceptable.


Wrong comment? "returns.."

iZUMi-kyouka · 2026-04-13T17:26:09Z

src/main/java/teammates/sqllogic/api/Logic.java

+    /**
+     * Updates an issuer to provider name mapping.
+     */
+    public LoginIssuer updateLoginIssuer(LoginIssuer loginIssuer)


When would we need this? Seems unlikely that we need to update a mapping

iZUMi-kyouka · 2026-04-13T17:27:33Z

src/main/java/teammates/sqllogic/api/Logic.java

     * privileges.
     *
-     * @param instructorGoogleId the Google ID of the instructor creating the
+     * @param instructorAccountId the Account ID of the instructor creating the


I think account should not be capitalised

iZUMi-kyouka · 2026-04-13T17:28:16Z

src/main/java/teammates/sqllogic/api/Logic.java

-     * Parameters regkey and googleId are non-null.
+     * Validates that an existing instructor can be linked to an existing account, then performs the link.
+     *
+     * <p>This is a lower-level helper flow than the registration-key-based join path.</p>


Why do we need this comment?

iZUMi-kyouka · 2026-04-13T17:28:34Z

src/main/java/teammates/sqllogic/api/Logic.java

-        if (googleId == null) {
-            throw new InvalidParametersException("Instructor's googleId cannot be null");
+        if (accountId == null) {
+            throw new InvalidParametersException("Instructor's account Id cannot be null");


iZUMi-kyouka · 2026-04-13T17:30:55Z

src/main/java/teammates/sqllogic/api/Logic.java

-        validateJoinCourseRequest(googleId, instructor);
-        return usersLogic.joinCourseForInstructor(googleId, instructor);
+        validateInstructorAccountLinkRequest(accountId, instructor);
+        return usersLogic.attachAccountToInstructor(accountId, instructor);


I personally think "link" sounds better

iZUMi-kyouka · 2026-04-13T17:34:23Z

src/main/java/teammates/sqllogic/core/UsersLogic.java


-        // Update the googleId of the student entity for the instructor which was created from sample data.
+        // If this instructor already has a student record in the same course
+        // without an existing account (e.g. from sample/imported data),


This is not possible due to unique constrant in schema

TobyCyan · 2026-04-14T03:01:04Z

src/main/java/teammates/common/datatransfer/OidcProviderNameType.java

+        if (providerName == null) {
+            return null;
+        }


Maybe can use StringHelper.isEmpty for null check

TobyCyan · 2026-04-14T03:04:39Z

src/main/java/teammates/common/util/FieldValidator.java


-    public static final String GOOGLE_ID_FIELD_NAME = "Google ID";
-    public static final int GOOGLE_ID_MAX_LENGTH = 254;
+    public static final String OIDC_ISSUER_FIELD_NAME = "OIDC issuer";


Should there be a max length for issuer too?

TobyCyan · 2026-04-14T03:26:14Z

src/main/java/teammates/sqllogic/core/AccountsLogic.java

            }
        } else {
-            // Check if this Google ID has already joined this course
+            // Check if this Account ID has already joined this course


The A in Account should be lowercase to be uniform with UserInfo

TobyCyan · 2026-04-14T03:27:29Z

src/main/java/teammates/sqllogic/core/AccountsLogic.java

        if (instructorForKey.isRegistered()) {
-            if (instructorForKey.getGoogleId().equals(googleId)) {
-                Account existingAccount = accountsDb.getAccountByGoogleId(googleId);
+            if (instructorForKey.getAccountId().equals(accountId)) {
+                Account existingAccount = accountsDb.getAccount(accountId);
                if (existingAccount != null) {
                    throw new EntityAlreadyExistsException("Instructor has already joined course");
                }
            } else {
                throw new EntityAlreadyExistsException("Instructor has already joined course");
            }
        } else {


Maybe can consider guard clause to avoid deep nesting here.

TobyCyan · 2026-04-14T03:28:58Z

src/main/java/teammates/sqllogic/core/CoursesLogic.java

     * Returns a list of {@link Course} for all courses a given student is enrolled in.
     *
-     * @param googleId The Google ID of the student
+     * @param accountId The Account ID of the student


TobyCyan · 2026-04-14T03:40:23Z

src/main/java/teammates/storage/sqlapi/AccountsDb.java


-        if (getAccountByGoogleId(account.getGoogleId()) != null) {
+        if (getAccount(account.getId()) != null ||
+                getAccountByLoginIssuerAndSub(account.getLoginIssuer(), account.getOidcSubject())!= null) {


Should have a space between operand and operator at account.getOidcSubject())!=

Nsohko added 4 commits April 11, 2026 12:28

Update entity layer

784bbf3

Update db layer

53318bc

Migrate logic layer

a2a042e

Fix lint

17c7ab9

Nsohko requested a review from mingyuanc April 11, 2026 13:28

mingyuancode requested review from Copilot and iZUMi-kyouka April 11, 2026 13:47

Copilot started reviewing on behalf of mingyuancode April 11, 2026 13:48 View session

Copilot AI reviewed Apr 11, 2026

View reviewed changes

Fix copilot feedback

cb8d70e

iZUMi-kyouka reviewed Apr 13, 2026

View reviewed changes

TobyCyan reviewed Apr 14, 2026

View reviewed changes

Conversation

Nsohko commented Apr 11, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iZUMi-kyouka Apr 13, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TobyCyan Apr 14, 2026 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

iZUMi-kyouka Apr 13, 2026 •

edited

Loading

TobyCyan Apr 14, 2026 •

edited

Loading