Page MenuHomePhabricator
Create Task
Maniphest T359149

Wikibase apitests broken in CI: Unsupported Content-Type: application/json-patch+json
Closed, ResolvedPublic
Actions

Description

Example build:

  1) PATCH statement tests
       PATCH /entities/items/{item_id}/statements/{statement_id}
         200 success response
           allows content-type application/json-patch+json:

      AssertionError: 
7Response body: {
  errorKey: 'rest-unsupported-content-type',
  messageTranslations: { en: 'Unsupported Content-Type: application/json-patch+json' },
  httpCode: 415,
  httpReason: 'Unsupported Media Type'
}8
Invalid status: expected 415 to equal 200      
      actual expected
      
      415200
      
      at module.exports.expect.expect (tests/mocha/helpers/chaiHelper.js:26:27)
      at Proxy.chainableMethodWrapper (node_modules/chai/lib/chai/utils/addChainableMethod.js:113:49)
      at assertValid200Response (tests/mocha/api-testing/PatchItemStatementTest.js:71:32)
      at Context.<anonymous> (tests/mocha/api-testing/PatchItemStatementTest.js:123:6)
      at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

  2) PATCH statement tests
       PATCH /statements/{statement_id}
         200 success response
           allows content-type application/json-patch+json:

      AssertionError: 
7Response body: {
  errorKey: 'rest-unsupported-content-type',
  messageTranslations: { en: 'Unsupported Content-Type: application/json-patch+json' },
  httpCode: 415,
  httpReason: 'Unsupported Media Type'
}8
Invalid status: expected 415 to equal 200      
      actual expected
      
      415200
      
      at module.exports.expect.expect (tests/mocha/helpers/chaiHelper.js:26:27)
      at Proxy.chainableMethodWrapper (node_modules/chai/lib/chai/utils/addChainableMethod.js:113:49)
      at assertValid200Response (tests/mocha/api-testing/PatchItemStatementTest.js:71:32)
      at Context.<anonymous> (tests/mocha/api-testing/PatchItemStatementTest.js:123:6)
      at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

  3) PATCH property statement
       PATCH /statements/{statement_id}
         200 success response
           allows content-type application/json-patch+json:

      AssertionError: 
7Response body: {
  errorKey: 'rest-unsupported-content-type',
  messageTranslations: { en: 'Unsupported Content-Type: application/json-patch+json' },
  httpCode: 415,
  httpReason: 'Unsupported Media Type'
}8
Invalid status: expected 415 to equal 200      
      actual expected
      
      415200
      
      at module.exports.expect.expect (tests/mocha/helpers/chaiHelper.js:26:27)
      at Proxy.chainableMethodWrapper (node_modules/chai/lib/chai/utils/addChainableMethod.js:113:49)
      at assertValid200Response (tests/mocha/api-testing/PatchPropertyStatementTest.js:68:32)
      at Context.<anonymous> (tests/mocha/api-testing/PatchPropertyStatementTest.js:120:6)
      at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

  4) PATCH property statement
       PATCH /entities/properties/{property_id}/statements/{statement_id}
         200 success response
           allows content-type application/json-patch+json:

      AssertionError: 
7Response body: {
  errorKey: 'rest-unsupported-content-type',
  messageTranslations: { en: 'Unsupported Content-Type: application/json-patch+json' },
  httpCode: 415,
  httpReason: 'Unsupported Media Type'
}8
Invalid status: expected 415 to equal 200      
      actual expected
      
      415200
      
      at module.exports.expect.expect (tests/mocha/helpers/chaiHelper.js:26:27)
      at Proxy.chainableMethodWrapper (node_modules/chai/lib/chai/utils/addChainableMethod.js:113:49)
      at assertValid200Response (tests/mocha/api-testing/PatchPropertyStatementTest.js:68:32)
      at Context.<anonymous> (tests/mocha/api-testing/PatchPropertyStatementTest.js:120:6)
      at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

According to @Jakob_WMDE, this broke with the core change Rest router should provide parsed body data to handler (T358557), though it’s not yet clear why.

EventBus tests were also broken, see https://gerrit.wikimedia.org/r/c/mediawiki/extensions/EventBus/+/1008562

Details

Related Changes in Gerrit:
SubjectAuthorRepoBranchLines +/-
Rest: allow Handlers to disable body parsing.Daniel Kinzlermediawiki/coremaster+114 -0
Revert "REST: Temporarily skip tests involving unusual content types"Daniel Kinzlermediawiki/extensions/Wikibasemaster+29 -58
REST: Temporarily skip tests involving unusual content typesJakobmediawiki/extensions/Wikibasemaster+58 -29
Rest: allow Handlers to disable body parsing.Daniel Kinzlermediawiki/corewmf/1.42.0-wmf.21+114 -0
Customize query in gerrit

Related Objects

Event Timeline

Lucas_Werkmeister_WMDE created this task.Mar 5 2024, 10:00 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 5 2024, 10:00 AM
Jakob_WMDE updated the task description. (Show Details)Mar 5 2024, 10:01 AM
Lucas_Werkmeister_WMDE updated the task description. (Show Details)Mar 5 2024, 10:01 AM
gerritbot added a comment.Mar 5 2024, 10:06 AM

Change 1008812 had a related patch set uploaded (by Jakob; author: Jakob):

[mediawiki/extensions/Wikibase@master] REST: Temporarily skip tests involving unusual content types

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

gerritbot added a project: Patch-For-Review.Mar 5 2024, 10:06 AM
Lucas_Werkmeister_WMDE added a comment.Mar 5 2024, 10:26 AM

Note: Though the error message looks identical, this doesn’t come from Wikibase’s ContentTypeCheckMiddleware; with a debugger, I could instead see that it comes from \MediaWiki\Rest\Handler::parseBodyData():

	/**
	 * @throws HttpException on failed check
	 */
	public function parseBodyData( RequestInterface $request ): ?array {
		// Parse the body based on its content type
		$contentType = $request->getBodyType();
		switch ( $contentType ) {
			case 'application/x-www-form-urlencoded':
			case 'multipart/form-data':
				return $request->getPostParams();
			case 'application/json':
				$jsonStream = $request->getBody();
				$parsedBody = json_decode( "$jsonStream", true );
				if ( !is_array( $parsedBody ) ) {
					throw new LocalizedHttpException(
					// Fixme: missing parameter
						new MessageValue( 'rest-json-body-parse-error', [ "" ] ),
						400
					);
				}
				return $parsedBody;
			default:
				throw new LocalizedHttpException( // <== HERE
					new MessageValue( 'rest-unsupported-content-type', [ $contentType ?? '(null)' ] ),
					415
				);
		}
	}
Lucas_Werkmeister_WMDE added a comment.Mar 5 2024, 10:48 AM

This hack makes the PATCH /entities/items/{item_id}/statements/{statement_id} test work again:

diff --git a/repo/rest-api/src/RouteHandlers/PatchItemStatementRouteHandler.php b/repo/rest-api/src/RouteHandlers/PatchItemStatementRouteHandler.php
index 76cfe52a94..615717b413 100644
--- a/repo/rest-api/src/RouteHandlers/PatchItemStatementRouteHandler.php
+++ b/repo/rest-api/src/RouteHandlers/PatchItemStatementRouteHandler.php
@@ -4,6 +4,7 @@
 
 use MediaWiki\MediaWikiServices;
 use MediaWiki\Rest\Handler;
+use MediaWiki\Rest\LocalizedHttpException;
 use MediaWiki\Rest\RequestInterface;
 use MediaWiki\Rest\Response;
 use MediaWiki\Rest\ResponseInterface;
@@ -22,6 +23,7 @@
 use Wikibase\Repo\RestApi\RouteHandlers\Middleware\MiddlewareHandler;
 use Wikibase\Repo\RestApi\RouteHandlers\Middleware\UserAgentCheckMiddleware;
 use Wikibase\Repo\RestApi\WbRestApi;
+use Wikimedia\Message\MessageValue;
 use Wikimedia\ParamValidator\ParamValidator;
 
 /**
@@ -137,6 +139,23 @@ public function getParamSettings(): array {
 		];
 	}
 
+	public function parseBodyData( RequestInterface $request ): ?array {
+		if ( $request->getBodyType() === ContentTypeCheckMiddleware::TYPE_JSON_PATCH ) {
+			// copy+paste from the standard JSON case in the parent method
+			$jsonStream = $request->getBody();
+			$parsedBody = json_decode( "$jsonStream", true );
+			if ( !is_array( $parsedBody ) ) {
+				throw new LocalizedHttpException(
+				// Fixme: missing parameter
+					new MessageValue( 'rest-json-body-parse-error', [ "" ] ),
+					400
+				);
+			}
+			return $parsedBody;
+		}
+		return parent::parseBodyData( $request );
+	}
+
 	/**
 	 * @inheritDoc
 	 */

But I’m not sure if it was intended in T358557 that all handlers should now have to override parseBodyData() if they want to support content types other than application/x-www-form-urlencoded, multipart/form-data and application/json.

Also, this doesn’t seem to be a test problem, it’ll actually break JSON PATCH requests in production when this rolls out with the train. So this should probably be a train blocker?

Lucas_Werkmeister_WMDE mentioned this in T358557: Rest router should provide parsed body data to handler..Mar 5 2024, 10:52 AM
gerritbot added a comment.Mar 5 2024, 10:52 AM

Change 1008812 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] REST: Temporarily skip tests involving unusual content types

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

ReleaseTaggerBot added a project: MW-1.42-notes (1.42.0-wmf.22; 2024-03-12).Mar 5 2024, 11:00 AM
Maintenance_bot removed a project: Patch-For-Review.Mar 5 2024, 11:30 AM
daniel subscribed.Mar 5 2024, 12:15 PM

Working on a fix

gerritbot added a comment.Mar 5 2024, 12:26 PM

Change 1008419 had a related patch set uploaded (by Daniel Kinzler; author: Daniel Kinzler):

[mediawiki/core@master] Rest: allow Handlers to disable body parsing.

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

gerritbot added a project: Patch-For-Review.Mar 5 2024, 12:26 PM
daniel mentioned this in T354439: 1.42.0-wmf.21 deployment blockers.Mar 5 2024, 12:35 PM
Jdforrester-WMF triaged this task as Unbreak Now! priority.Mar 5 2024, 1:43 PM
Jdforrester-WMF added a parent task: T354439: 1.42.0-wmf.21 deployment blockers.
Jdforrester-WMF subscribed.

UBN as a (potential) train-blocker.

Jakob_WMDE added a subscriber: Ifrahkhanyaree_WMDE.Mar 5 2024, 2:41 PM

I don't think this needs to be a train blocker. We just agreed in a meeting with our PM (cc @Ifrahkhanyaree_WMDE) that we're fine with it being broken for a week given this is all still considered experimental. Patch requests using application/json as the content type still work, only application/json-patch+json doesn't.

Lucas_Werkmeister_WMDE lowered the priority of this task from Unbreak Now! to High.Mar 5 2024, 3:25 PM
Lucas_Werkmeister_WMDE removed a parent task: T354439: 1.42.0-wmf.21 deployment blockers.

Alright, tentatively lowering to High and removing parent task then.

gerritbot added a comment.Mar 5 2024, 4:41 PM

Change 1008758 had a related patch set uploaded (by Daniel Kinzler; author: Daniel Kinzler):

[mediawiki/core@wmf/1.42.0-wmf.21] Rest: allow Handlers to disable body parsing.

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

gerritbot added a comment.Mar 5 2024, 4:44 PM

Change 1008419 merged by jenkins-bot:

[mediawiki/core@master] Rest: allow Handlers to disable body parsing.

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

gerritbot added a comment.Mar 5 2024, 5:35 PM

Change 1008758 abandoned by Daniel Kinzler:

[mediawiki/core@wmf/1.42.0-wmf.21] Rest: allow Handlers to disable body parsing.

Reason:

This can probably wait for next week's train, no need for a backport

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

Maintenance_bot removed a project: Patch-For-Review.Mar 5 2024, 6:31 PM
gerritbot added a comment.Mar 5 2024, 7:13 PM

Change 1008763 had a related patch set uploaded (by Daniel Kinzler; author: Daniel Kinzler):

[mediawiki/extensions/Wikibase@master] Revert "REST: Temporarily skip tests involving unusual content types"

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

gerritbot added a project: Patch-For-Review.Mar 5 2024, 7:13 PM
Lucas_Werkmeister_WMDE mentioned this in T359403: Wikibase apitests broken in CI: Invalid status: expected 500 to be within 200..299.Mar 6 2024, 3:00 PM
gerritbot added a comment.Mar 12 2024, 1:16 PM

Change 1008763 abandoned by Jakob:

[mediawiki/extensions/Wikibase@master] Revert "REST: Temporarily skip tests involving unusual content types"

Reason:

All the tests here have been re-enabled or deleted in the meantime, so we don't need this patch anymore. I540d25e3b997e791a40f8ba68712e33f5f662fc1 re-enabled some, Id76125170ae6b2c6f32b926c96fe25d4ae804177 deleted the rest because they are no longer needed.

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

Maintenance_bot removed a project: Patch-For-Review.Mar 12 2024, 1:30 PM
daniel added a project: MediaWiki-API-Platform-Team.Mar 19 2024, 2:04 PM
daniel moved this task from Incoming (Needs Triage) to Done on the MediaWiki-API-Platform-Team board.

I think this should be fixed now, can you confirm?

Jakob_WMDE closed this task as Resolved.Mar 19 2024, 2:31 PM

Yes, thanks!

Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits