Get SpaceDock co-authors from POST form lists

[HebaruSan]

Problem

See #293 and #294; #287 doesn't work. Instead of the shared_authors property containing the co-authors, we just receive username, so attempting to add the co-authors throws an exception.

Cause

You can't send a list of dicts in a POST form. I thought it was JSON.

Changes

Someday soon, SpaceDock will start sending multiple values for the user fields, one per author (see KSP-SpaceDock/SpaceDock#490). This is supported in POST forms. Once we have that, this pull request will read those lists to get the co-authors.

Note that KSP-SpaceDock/SpaceDock#490 must be deployed to production before this is merged! Currently there is no specific plan for the next upgrade, but SpaceDock updates have been somewhat frequent lately, so it shouldn't take too long.

[techman83]

Do we have a sample spacedock payload for this? I wouldn't mind constructing a test case. I did some looking and it's noteable the list ends up empty if a key is missing

[{'username':x[0], 'user_github': x[1]} for x in zip(raw.getlist('username'), raw.getlist('user_github'))]
[]

vs

[{'username':x[0]} for x in zip(raw.getlist('username'))]
[{'username': 'modauthor1'}, {'username': 'another_user'}]

What's the complexity involved in using JSON instead? I must admit that I was tripped up by it being a forms based endpoint.

[techman83]

Are all these keys going to be present if there are multiple authors?

[HebaruSan]

That's the plan, yeah. They're already sent unconditionally, regardless of if there is one author or twelve, and KSP-SpaceDock/SpaceDock#490 is just making them contain lists:

https://github.com/KSP-SpaceDock/SpaceDock/pull/490/files

[techman83]

I've crafted a test for this, intended destination tests.webhooks.TestWebhookSpaceDockAdd

    @mock.patch('netkan.webhooks.config.WebhooksConfig.client')
    def test_multi_authors_forms(self, queued: MagicMock):
        data = self.mock_netkan_hook()
        data.update({
            'username': ['author1', 'author2'],
            'user_github': ['author1_gh']
        })
        response = self.client.post('/sd/add/ksp', data=data)
        self.assertEqual(response.status_code, 204)
        call = queued.method_calls.pop().call_list().pop()
        body = json.loads(call[2].get('Entries')[0].get('MessageBody'))
        self.assertListEqual(
            body.get('all_authors'),
            [{'username': 'author1', 'user_github': 'author1_gh',
                'email': 'modauthor1@gmail.com'}, {'username': 'author2'}]
        )

Which fails:

AssertionError: Lists differ: [] != [{'username': 'author1', 'user_github': 'author1_gh'}, {'username': 'author2'}]

Second list contains 2 additional elements.
First extra element 0:
{'username': 'author1', 'user_github': 'author1_gh'}

- []
+ [{'user_github': 'author1_gh', 'username': 'author1'}, {'username': 'author2'}]

The problem is the zip, it'll return a tuple with a length equal to the item with the least items. Which will be fine if the lists come through in equal lengths, regardless of if an author is missing a github or something.

If the payload comes through more like this, the lists will all be equal

('user', 'author1'), ('user', 'author2'),('user_github', 'author3')
('user_github', 'author1_gh'), ('user_github', ''),('user_github', 'author3_gh')
('email', 'author1@email'), ('email', ''),('email': '')
('user_forum_id', ''), ('user_forum_id', ''),(user_forum_id': '')
('user_forum_username', ''), ('user_forum_username', ''),('user_forum_username': '')

[HebaruSan]

The main thing that makes me hesitate to switch to JSON is that I don't know how to do that transition without some downtime of the link (and having to rewrite stuff). If we change SpaceDock first, then Infra would start receiving JSON and not know what to do about it; if we change Infra first, then it would break until SpaceDock started sending JSON.

If we stick with POST forms, we can safely change SpaceDock first, since request.form.get still works for a list (it just returns the first element, which does exactly what we have today).

This stuff is documented to be a POST form, I just failed to read the comment:

NetKAN-Infra/netkan/netkan/webhooks/spacedock_add.py

Lines 19 to 35 in 38aa44b

	# For mod creation hook on SpaceDock, creates pull requests
	# Handles: https://netkan.ksp-ckan.space/sd/add
	# POST form parameters:
	# name: Mod Name Entered by the User on spacedock
	# id: 12345
	# license: GPL-3.0
	# username: modauthor1
	# email: modauthor1@gmail.com
	# short_description: A mod that you should definitely install
	# description: A mod that you should definitely install, and so on and so on
	# external_link: https://forum.kerbalspaceprogram.com/index.php?/topic/999999-ThreadTitle
	# source_link: https://github.com/user/repo
	# user_url: https://spacedock.info/profile/ModAuthor1
	# mod_url: https://spacedock.info/mod/12345
	# site_name: SpaceDock
	@spacedock_add.route('/add/<game_id>', methods=['POST'])
	def add_hook(game_id: str) -> Tuple[str, int]:

[techman83]

The main thing that makes me hesitate to switch to JSON is that I don't know how to do that transition without some downtime of the link (and having to rewrite stuff). If we change SpaceDock first, then Infra would start receiving JSON and not know what to do about it; if we change Infra first, then it would break until SpaceDock started sending JSON.

We'd inspect the content type. Requests will do the RightThing ™️ and set it to application/x-www-form-urlencoded or application/json (ie requests.post('http://httpbin.org/post', json={"key": "value"}))

This stuff is documented to be a POST form, I just failed to read the comment:

Oh I read and understood the structure. But I'm not super experienced with forms + arrays. Without an expected payload it's kinda tricky to be certain I'm testing the same thing. It's doesn't cause a critical failure if there were a problem, just all authors would end up empty.

[HebaruSan]

We have a prior example of SpaceDock↔CKAN communication with POST form lists, but in the other direction, in the batched download counts (see #228 and KSP-SpaceDock/SpaceDock#401). CKAN sends a list and SpaceDock receives it with request.form.getlist. That's why I'm confident that this approach is OK.

[techman83]

Oh I think the approach is ok, just the structure is a bit janky to work with IMO. And if we're making changes, JSON would be nicer. I'll chip away at a test case and see if I'm misunderstanding something.

[techman83]

I'd love a test case, but I'll take your word on the source data. Just took me a bit to understand what it was doing. There would be nothing stopping us from switching to JSON in the future if desired.