Null values in assoc columns in user_settings prevent correct data reading after update

[Vitaliy-1]

Describe the bug
The bug described on the forum: https://forum.pkp.sfu.ca/t/null-values-in-user-settings-safe-to-remove/68272
Old installations may have NULL values in assoc_type and assoc_id columns in user_settings table. This leads to duplicated rows when this data is updated, e.g. through the user profile form:

user_id	locale	setting_name	assoc_type	assoc_id	setting_value	setting_type	
4	en_US	givenName	NULL            NULL            Doe	        string	
4	en_US	givenName	0	        0	        NewDoe	        string	

According to the report, this leads to problems in reading user data, the one that I can reproduce appears in Add Reviewer form: searching for a reviewer gives duplicated entries of this particular user in example:

Also, during data retrieval, old setting value, e.g. Doe may be displayed instead of NewDoe. This one I wasn't able to reproduce

To Reproduce
At glance, this problem affects old installs and I'm unable to reproduce it on a clean instance. I can confirm that my old test instance upgraded from 3.0 is affected.

What application are you using?
OJS 3.3

Additional information
I think to solve the issue the best would be to add an upgrade script that removes duplicates (those with 0 are newer and should remain intact while rows with NULL should be removed) or replaces NULL values with the default 0s if the row isn't duplicated.
Associated issue: #7146

[jonasraoni]

Just adding notes from my other issue:

Identification
The rows returned by the query below are duplicated

SELECT s.user_id, s.setting_name, s.locale
FROM user_settings s
GROUP BY user_id, setting_name, locale
HAVING COUNT(0) > 1

---

Variations
The apiKey and apiKeyEnabled are also duplicated, but they follow a slightly different pattern, the older records have assoc_type = 256 and assoc_id = null

---

Possible source
Perhaps the problem was introduced here: https://github.com/pkp/pkp-lib/pull/6347/files#diff-f4951b8d90b205bf60a1111a761b62eb75f598c0c536738da01d027561a33f1cR74-R75

---

Solution
As far as I could see from my observations, there's no risk to discard the nulled data once it becomes duplicated, so I think we're safe to provide a solution out-of-the-box with an update script.

• Decide whether it makes sense to store 0 or null into these fields (without inspecting the code, I'd go with null).
• Upgrade the codebase to follow the right pattern when updating the user_settings table (self-note: check the setting_type).
• Inspect how these fields are used, perhaps it makes sense to add extra checks (e.g. AND assoc_type IS NULL AND assoc_id IS NULL) or to remove them altogether, in case they are not used at all.
• Write upgrade script to normalize the data into the right pattern and discard the duplicated records.
• Users that fixed the data by themselves should be safe to run the upgrade.

[jonasraoni]

@asmecher before I touch this, follow some questions:

1. I just checked the code and I'm inclined to remove the assoc_type and assoc_id from the table user_settings.

• We're not using them, and they've been causing issues
• Parts of the code are already ignoring these fields
• It's not possible to setup a foreign key on such dynamic relation
• In case we really need it somewhere in the future, we can structure it better or use an alternative way

2. I guess such modifications are welcome just for 3.4.0, right?

[asmecher]

@jonasraoni, yes, the user_settings table's assoc_id and assoc_type columns can be removed as unused. This used to allow user settings to be made context-specific, but that feature was never used. (If we need to re-add a feature like that, then a more specific context_id column would be a better idea -- but let's not go there unless we need to.)

3.4.0 (main) would be a good time for this.

[jonasraoni]

Just for reference, in case someone land here, here's a query to fix the issue while I didn't start working on this:
#7414 (comment)

[jonasraoni]

Looks like the assoc_id/assoc_type usage was already wiped off, so I just created the migration to fix the duplicated items and remove the columns.
I didn't test yet the migration (just offloading the job for Travis), so maybe it will need some extra work later.

PRs for /main:
pkp/ojs#3230
pkp/omp#1032
pkp/ops#200
#7464

[jonasraoni]

Summary:

• 3.3.0: The issue was introduced in this version and fixes to avoid creating new cases were merged. But the database will remain with duplicated records, technically skilled users might apply a manual fix (references: pkp/pkp-lib#6991 Improve user search query #7414 (comment) and https://github.com/pkp/pkp-lib/blob/main/classes/migration/upgrade/v3_4_0/I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php#L37-L92).

• 3.4.0: The fields which caused the issue are removed. The upgrade script will take care of cleaning-up the duplicated data.

[jonasraoni]

Reopening due to this error I found in one of the upgrade tests:

https://app.travis-ci.com/github/pkp/ojs/jobs/556609009
ERROR: Upgrade failed: DB: SQLSTATE[2BP01]: Dependent objects still exist: 7 ERROR: cannot drop index user_settings_pkey because constraint user_settings_pkey on table user_settings requires it
HINT: You can drop constraint user_settings_pkey on table user_settings instead. (SQL: drop index "user_settings_pkey")

[jonasraoni]

Done! Here's the explanation/cause: #7670