Create Student, Instructor and User Entities for PostgreSQL Migration #12071

domlimm · 2023-02-10T09:28:04Z

Part of #12048.

Outline

This PR creates the new Student and Instructor Entity to be used for PostgreSQL Migration.

Below are the findings for the 4 available inheritance strategies in Hibernate:

MappedSuperclass

Not scalable. If we use this, ancestors cannot contain associations with other entities
Each DB table contains both base and sub classes properties

Single Table

Performant (polymorphic query performance) as only 1 table needs to be accessed when querying parent entities. Best performance among the other strategies
However, can no longer use NOT NULL constraints on subclass entity properties. I think this means that the identifying attributes of the rows of subclasses can be nullable?
For our case of Student/Instructor IS-A User, we will only have 1 table in the DB, i.e., User, with all the fields combined. This means Single Table is out of our options? Student is a User but it shouldn't have Instructor attributes/data.
Could use discriminator values which is used to differentiate between rows belonging to separate subclass types.

Joined Table aka Table-per-subclass mapping strategy (This has been selected)

TLDR: An inherited state is retrieved by joining with the table of the superclass
Great because the PK of User appears in its child classes, e.g., In Student/Instructor
Performance issue as retrieving entities requires joins between the tables, expensive for large number of records. Number of joins is higher when we query parent class as it will join with every single related child
Discriminator column not required. But each subclass must declare a table column holding the object identifier (which is kind of what we want as each subclass table contains the FKs to the base class, User; PKs of sub class tables are also FKs to the superclass table PK. If @PKJoinCol not set, the PK/FK cols are assumed to have the same names as the PK cols of primary table of the superclass)
When using polymorphic queries, base class table must be joined with all subclass tables to fetch every associated subclass instance

Table per Class

Performance issue because we union in the background when we query the base class i.e., User
Each table defines all persistent states of the class, including the inherited state.
If we want to use polymorphic associations (eg An association to the superclass of our hierarchy), we need to use union subclass mapping. This requires multiple UNION queries, be aware of performance implications of a large class hierarchy. Also, not all database systems support UNION ALL. PostgreSQL81Dialectk supports UNION ALL

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

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

samuelfangjw

Some progress, let's aim to finish this soon!

Comment out code that will cause the build to fail.
Add User, Instructor and Student to the list of annotated classes in HibernateUtil.java
Please keep your branch updated with the v9 branch.

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

samuelfangjw · 2023-02-13T17:39:30Z

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

+    // Cascade?
+    @OneToOne
+    @JoinColumn(name = "teamId")
+    private Team team;


I've decided to move this to User instead. Two reasons, firstly there is a little know "Instructors" team that is hardcoded. Secondly, this gives us the flexibility to support teams for instructors or teams with both student and instructors in the future.

Also, this is definitely not OneToOne. This is a ManyToOne relationship.

Yeah that makes sense!

I've decided to move this to User instead. Two reasons, firstly there is a little know "Instructors" team that is hardcoded. Secondly, this gives us the flexibility to support teams for instructors or teams with both student and instructors in the future.

I think I misunderstood the direction. Please correct me if im wrong, from the Student's POV to have this attribute (previously), we read it as Many Students belong to One Team or One Team has Many Students? Hmmm so confusing! And as we were briefed, we should be going for bidirectional r/s too?

Also, this is definitely not OneToOne. This is a ManyToOne relationship.

For this, might want to take a look at what each annotation means and at the differences between them. For best practices, I try to follow the ones defined here: https://thorben-janssen.com/best-practices-many-one-one-many-associations-mappings/.

Will give it a read! 🙇🏻

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

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

samuelfangjw · 2023-02-13T17:55:10Z

As for cascades, deleting an instructor/student should not cause anything else to be deleted. Deletion should not be cascaded.

samuelfangjw · 2023-02-14T12:19:43Z

@domlimm, before I take a look at this, please make sure the BE can be run without errors and the tables for the entities are correctly created in the db. Thanks!

samuelfangjw

Getting closer, just a few more changes and we can merge

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

samuelfangjw

Some issues but not blockers, to fix in the next PR. I will merge this first as this is a potential blocker for others.

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

samuelfangjw · 2023-02-15T14:03:29Z

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

+    protected Instructor() {
+        // required by Hibernate
+    }
+


Missing constructors for these entities

…stgreSQL Migration (TEAMMATES#12071)

…Migration (#12071)

* [#12048] Set up github action workflows * [#12048] v9: Skeleton implementation (#12056) * [#12048] Add isMigrated flag to course (#12063) * [#12048] Add is migrated flag to datastore account (#12070) * Temporarily disable liquibase migrations * [#12048] Create Notification Entity for PostgreSQL migration (#12061) * [#12048] Create notification DB layer for v9 migration (#12075) * [#12048] Add UsageStatistics entity and db (#12076) * Migrate GetCourseAction.java * Migrate DeleteCourseAction.java and relevant logic functions * Migrate BinCourseAction.java and its related logic functions * Update checkSpecificAccessControl functions in BinCourseAction and DeleteCourseAction classes * Migrate RestoreCourseAction and its related logic functions * Migrate UpdateCourseAction with its related logic functions * [#12048] Add Account Entity (#12087) * [#12048] Create SQL logic for CreateNotificationAction and add relevant tests for v9 migration (#12077) * [#12048] Create Student, Instructor and User Entities for PostgreSQL Migration (#12071) * [#12048] V9: Cleanup and refactor (#12090) * Edit GetCourseAction and refactor out the old datastore code * [#12048] Remove redundant InstructorRole Enum (#12091) * Fix compilation error * Update check for database to fetch from * Add unit tests for CoursesDb * [#12048] Update GetUsageStatisticsAction to include SQL entities (#12084) * Add CoursesLogicTest class * Disable failing tests * Fix compilation error * Fix Checkstyle errors * Merge branch * Change flow for updating courses. * Update updateCourse JavaDoc comment. * Update CreateCourseAction and related methods * Update GetCourseAction. * Update UpdateCourseAction * Update BinCourseAction and RestoreCourseAction * Update DeleteCourseAction * Migrate GetCourseSectionNamesAction and related methods. * Add Unit tests for Logic layer of Course. * Fix Checkstyle errors * Add unit test for GetCourseAction's execute function * Add verify for CoursesDb unit tests and use assertNull and assertNotNull * Move fetching of course to logic layer. * Fix Checkstyle errors. * Move canCreateCourse logic to logic layer. * Change *CourseAction classes to use isCourseMigrated * Fix CoursesLogic's initLogicDependencies method call * Add unit tests for GetCourseAction. * Remove commented out method. * Add minimal unit tests for BinCourseAction, DeleteCourseAction and RestoreCourseAction. * Add minimal unit tests for GetCourseSectionAction and UpdateCourseAction. * Remove unused EntityType parameter. * Add minimal unit tests for CreateCourseAction. * Fix Checkstyle errors. * Ignore all old datastore test cases for *CourseAction classes. * Fix 'text' type to 'test'. * Change binCourseToRecycleBin to return the binned course. * Update moveCourseToRecycleBin test. * Update test name. --------- Co-authored-by: Samuel Fang <samuelfangjw@gmail.com> Co-authored-by: dao ngoc hieu <53283766+daongochieu2810@users.noreply.github.com> Co-authored-by: Samuel Fang <60355570+samuelfangjw@users.noreply.github.com> Co-authored-by: wuqirui <53338059+hhdqirui@users.noreply.github.com> Co-authored-by: Dominic Lim <46486515+domlimm@users.noreply.github.com>

…Migration (#12071)

* [#12048] Set up github action workflows * [#12048] v9: Skeleton implementation (#12056) * [#12048] Add isMigrated flag to course (#12063) * [#12048] Add is migrated flag to datastore account (#12070) * Temporarily disable liquibase migrations * [#12048] Create Notification Entity for PostgreSQL migration (#12061) * [#12048] Create notification DB layer for v9 migration (#12075) * [#12048] Add UsageStatistics entity and db (#12076) * Migrate GetCourseAction.java * Migrate DeleteCourseAction.java and relevant logic functions * Migrate BinCourseAction.java and its related logic functions * Update checkSpecificAccessControl functions in BinCourseAction and DeleteCourseAction classes * Migrate RestoreCourseAction and its related logic functions * Migrate UpdateCourseAction with its related logic functions * [#12048] Add Account Entity (#12087) * [#12048] Create SQL logic for CreateNotificationAction and add relevant tests for v9 migration (#12077) * [#12048] Create Student, Instructor and User Entities for PostgreSQL Migration (#12071) * [#12048] V9: Cleanup and refactor (#12090) * Edit GetCourseAction and refactor out the old datastore code * [#12048] Remove redundant InstructorRole Enum (#12091) * Fix compilation error * Update check for database to fetch from * Add unit tests for CoursesDb * [#12048] Update GetUsageStatisticsAction to include SQL entities (#12084) * Add CoursesLogicTest class * Disable failing tests * Fix compilation error * Fix Checkstyle errors * Merge branch * Change flow for updating courses. * Update updateCourse JavaDoc comment. * Update CreateCourseAction and related methods * Update GetCourseAction. * Update UpdateCourseAction * Update BinCourseAction and RestoreCourseAction * Update DeleteCourseAction * Migrate GetCourseSectionNamesAction and related methods. * Add Unit tests for Logic layer of Course. * Fix Checkstyle errors * Add unit test for GetCourseAction's execute function * Add verify for CoursesDb unit tests and use assertNull and assertNotNull * Move fetching of course to logic layer. * Fix Checkstyle errors. * Move canCreateCourse logic to logic layer. * Change *CourseAction classes to use isCourseMigrated * Fix CoursesLogic's initLogicDependencies method call * Add unit tests for GetCourseAction. * Remove commented out method. * Add minimal unit tests for BinCourseAction, DeleteCourseAction and RestoreCourseAction. * Add minimal unit tests for GetCourseSectionAction and UpdateCourseAction. * Remove unused EntityType parameter. * Add minimal unit tests for CreateCourseAction. * Fix Checkstyle errors. * Ignore all old datastore test cases for *CourseAction classes. * Fix 'text' type to 'test'. * Change binCourseToRecycleBin to return the binned course. * Update moveCourseToRecycleBin test. * Update test name. --------- Co-authored-by: Samuel Fang <samuelfangjw@gmail.com> Co-authored-by: dao ngoc hieu <53283766+daongochieu2810@users.noreply.github.com> Co-authored-by: Samuel Fang <60355570+samuelfangjw@users.noreply.github.com> Co-authored-by: wuqirui <53338059+hhdqirui@users.noreply.github.com> Co-authored-by: Dominic Lim <46486515+domlimm@users.noreply.github.com>

samuelfangjw reviewed Feb 10, 2023

View reviewed changes

src/main/java/teammates/storage/sqlentity/Student.java Outdated Show resolved Hide resolved

samuelfangjw reviewed Feb 10, 2023

View reviewed changes

src/main/java/teammates/storage/sqlentity/Student.java Outdated Show resolved Hide resolved

domlimm changed the title ~~Create Student Entity for PostgreSQL Migration~~ Create Student and Instructor Entity for PostgreSQL Migration Feb 11, 2023

domlimm mentioned this pull request Feb 11, 2023

Create Instructor Entity for PostgreSQL Migration #12073

Closed

domlimm marked this pull request as ready for review February 12, 2023 10:11

samuelfangjw reviewed Feb 12, 2023

View reviewed changes

src/main/java/teammates/storage/sqlentity/Instructor.java Outdated Show resolved Hide resolved

src/main/java/teammates/storage/sqlentity/Instructor.java Outdated Show resolved Hide resolved

src/main/java/teammates/storage/sqlentity/Instructor.java Outdated Show resolved Hide resolved

domlimm requested a review from samuelfangjw February 13, 2023 17:01

samuelfangjw requested changes Feb 13, 2023

View reviewed changes

domlimm force-pushed the v9-migration-student-entity branch from 6bb30be to 7907786 Compare February 14, 2023 08:13

domlimm changed the title ~~Create Student and Instructor Entity for PostgreSQL Migration~~ Create Student, Instructor and User Entities for PostgreSQL Migration Feb 14, 2023

domlimm requested a review from samuelfangjw February 14, 2023 12:15

domlimm force-pushed the v9-migration-student-entity branch from 1fa227b to 2659481 Compare February 15, 2023 04:32

domlimm added 16 commits February 15, 2023 15:56

Init student entity

c7e9f38

Add foreign key mappings and update related constructor and methods

782d557

Set up base Student entity with Joined table inheritance strategy

61686e7

Clean up comments and remove Java inheritance as parent undefined

a057ace

Fix inconsistent attributes with printing Student

6fee193

Set attributes to not nullable

697c49a

Add associations

d40e7b8

Shift class constructor for readability

777720c

Add instructor entity

dd4c40b

Set attributes to not nullable

fa736c6

Remove PK join column and duplicate id attribute

3a4f222

Add mapping for Team attribute and clean up

bb65ff2

Remove unused imports

9a26d1a

Fix import and wrong type

c780280

Change data type

8e98e8f

Add User entity

273e4ef

samuelfangjw reviewed Feb 15, 2023

View reviewed changes

src/main/java/teammates/storage/sqlentity/Student.java Outdated Show resolved Hide resolved

domlimm added 9 commits February 15, 2023 18:14

Remove comment

2157040

Fix association for Course attribute

e4bfcb6

Update inherited methods' body

ad925da

Fix checkstyle

4e3cab2

Remove backwards compatibility code

64ad8e6

Move InstructorPermissionRole enum

01950fa

Add missing foreign key column

99fb95c

Update equivalence conditions

8765b0c

Fix checkstyle

ec30f4f

domlimm requested review from samuelfangjw and hhdqirui and removed request for samuelfangjw and hhdqirui February 15, 2023 13:57

samuelfangjw approved these changes Feb 15, 2023

View reviewed changes

samuelfangjw merged commit 780101a into TEAMMATES:v9-migration Feb 15, 2023

domlimm added a commit to domlimm/teammates that referenced this pull request Feb 22, 2023

[TEAMMATES#12048] Create Student, Instructor and User Entities for Po…

33cc42f

…stgreSQL Migration (TEAMMATES#12071)

samuelfangjw pushed a commit that referenced this pull request Feb 26, 2023

[#12048] Create Student, Instructor and User Entities for PostgreSQL …

1e5804b

…Migration (#12071)

samuelfangjw pushed a commit that referenced this pull request Apr 8, 2023

[#12048] Create Student, Instructor and User Entities for PostgreSQL …

036165a

…Migration (#12071)

wkurniawan07 added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging c.Task Other non-user-facing works, e.g. refactoring, adding tests labels Jan 21, 2024

wkurniawan07 added this to the V9.0.0-beta.0 milestone Jan 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create Student, Instructor and User Entities for PostgreSQL Migration #12071

Create Student, Instructor and User Entities for PostgreSQL Migration #12071

domlimm commented Feb 10, 2023 •

edited

Loading

samuelfangjw left a comment •

edited

Loading

samuelfangjw Feb 13, 2023

samuelfangjw Feb 13, 2023

domlimm Feb 14, 2023 •

edited

Loading

samuelfangjw Feb 14, 2023

domlimm Feb 14, 2023

samuelfangjw commented Feb 13, 2023

samuelfangjw commented Feb 14, 2023

samuelfangjw left a comment

samuelfangjw left a comment

samuelfangjw Feb 15, 2023

Create Student, Instructor and User Entities for PostgreSQL Migration #12071

Create Student, Instructor and User Entities for PostgreSQL Migration #12071

Conversation

domlimm commented Feb 10, 2023 • edited Loading

samuelfangjw left a comment • edited Loading

Choose a reason for hiding this comment

samuelfangjw Feb 13, 2023

Choose a reason for hiding this comment

samuelfangjw Feb 13, 2023

Choose a reason for hiding this comment

domlimm Feb 14, 2023 • edited Loading

Choose a reason for hiding this comment

samuelfangjw Feb 14, 2023

Choose a reason for hiding this comment

domlimm Feb 14, 2023

Choose a reason for hiding this comment

samuelfangjw commented Feb 13, 2023

samuelfangjw commented Feb 14, 2023

samuelfangjw left a comment

Choose a reason for hiding this comment

samuelfangjw left a comment

Choose a reason for hiding this comment

samuelfangjw Feb 15, 2023

Choose a reason for hiding this comment

domlimm commented Feb 10, 2023 •

edited

Loading

samuelfangjw left a comment •

edited

Loading

domlimm Feb 14, 2023 •

edited

Loading