Additional validation on updating social fields via API v2 [PLAT-925] #8551

erinspace · 2018-07-17T01:43:15Z

Purpose

One could update social fields via API v2 after the social fields were added. This adds additional validation using jsonschema, and ~~allows fields that aren't currently allowed for in the model to be sent and ignored.~~

This updates the format a bit, migrates old data to match that format, and makes sure that payloads to set the data conform to that spec.

Changes

add jsonschema document for the social fields
use this to validate data being sent to the API
add a validate method on the serializer to process the data
Social fields that can be lists are now stored as lists, and those that are singular are stored as strings

QA Notes

This would only require API testing, and shouldn't be very different from what exists already anyhow! Should be all good, and will be tested by Embosf when they use this feature for the social page!

Documentation

CenterForOpenScience/developer.osf.io#23

Side Effects

None anticipated

Ticket

https://openscience.atlassian.net/browse/PLAT-925

pattisdr

awesome @erinspace

pattisdr · 2018-07-20T17:01:13Z

api/users/schemas.py

@@ -0,0 +1,108 @@
+# -*- coding: utf-8 -*-
+
+def get_user_social_jsonschema():


Nice schema 👍

pattisdr · 2018-07-20T17:05:15Z

api/users/serializers.py

@@ -128,20 +130,31 @@ def profile_image_url(self, user):
        size = self.context['request'].query_params.get('profile_image_size')
        return user.profile_image_url(size=size)

+    def validate_social(self, value):


I forgot you could do this! .validate_<field_name>

caseyrollins · 2018-07-23T16:13:05Z

api/users/schemas.py

@@ -0,0 +1,108 @@
+# -*- coding: utf-8 -*-
+
+def get_user_social_jsonschema():


@erinspace, @Johnetordoff's PR adds validation schemas for employment and education information and implements a from_json method for loading the correct schema. Would you mind mimicking that approach in this PR?

caseyrollins · 2018-07-26T18:28:19Z

api/users/serializers.py

+
+        social_dict = {}
+        for key, val in value.iteritems():
+            # Ignore fields that are not specified in the current social fields list


Drive-by: this shouldn't be necessary if you add

"additionalProperties": false

to the json schema (although still a good idea to have tests for it).

Just for history's sake, when I wrote this the requirement was "Ignore attempts to update social fields that are not in [the list of fields]" which is what this was doing. Requirement has changed so will now error with extra fields instead of ignoring.

caseyrollins

Looks good to me; passing off to @sloria to review the data migration. Would you mind updating the docstring around the social field in users.py so devs will know the (new) correct structure?

sloria · 2018-08-01T20:00:00Z

@erinspace Can you get this up to date with develop and repoint the PR pls?

erinspace · 2018-08-01T21:04:56Z

@sloria, up-to-dated and re-pointed!

sloria

Pass finished.

sloria · 2018-08-03T14:48:39Z

api/users/social-schema.json

@@ -0,0 +1,73 @@
+{


Schemas are now located in api/users/schemas.

sloria · 2018-08-03T14:57:37Z

osf/migrations/0112_update_social_data_format.py

+
+    users_updated = 0
+    for user in users_with_social:
+        old_social = {}


Nit: Isn't this "new_social"?

Naw, cause it's in the reset_social_fields method, so it's putting it back to the old social field!

sloria · 2018-08-03T14:59:23Z

osf/migrations/0112_update_social_data_format.py

+    for user in users_with_social:
+        old_social = {}
+        for key, value in user.social.iteritems():
+            if key in SOCIAL_LIST_FIELDS and key != 'profileWebsites':


Just curious: why include SOCIAL_LIST_FIELDS in the first place? Is it because profileWebsites is a social list field and you wanted to be complete? Alternatively, you could rename the constant to FIELDS_TO_MIGRATE and omit profileWebsites. Not a big deal, though.

sloria · 2018-08-03T15:13:41Z

osf/migrations/0112_update_social_data_format.py

+        old_social = {}
+        for key, value in user.social.iteritems():
+            if key in SOCIAL_LIST_FIELDS and key != 'profileWebsites':
+                    old_social[key] = value[0]


The migration should error if it finds a value that contains more than one item, so we don't accidentally remove data.

sloria · 2018-08-03T15:16:30Z

api/users/schemas.py

+
+here = os.path.split(os.path.abspath(__file__))[0]
+
+def from_json(fname):


This file was moved to a directory. I'm getting an ImportError when initializing the app. I think this file just needs to be removed.

sloria · 2018-08-03T15:16:57Z

api/users/serializers.py

    WaterbutlerLink, ShowIfCurrentUser
 )
 from api.base.utils import absolute_reverse, get_user_auth, waterbutler_api_url_for
 from api.files.serializers import QuickFilesSerializer
 from osf.exceptions import ValidationValueError, ValidationError
 from osf.models import OSFUser, QuickFilesNode
+from api.users.schemas import from_json


should be api.users.schemas.utils.

sloria

Pass finished. Mostly looked at the migration.

There is a significant number (> 10K) of users that have the github, linkedIn, or twitter keys in social. Therefore, the migration will need to be optimized in order to prevent downtime.

Look into doing an update in order to bulk-update (using the bulk update strategy we've used in the past).

Turns out Django has built-in support for querying on JSON fields, via KeyTextTransform

from django.contrib.postgres.fields.jsonb import KeyTextTransform

(
    OSFUser.objects.filter(social__has_any_keys=['twitter'])
    .annotate(twitter=KeyTextTransform('twitter', 'social'))
    .annotate(twitter_length=Length('twitter'))...
)

This should enable you to update the users in one or a few queries.

sloria · 2018-08-20T14:21:41Z