TEAMMATES / teammates

This is the project website for the TEAMMATES feedback management tool for education
https://teammatesv4.appspot.com/
GNU General Public License v2.0
1.66k stars 3.3k forks source link

Data Migration: Setter changes are persisted before data verification checks #12779

Open marquestye opened 7 months ago

marquestye commented 7 months ago

In some logic class methods,

  1. an entity is retrieved from the database,
  2. the entity's fields are updated with new values, before
  3. the entity is passed into an db.updateEntity() method, which then performs the verification checks.

For example:

https://github.com/TEAMMATES/teammates/blob/b69776810e1db550075cc31115f8fd3c573673ae/src/main/java/teammates/sqllogic/core/FeedbackQuestionsLogic.java#L193-L211

https://github.com/TEAMMATES/teammates/blob/b69776810e1db550075cc31115f8fd3c573673ae/src/main/java/teammates/sqllogic/core/FeedbackResponsesLogic.java#L205-L214

However, according to the Hibernate documentation, changes made by the setters are automatically persisted into the database, even though the new values could be invalid and fail the verification checks. This results in the database entity being polluted with invalid data.

I've tested this locally on NotificationsLogic.updateNotification: https://github.com/TEAMMATES/teammates/blob/b69776810e1db550075cc31115f8fd3c573673ae/src/main/java/teammates/sqllogic/core/NotificationsLogic.java#L81-L90

    @Test
    public void testUpdateNotification_invalidParameters_originalUnchanged() throws Exception {

        Notification notif = new Notification(
                Instant.parse("2011-01-01T00:00:00Z"),
                Instant.parse("2099-01-01T00:00:00Z"),
                NotificationStyle.DANGER,
                NotificationTargetUser.GENERAL,
                "A deprecation note",
                "<p>Deprecation happens in three minutes</p>");
        notificationsLogic.createNotification(notif);

        String invalidLongTitle = "1234567890".repeat(10);

        assertThrows(
                InvalidParametersException.class,
                () -> notificationsLogic.updateNotification(
                        notif.getId(),
                        Instant.parse("2011-01-01T00:00:00Z"),
                        Instant.parse("2099-01-01T00:00:00Z"),
                        NotificationStyle.DANGER,
                        NotificationTargetUser.GENERAL,
                        invalidLongTitle,
                        "<p>Deprecation happens in three minutes</p>"
                )
        );

        Notification actual = notificationsLogic.getNotification(notif.getId());
        assert actual.getTitle().equals("A deprecation note") : actual.getTitle();
        // java.lang.AssertionError: 12345678901234567890...

    }

The update fails, but the entity retrieved from the database afterwards is still updated with invalidLongTitle.

jayasting98 commented 7 months ago

The only solutions I can think of are to validate before using the setters, or evicting the object from the session cache.

void evict(Object object) throws HibernateException Remove this instance from the session cache. Changes to the instance will not be synchronized with the database. This operation cascades to associated instances if the association is mapped with cascade="evict".

See also: