Page MenuHomePhabricator

Cannot store array properties
Closed, ResolvedPublicBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

  • Define a "provider/validator":
"Mentorship": {
					"store": {
						"type": "wikipage",
						"args": [
							"MediaWiki:GrowthExperimentsMentorship.json"
						]
					},
					"validator": {
						"type": "jsonschema",
						"args": [
							"CommunityConfigurationExample/src/Schemas/schema_growth-experiments_mentorship.json"
						]
					},
					"type": "mw-config"
				}

CommunityConfigurationExample/src/Schemas/schema_growth-experiments_mentorship.json

{
	"$schema": "https://json-schema.org/draft-07/schema#",
	"$id": "/communityconfigurationexample/newcomershomepage/1.0.0",
	"type": "object",
	"properties": {
		"GEHomepageSuggestedEditsIntroLinks": {
			"type": "object",
			"properties": {
				"createIntroLink":{
					"type": "string"
				},
				"imageIntroLink":{
					"type": "string"
				}
			},
			"additionalProperties": false
		},
		"TestPropPageTitles": {
			"type": "array",
			"items": {
				"type": "string"
			}
		}
	},
	"required": [
		"GEHomepageSuggestedEditsIntroLinks"
	],
	"additionalProperties": false
}
  • Try to store the following data in MediaWiki:GrowthExperimentsMentorship.json
{
	"GEHomepageSuggestedEditsIntroLinks": {
		"createIntroLink": "ABBA",
		"imageIntroLink": "Help:CS1 errors"
	},
    "TestPropPageTitles": [ "Page 1", "Page 2" ]
}

What happens?:
An error is shown Object value found, but an array is required. Key: TestPropPageTitles

What should have happened instead?:
The data should be considered valid and stored.

Software version (skip for WMF-hosted wikis like Wikipedia):

master and main branches of CommunityConfiguration and CommunityConfigurationExample respectively

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Sgs updated the task description. (Show Details)

Change 1009746 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[mediawiki/extensions/CommunityConfiguration@master] JsonSchemaValidator: Only convert associative arrays recursively

https://gerrit.wikimedia.org/r/1009746

Change 1009747 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[mediawiki/extensions/CommunityConfiguration@master] JsonSchemaValidator: Use JSON serde for converting associative array to stdClass

https://gerrit.wikimedia.org/r/1009747

Thanks for filling the task @Sgs. This is caused by JsonSchemaValidator::arrayToStdClass treating all arrays the same (and converting them to stdClass), while we only want to convert associative arrays into stdClass, and leave indexed arrays as-is. I have three possible options how to fix this:

  1. Fixing JsonSchemaValidator::arrayToStdClass to only convert associative arrays: We can make the converting function only convert non-associative arrays. That way, the JSON validation library would see any "type": "array" values as actual arrays and not objects.
  2. Do not convert associative arrays into stdClass ourself: Instead of writing the code for converting an associative array into stdClass ourselves, we can let ext-json do that for us. By calling FormatJson::decode( FormatJson::encode( $array ) ), we will have a stdClass, which follows JSON conventions. We can replace the whole arrayToStdClass function with this single line, and it will work (and fix the bug @Sgs reported here).
  3. Use stdClass as the canonical representation across CommunityConfiguration: Currently, an associative array is the main representation of a configuration store content. We can switch this to a stdClass instead (using FormatJson::decode( ... ) instead of FormatJson::decode( ..., true )).

I implemented option 1 as https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CommunityConfiguration/+/1009746 and option 2 as https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CommunityConfiguration/+/1009747, so others can take a look at the options in more detail and advise which one look better. I did not implement option 3, as it is considerably wider. From my perspective:

  1. It is fairly easy to see if an array is associative or not. This information can be easily reused in the converting. However, it makes the code more complex, and it might contain similar issues that we notice only after we try saving something.
  2. JSON serdes such as ext-json in PHP is used in so many places (in Wikimedia, but also elsewhere), and it is fair to assume they're working well. Even though converting associative arrays to stdClasses are not their intended purpose, it is fair to assume a serialization followed by a deserialization will continue working as intended (if it didn't, it'd imply a bug in the deserialization logic of the library). This means we can possibly fix more bugs than this particular one by option 2. However, invoking the whole serde library two times is likely to be significantly less performant.
  3. This will mean a lot of changes across all of CC. We might find ourselves in the need of the opposite direction (which should be arguably easier), but maybe not even that (WikiPageConfigProvider::get can be rewritten to work assuming config is a stdClass).

I think option 3 means a lot of changes (while the other two options are self-contained to the validator class). However, now is likely the only time in which we can afford to make such a change (as it changes the interface between client extensions and us considerably), so if we feel that is the best solution, we should just go ahead and do it, as doing that later will be next-to-impossible.

@Sgs Do you have any thoughts on the three options?

@Sgs Do you have any thoughts on the three options?

I’m leaning towards (1).
(2) Not sure about ext-json PHP implementation but generally serdes are costly so using on each validation call seems not a good idea.
(3) I don’t see a clear advantage of using stdClass as the config type everywhere. Do you?

I agree with your conclusions. Let's go with (1) then. Thanks!

Change 1009747 abandoned by Urbanecm:

[mediawiki/extensions/CommunityConfiguration@master] JsonSchemaValidator: Use JSON serde for converting associative array to stdClass

Reason:

in favour of https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CommunityConfiguration/+/1009746

https://gerrit.wikimedia.org/r/1009747

Change 1009746 merged by jenkins-bot:

[mediawiki/extensions/CommunityConfiguration@master] JsonSchemaValidator: Only convert associative arrays recursively

https://gerrit.wikimedia.org/r/1009746

FYI @Etonkovidova, because T358799 got merged, this will likely need a PHP version of the same schema instead.

FYI @Etonkovidova, because T358799 got merged, this will likely need a PHP version of the same schema instead.

Thx, @Urbanecm_WMF - checked locally as per the task description, all seems to be working as expected.