Remove user data from couch user model #34805

esoergel · 2024-06-21T13:42:55Z

Product Description

Technical Summary

This is cleanup from the migration to SQL that happened earlier in the year. The couch field is no longer being updated and isn't in use anywhere, so this PR removes it. Turns out removing that field breaks a couple things that accessed the raw user document. Fixes for those are included.

Since the model change needs to go live before the migration is run, this PR includes a management command to be run post-deploy. #34798 will include a django migration that calls that command for 3rd-party environments.

Feature Flag

Safety Assurance

There were apparently a handful of places in tests that took issue with this field going away, so there is a chance there are other unknown, untested areas that reference this.

Safety story

Automated test coverage

QA Plan

Migrations

This has a model change and a management command, but the automatic migration will come in a second PR

Rollback instructions

This PR can be reverted after deploy with no further considerations

Labels & Review

Risk label is set correctly
The set of people pinged as reviewers is appropriate for the level of risk of the change

It is overridden anyways in dehydrate

This was previously checking that user data changed before calling `add_changes`, and then checking again inside `add_changes`, but this time against the couch field, which shouldn't be in-use anymore

The migration will come later

The query only returns docs that _don't_ have that value set, then fetches those from couch and attempts to get that value. However, this information is no longer stored in couch anyways.

esoergel · 2024-06-21T17:36:16Z

corehq/apps/geospatial/views.py

@@ -367,32 +366,28 @@ def get_paginated_cases_without_gps(self, domain, page, limit):

 def _get_paginated_users_without_gps(domain, page, limit, query):
    location_prop_name = get_geo_user_property(domain)
-    query = (
+    res = (


@zandre-eng What do you think of this change?

Apologies for the late response on this as I was away on PTO. The changes look good, and it's nice that we're able to handle pagination now directly in the ES query.

These have been broken since the migration to SQL, apparently

Jtang-1

reviewed, didn't have any additional comments but not enough confidence of the context to approve

MartinRiese · 2024-06-26T19:11:38Z

corehq/apps/user_importer/helpers.py

@@ -174,20 +174,16 @@ def update_user_data(self, data, uncategorized_data, profile_name, profiles_by_n
        if user_data.profile_id and user_data.profile_id != old_profile_id:
            self.logger.add_info(UserChangeMessage.profile_info(user_data.profile_id, profile_name))

+        if old_user_data != user_data.raw:
+            self.logger.add_changes({'user_data': user_data.raw}, skip_confirmation=True)


I understand we did that before too. But is logging the user data a good idea?

Can you elaborate? This code logs all changes made to a user for auditing purposes.

Maybe it is just the name of the field being user_data and in actuality it does not contain anything sensitive. But I wonder if there was a chance something that we don't want logged ends up in there. I think it would not be clear to every developer changing what goes in user_data that it will also end up in the logs.

true - though logger here actually isn't a normal python logger, it's an instance of UserChangeLogger, which saves details of changes to postgres to power an auditing report.

MartinRiese · 2024-06-26T19:16:37Z

corehq/apps/domain_migration_flags/api.py

@@ -126,3 +127,25 @@ def all_domains_with_migrations_in_progress():
 def reset_caches(domain, slug):
    any_migrations_in_progress(domain, strict=True)
    get_migration_status(domain, slug, strict=True)
+
+
+def once_off_migration(slug):


I like this.

AddisonDunn · 2024-06-26T19:13:52Z

corehq/apps/api/resources/v0_1.py

@@ -61,7 +61,7 @@ class Meta(CustomResourceMeta):

 class CommCareUserResource(UserResource):
    groups = fields.ListField(attribute='get_group_ids')
-    user_data = fields.DictField(attribute='user_data')
+    user_data = fields.DictField()


Won't the API still be trying to update the user_data attribute on the CommCareUser model? Or is that somehow otherwise accounted for?

Yep, good question. That's handled here:

commcare-hq/corehq/apps/api/user_updates.py

Lines 112 to 122 in d3d0b80

def _update_user_data(user, new_user_data, user_change_logger):

try:

profile_id = new_user_data.pop(PROFILE_SLUG, ...)

changed = user.get_user_data(user.domain).update(new_user_data, profile_id=profile_id)

except UserDataError as e:

raise UpdateUserException(str(e))

if user_change_logger and changed:

user_change_logger.add_changes({

'user_data': user.get_user_data(user.domain).raw

})

Ah I see, obj_update should manually override the update for this field.

AddisonDunn · 2024-06-26T20:36:49Z

corehq/apps/users/util.py

    from corehq.apps.users.model_log import UserModelAction

    if action in [UserModelAction.CREATE, UserModelAction.DELETE]:
        changed_details = couch_user.to_json()
+        changed_details['user_data'] = couch_user.get_user_data(for_domain).raw if for_domain else {}


Just checking my understanding: the logs have been missing the user data field for the last few months cause user data is already migrated?

that's right 😬

AddisonDunn · 2024-06-26T20:55:00Z

corehq/apps/geospatial/views.py

        .sort('created_on', desc=True)
+        .start((page - 1) * limit)
+        .size(limit)
+        .run()


Man was this pulling all the users every function call when you could just pass the page directly to ES? 💀

yep. And then fetching the users again from couch. It does the same thing in the function below this one too - I just didn't want to get carried away with cleanup.

Nice, I see tests cover this

AddisonDunn · 2024-06-27T13:55:03Z

corehq/apps/userreports/expressions/specs.py

            return None
+        if related_doc_type == 'CommCareUser':
+            doc['user_data'] = CommCareUser.wrap(doc).get_user_data(domain).to_dict()


I guess user data just hasn't been part of UCRs since the migration?

Do these expressions not apply to WebUsers too?

I guess user data just hasn't been part of UCRs since the migration?

That's right - this has been broken since the migration.

You can't do a related doc lookup on a web user, since they don't have a top level domain attribute. For the same reason, they also don't have a user_data attribute (since it varies from one domain to the next), though I guess we could inject one in this context.

For what it's worth, this means of directly accessing the user_data from the raw JSON has been broken to some degree for years - user data is only supposed to be accessed via helper methods, as fields controlled by the profile aren't stored directly in the user document. That means that the profile wasn't accounted for in the API or in UCR. I just broke it further 😆

corehq/dbaccessors/couchapps/all_docs.py

AddisonDunn · 2024-06-27T14:25:44Z

corehq/apps/users/management/commands/rm_couch_user_data.py

+                 + get_doc_count_by_type(db, 'CommCareUser'))
+        all_ids = chain(iter_all_doc_ids(db, 'WebUser'),
+                        iter_all_doc_ids(db, 'CommCareUser'))
+        iter_update(db, _update_user, with_progress_bar(all_ids, count), verbose=True)


To check my understanding, is this removing the couch data by just unwrapping and re-wrapping the doc without the user_data field?

iter_update is akin to map - it fetches documents by ID in chunks, applies _update_user to them, and saves the return value (also in chunks) if anything changed. So here _update_user defines the actual change to each document, which is user_doc.pop('user_data', ...) (.pop differs from .get in that it mutates the object).

Ah, gotcha. I wasn't registering that pop is removing the value (I know that it does, just sort of slips my mind sometimes). That makes sense.

AddisonDunn · 2024-06-27T14:25:46Z

corehq/apps/users/management/commands/rm_couch_user_data.py

+
+
+class Command(BaseCommand):
+    help = "Populate SQL user data from couch"


I think this migration just removes couch data not ~~doesn't~~ populate SQL? Or is there some magic in saving the doc that populates SQL I haven't discovered?

oh whoops you're right - I forgot to update the help text. Let me do that now

esoergel · 2024-06-27T21:13:27Z

Just ran the management command on staging, where it succeeded without issues:

Elapsed time: 0:28:04
couldn't find 0 docs
ignored 86 docs
deleted 0 docs
updated 76144 docs

esoergel · 2024-07-16T20:38:39Z

Ran the management command on this PR on prod:

Started at 2024-07-16 19:26:20
Processing [..................................................] 1395446/1395444 100% 1:02:32.965644 elapsed
Finished at 2024-07-16 20:28:53
Elapsed time: 1:02:33

india:

Started at 2024-07-16 19:27:33
Processing [..................................................] 159858/159858 100% 0:18:35.724473 elapsed
Finished at 2024-07-16 19:46:09
Elapsed time: 0:18:36

and swiss:

Started at 2024-07-16 19:28:49
Processing [..................................................] 935/935 100% 0:00:02.457244 elapsed
Finished at 2024-07-16 19:28:51
Elapsed time: 0:00:02

esoergel added 4 commits June 19, 2024 12:58

Remove old migration

07d5a70

This is no longer a class attribute

160d540

It is overridden anyways in dehydrate

Check user data changes against SQL original

45137f3

This was previously checking that user data changed before calling `add_changes`, and then checking again inside `add_changes`, but this time against the couch field, which shouldn't be in-use anymore

Log user data changes and not just from to_json

8e813e2

dimagimon added reindex/migration Reindex or migration will be required during or before deploy Risk: High Change affects files that have been flagged as high risk. Risk: Medium Change affects files that have been flagged as medium risk. labels Jun 21, 2024

Remove unused user data field

983b087

The migration will come later

esoergel force-pushed the es/user-data-prep branch from 8da070c to 983b087 Compare June 21, 2024 13:52

Just use ES results directly

d04391c

The query only returns docs that _don't_ have that value set, then fetches those from couch and attempts to get that value. However, this information is no longer stored in couch anyways.

esoergel force-pushed the es/user-data-prep branch from 675b859 to d04391c Compare June 21, 2024 17:35

esoergel commented Jun 21, 2024

View reviewed changes

esoergel mentioned this pull request Jun 21, 2024

Migration: Remove user data from couch model #34798

Merged

3 tasks

esoergel changed the title ~~Es/user data prep~~ Remove user data from couch user model Jun 21, 2024

Fix related doc lookups for user data

5081177

These have been broken since the migration to SQL, apparently

jingcheng16 self-requested a review June 24, 2024 18:04

Remove db requirement for champ UCR tests

ca7845f

esoergel marked this pull request as ready for review June 24, 2024 20:47

esoergel requested review from calellowitz and stephherbers as code owners June 24, 2024 20:47

esoergel requested review from AddisonDunn and Jtang-1 June 24, 2024 20:47

esoergel added 2 commits June 24, 2024 17:20

Remove old, unused user_data field

ab0ed8a

Make body of migration run only once

a181308

esoergel force-pushed the es/user-data-prep branch from 77ba5e1 to a181308 Compare June 24, 2024 21:25

Jtang-1 reviewed Jun 24, 2024

View reviewed changes

MartinRiese reviewed Jun 26, 2024

View reviewed changes

AddisonDunn reviewed Jun 27, 2024

View reviewed changes

update help text

da40b4d

AddisonDunn approved these changes Jun 27, 2024

View reviewed changes

esoergel merged commit 9d20392 into master Jun 27, 2024
13 checks passed

esoergel deleted the es/user-data-prep branch June 27, 2024 21:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove user data from couch user model #34805

Remove user data from couch user model #34805

esoergel commented Jun 21, 2024 •

edited

Loading

esoergel Jun 21, 2024

zandre-eng Jul 1, 2024

Jtang-1 left a comment

MartinRiese Jun 26, 2024

esoergel Jun 26, 2024

MartinRiese Jun 27, 2024

esoergel Jun 27, 2024

MartinRiese Jun 27, 2024

MartinRiese Jun 26, 2024

AddisonDunn Jun 26, 2024

esoergel Jun 27, 2024

AddisonDunn Jun 27, 2024

AddisonDunn Jun 26, 2024

esoergel Jun 27, 2024

AddisonDunn Jun 26, 2024

esoergel Jun 27, 2024

AddisonDunn Jun 27, 2024

AddisonDunn Jun 27, 2024

AddisonDunn Jun 27, 2024

esoergel Jun 27, 2024

esoergel Jun 27, 2024

AddisonDunn Jun 27, 2024

esoergel Jun 27, 2024

AddisonDunn Jun 27, 2024

AddisonDunn Jun 27, 2024 •

edited

Loading

esoergel Jun 27, 2024

esoergel commented Jun 27, 2024

esoergel commented Jul 16, 2024

	def _update_user_data(user, new_user_data, user_change_logger):
	try:
	profile_id = new_user_data.pop(PROFILE_SLUG, ...)
	changed = user.get_user_data(user.domain).update(new_user_data, profile_id=profile_id)
	except UserDataError as e:
	raise UpdateUserException(str(e))

	if user_change_logger and changed:
	user_change_logger.add_changes({
	'user_data': user.get_user_data(user.domain).raw
	})



		class Command(BaseCommand):
		help = "Populate SQL user data from couch"

Remove user data from couch user model #34805

Remove user data from couch user model #34805

Conversation

esoergel commented Jun 21, 2024 • edited Loading

Product Description

Technical Summary

Feature Flag

Safety Assurance

Safety story

Automated test coverage

QA Plan

Migrations

Rollback instructions

Labels & Review

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jtang-1 left a 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

AddisonDunn Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esoergel commented Jun 27, 2024

esoergel commented Jul 16, 2024

esoergel commented Jun 21, 2024 •

edited

Loading

AddisonDunn Jun 27, 2024 •

edited

Loading