houston-user-deactivation-test.md

About SQLAlchemy database persistence

Problem

User deactivation didn't include code to persist to the database (e.g. db.session.merge(user)). Why did the user deactivation test pass?

Links to code (before fix)

• User deactivation code (resources): https://github.com/WildMeOrg/houston/blob/c35500bec2d324775935009fe5451aef683023fb/app/modules/users/resources.py#L195-L210

• User deactivation code (model): https://github.com/WildMeOrg/houston/blob/c35500bec2d324775935009fe5451aef683023fb/app/modules/users/models.py#L720-L732

• User deactivation test: https://github.com/WildMeOrg/houston/blob/c35500bec2d324775935009fe5451aef683023fb/tests/modules/users/resources/test_signup.py#L286-L317

Investigation findings

1. User.query.get(user_guid) returns the same instance in the app and in the test

   Same instance meaning hash(user) is the same.

   When using <model>.query.get(<primary-key>), sqlalchemy caches it in its "identity map". This means it doesn't try to fetch from the database if it is already in the identity map.

   So when the app modifies the user, then the test tries to assert the changes to the user, the test will pass even if the changes are not in the database.

   It is possible to force sqlalchemy to fetch from the database by doing User.query.filter(User.guid == user_guid).first().

2. The deactivated changes was actually persisted to the test database

   Surprisingly even though there is no code to persist the changes to the database, when I looked at the houston_test database, the changes were there:

   karen@yukihira:~/src/wildme/houston$ docker-compose exec db psql -U houston houston_test -c 'select * from "user"'
             created           |          updated           |           viewed           |                 guid                 | version |
      email               |                                                          password
    |     full_name     | website | location | affiliation | forum_id | locale | accepted_user_agreement | use_usa_date_format | show_email_in_profile
    | receive_notification_emails | receive_newsletter_emails | shares_data | default_identification_catalogue | profile_fileupload_guid | static_role
   s
   ----------------------------+----------------------------+----------------------------+--------------------------------------+---------+-----------
   -----------------------+---------------------------------------------------------------------------------------------------------------------------
   -+-------------------+---------+----------+-------------+----------+--------+-------------------------+---------------------+----------------------
   -+-----------------------------+---------------------------+-------------+----------------------------------+-------------------------+------------
   --
    2021-10-15 15:30:26.738675 | 2021-10-15 15:30:26.738684 | 2021-10-15 15:30:26.73881  | c4dc08e1-f8b8-4971-b282-d57f227451af |         | useradmin@
   localhost              | \x243262243132246c462e37735252736d66376e59444635384f58456d4f6970677045396d325676306b62737631706c2f474f48612f32355671526f6d
    | First Middle Last |         |          |             |          | EN     | f                       | t                   | f
    | t                           | f                         | t           |                                  |                         |       79104
   0
    2021-10-15 15:30:27.048615 | 2021-10-15 15:30:27.048631 | 2021-10-15 15:30:27.048638 | a77915ed-0135-4d09-8702-7cd4f0b69f8b |         | oauth-user
   @wildme.org            | \x2432622431322442325452394e47434664626e62736462692f436561754765764957533177776d57326f4377354e3430305a5548422e46397a574a69
    |                   |         |          |             |          | EN     | f                       | t                   | f
    | t                           | f                         | t           |                                  |                         |       29900
   8
    2021-10-15 15:30:27.573834 | 2021-10-15 15:30:27.573851 | 2021-10-15 15:30:27.573858 | 61ad59de-8188-4bb3-9294-e00f170d3316 |         | -631943609
   8885476203@deactivated | \x24326224313224446a746d355930434352423339582f734448724f5665613239422e53514d677a4368624533797447304c764c38305971382f533243
    | Inactivated User  |         |          |             |          | EN     | f                       | t                   | f
    | t                           | f                         | t           |                                  |                         |
   0
   (3 rows)
   

   At what point was the deactivation changes persisted? It's actually in our test client code https://github.com/WildMeOrg/houston/blob/c35500bec2d324775935009fe5451aef683023fb/tests/utils.py#L92

   with db.session.begin():
   

3. with db.session.begin(): persists objects that have changed

   Even if you do:

   with db.session.begin():
       pass
   

   The changed user would still persist to the database because (I think, not tested) we set sqlalchemy to auto commit in https://github.com/WildMeOrg/houston/blob/c35500bec2d324775935009fe5451aef683023fb/app/extensions/flask_sqlalchemy/init.py#L52:

   kwargs['session_options']['autocommit'] = True
   

4. User.query.get(user_guid) also persists changes to the database

   I commented out the code in the test client to not do any db.session stuff, and still the deactivation changes got persisted. This time it was this line:

   User.query.get(user_guid)
   

   Doing the above line in the test persisted the deactivation changes.

   But even if we don't do this in the test and use the GET /api/v1/users/<user-guid> api, the deactivation changes still got persisted after the API call.

Conclusion

There is no way to check in our current test set up to check that we forgot to persist data to the database.

Suggestions

1. Use actual integration tests with requests or selenium against an actual running houston instance (which I'm working on)

   I wrote a test using requests with the user deactivation stuff and checked the user instances for the DELETE and GET apis and they are different, so the test failed as expected.

2. Add some middleware code to check there's no unpersisted changes when response is returned?