Skip to content

Commit ccd290c

Browse files
jenkins-botGerrit Code Review
jenkins-bot
authored and
Gerrit Code Review
committed
Merge "REST: make ETag computation lazy"
2 parents 8814435 + fa81737 commit ccd290c

File tree

3 files changed

+162
-54
lines changed

3 files changed

+162
-54
lines changed

‎includes/Rest/ConditionalHeaderUtil.php

Lines changed: 112 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -10,35 +10,45 @@
1010
class ConditionalHeaderUtil {
1111
/** @var bool */
1212
private $varnishETagHack = true;
13-
/** @var string|null */
13+
/** @var callback|string|null */
1414
private $eTag;
15-
/** @var int|null */
15+
/** @var callback|string|int|null */
1616
private $lastModified;
17-
/** @var bool */
17+
/** @var callback|bool */
1818
private $hasRepresentation;
1919

20+
private IfNoneMatch $eTagParser;
21+
22+
private ?array $eTagParts = null;
23+
24+
public function __construct() {
25+
$this->eTagParser = new IfNoneMatch;
26+
}
27+
2028
/**
2129
* Initialize the object with information about the requested resource.
2230
*
23-
* @param string|null $eTag The entity-tag (including quotes), or null if
24-
* it is unknown.
25-
* @param string|int|null $lastModified The Last-Modified date in a format
31+
* @param callable|string|null $eTag The entity-tag (including quotes), or null if
32+
* it is unknown. Can also be provided as a callback for later evaluation.
33+
* @param callable|string|int|null $lastModified The Last-Modified date in a format
2634
* accepted by ConvertibleTimestamp, or null if it is unknown.
27-
* @param bool|null $hasRepresentation Whether the server has a current
35+
* Can also be provided as a callback for later evaluation.
36+
* @param callable|bool|null $hasRepresentation Whether the server can serve a
2837
* representation of the target resource. This should be true if the
2938
* resource exists, and false if it does not exist. It is used for
3039
* wildcard validators -- the intended use case is to abort a PUT if the
3140
* resource does (or does not) exist. If null is passed, we assume that
32-
* the resource exists if an ETag was specified for it.
41+
* the resource exists if an ETag or last-modified data was specified for it.
42+
* Can also be provided as a callback for later evaluation.
3343
*/
34-
public function setValidators( $eTag, $lastModified, $hasRepresentation ) {
44+
public function setValidators(
45+
$eTag,
46+
$lastModified,
47+
$hasRepresentation = null
48+
) {
3549
$this->eTag = $eTag;
36-
if ( $lastModified === null ) {
37-
$this->lastModified = null;
38-
} else {
39-
$this->lastModified = (int)ConvertibleTimestamp::convert( TS_UNIX, $lastModified );
40-
}
41-
$this->hasRepresentation = $hasRepresentation ?? ( $eTag !== null );
50+
$this->lastModified = $lastModified;
51+
$this->hasRepresentation = $hasRepresentation;
4252
}
4353

4454
/**
@@ -52,6 +62,68 @@ public function setVarnishETagHack( $hack ) {
5262
$this->varnishETagHack = $hack;
5363
}
5464

65+
private function getETag(): ?string {
66+
if ( is_callable( $this->eTag ) ) {
67+
// resolve callback
68+
$this->eTag = ( $this->eTag )();
69+
}
70+
71+
return $this->eTag;
72+
}
73+
74+
private function getETagParts(): ?array {
75+
if ( $this->eTagParts !== null ) {
76+
return $this->eTagParts;
77+
}
78+
79+
$eTag = $this->getETag();
80+
81+
if ( $eTag === null ) {
82+
return null;
83+
}
84+
85+
$this->eTagParts = $this->eTagParser->parseETag( $eTag );
86+
if ( !$this->eTagParts ) {
87+
throw new RuntimeException( 'Invalid ETag returned by handler: `' .
88+
$this->eTagParser->getLastError() . '`' );
89+
}
90+
91+
return $this->eTagParts;
92+
}
93+
94+
private function getLastModified(): ?int {
95+
if ( is_callable( $this->lastModified ) ) {
96+
// resolve callback
97+
$this->lastModified = ( $this->lastModified )();
98+
}
99+
100+
if ( is_string( $this->lastModified ) ) {
101+
// normalize to int
102+
$this->lastModified = (int)ConvertibleTimestamp::convert(
103+
TS_UNIX,
104+
$this->lastModified
105+
);
106+
}
107+
108+
// should be int or null now.
109+
return $this->lastModified;
110+
}
111+
112+
private function hasRepresentation(): bool {
113+
if ( is_callable( $this->hasRepresentation ) ) {
114+
// resolve callback
115+
$this->hasRepresentation = ( $this->hasRepresentation )();
116+
}
117+
118+
if ( $this->hasRepresentation === null ) {
119+
// apply fallback
120+
$this->hasRepresentation = $this->getETag() !== null
121+
|| $this->getLastModified() !== null;
122+
}
123+
124+
return $this->hasRepresentation;
125+
}
126+
55127
/**
56128
* Check conditional request headers in the order required by RFC 7232 section 6.
57129
*
@@ -60,23 +132,13 @@ public function setVarnishETagHack( $hack ) {
60132
* continue processing the request.
61133
*/
62134
public function checkPreconditions( RequestInterface $request ) {
63-
$parser = new IfNoneMatch;
64-
if ( $this->eTag !== null ) {
65-
$resourceTag = $parser->parseETag( $this->eTag );
66-
if ( !$resourceTag ) {
67-
throw new RuntimeException( 'Invalid ETag returned by handler: `' .
68-
$parser->getLastError() . '`' );
69-
}
70-
} else {
71-
$resourceTag = null;
72-
}
73135
$getOrHead = in_array( $request->getMethod(), [ 'GET', 'HEAD' ] );
74136
if ( $request->hasHeader( 'If-Match' ) ) {
75137
$im = $request->getHeader( 'If-Match' );
76138
$match = false;
77-
foreach ( $parser->parseHeaderList( $im ) as $tag ) {
78-
if ( ( $tag['whole'] === '*' && $this->hasRepresentation ) ||
79-
$this->strongCompare( $resourceTag, $tag )
139+
foreach ( $this->eTagParser->parseHeaderList( $im ) as $tag ) {
140+
if ( ( $tag['whole'] === '*' && $this->hasRepresentation() ) ||
141+
$this->strongCompare( $this->getETagParts(), $tag )
80142
) {
81143
$match = true;
82144
break;
@@ -87,25 +149,27 @@ public function checkPreconditions( RequestInterface $request ) {
87149
}
88150
} elseif ( $request->hasHeader( 'If-Unmodified-Since' ) ) {
89151
$requestDate = HttpDate::parse( $request->getHeader( 'If-Unmodified-Since' )[0] );
152+
$lastModified = $this->getLastModified();
90153
if ( $requestDate !== null
91-
&& ( $this->lastModified === null || $this->lastModified > $requestDate )
154+
&& ( $lastModified === null || $lastModified > $requestDate )
92155
) {
93156
return 412;
94157
}
95158
}
96159
if ( $request->hasHeader( 'If-None-Match' ) ) {
97160
$inm = $request->getHeader( 'If-None-Match' );
98-
foreach ( $parser->parseHeaderList( $inm ) as $tag ) {
99-
if ( ( $tag['whole'] === '*' && $this->hasRepresentation ) ||
100-
$this->weakCompare( $resourceTag, $tag )
161+
foreach ( $this->eTagParser->parseHeaderList( $inm ) as $tag ) {
162+
if ( ( $tag['whole'] === '*' && $this->hasRepresentation() ) ||
163+
$this->weakCompare( $this->getETagParts(), $tag )
101164
) {
102165
return $getOrHead ? 304 : 412;
103166
}
104167
}
105168
} elseif ( $getOrHead && $request->hasHeader( 'If-Modified-Since' ) ) {
106169
$requestDate = HttpDate::parse( $request->getHeader( 'If-Modified-Since' )[0] );
107-
if ( $requestDate !== null && $this->lastModified !== null
108-
&& $this->lastModified <= $requestDate
170+
$lastModified = $this->getLastModified();
171+
if ( $requestDate !== null && $lastModified !== null
172+
&& $lastModified <= $requestDate
109173
) {
110174
return 304;
111175
}
@@ -127,11 +191,21 @@ public function checkPreconditions( RequestInterface $request ) {
127191
* take precedence.
128192
*/
129193
public function applyResponseHeaders( ResponseInterface $response ) {
130-
if ( $this->lastModified !== null && !$response->hasHeader( 'Last-Modified' ) ) {
131-
$response->setHeader( 'Last-Modified', HttpDate::format( $this->lastModified ) );
194+
if ( $response->getStatusCode() >= 400 ) {
195+
// Don't add Last-Modified and ETag for errors, including 412.
196+
// Note that 304 responses are required to have these headers set.
197+
// See IETF RFC 7232 section 4.
198+
return;
199+
}
200+
201+
$lastModified = $this->getLastModified();
202+
if ( $lastModified !== null && !$response->hasHeader( 'Last-Modified' ) ) {
203+
$response->setHeader( 'Last-Modified', HttpDate::format( $lastModified ) );
132204
}
133-
if ( $this->eTag !== null && !$response->hasHeader( 'ETag' ) ) {
134-
$response->setHeader( 'ETag', $this->eTag );
205+
206+
$eTag = $this->getETag();
207+
if ( $eTag !== null && !$response->hasHeader( 'ETag' ) ) {
208+
$response->setHeader( 'ETag', $eTag );
135209
}
136210
}
137211

‎includes/Rest/Handler.php

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -481,10 +481,17 @@ protected function getJsonLocalizer(): JsonLocalizer {
481481
protected function getConditionalHeaderUtil() {
482482
if ( $this->conditionalHeaderUtil === null ) {
483483
$this->conditionalHeaderUtil = new ConditionalHeaderUtil;
484+
485+
// NOTE: It would be nicer to have Handler implement a
486+
// ConditionalHeaderValues interface that defines methods that
487+
// ConditionalHeaderUtil can call. But the relevant methods already
488+
// exist in Handler as protected and stable to override.
489+
// We can't make them public without breaking all subclasses that
490+
// override them. So we pass closures for now.
484491
$this->conditionalHeaderUtil->setValidators(
485-
$this->getETag(),
486-
$this->getLastModified(),
487-
$this->hasRepresentation()
492+
fn () => $this->getETag(),
493+
fn () => $this->getLastModified(),
494+
fn () => $this->hasRepresentation()
488495
);
489496
}
490497
return $this->conditionalHeaderUtil;
@@ -1091,6 +1098,9 @@ protected function getLastModified() {
10911098
* This must be a complete ETag, including double quotes.
10921099
* See RFC 7231 §7.2 and RFC 7232 §2.3 for semantics.
10931100
*
1101+
* This method should return null if the resource doesn't exist. It may also
1102+
* return null if ETag semantics is not supported by the Handler.
1103+
*
10941104
* @stable to override
10951105
*
10961106
* @return string|null
@@ -1104,6 +1114,9 @@ protected function getETag() {
11041114
* exists. This is used for wildcard validators, for example "If-Match: *"
11051115
* fails if the resource does not exist.
11061116
*
1117+
* If this method returns null, the value returned by getETag() will be used
1118+
* to determine whether the resource exists.
1119+
*
11071120
* In a state-changing request, the return value of this method should
11081121
* reflect the state before the requested change is applied.
11091122
*

‎tests/phpunit/unit/includes/Rest/ConditionalHeaderUtilTest.php

Lines changed: 34 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,18 @@
66
use MediaWiki\Rest\RequestData;
77
use MediaWiki\Rest\Response;
88
use MediaWikiUnitTestCase;
9+
use PHPUnit\Framework\Assert;
910

1011
/**
1112
* @covers \MediaWiki\Rest\ConditionalHeaderUtil
1213
*/
1314
class ConditionalHeaderUtilTest extends MediaWikiUnitTestCase {
15+
private static function makeFail( string $fn ) {
16+
return static function () use ( $fn ) {
17+
Assert::fail( "$fn was not expected to be called" );
18+
};
19+
}
20+
1421
public static function provider() {
1522
return [
1623
'nothing' => [
@@ -74,7 +81,7 @@ public static function provider() {
7481
null,
7582
[ 'If-Match' => '"b"' ],
7683
412,
77-
[ 'ETag' => [ '"a"' ] ]
84+
[]
7885
],
7986
'ius true' => [
8087
'GET',
@@ -92,7 +99,7 @@ public static function provider() {
9299
null,
93100
[ 'If-Unmodified-Since' => 'Mon, 14 Oct 2019 00:00:00 GMT' ],
94101
412,
95-
[ 'Last-Modified' => [ 'Mon, 14 Oct 2019 00:00:01 GMT' ] ]
102+
[]
96103
],
97104
'im true, ius false' => [
98105
'GET',
@@ -160,6 +167,15 @@ public static function provider() {
160167
null,
161168
[ 'ETag' => [ '"a"' ] ]
162169
],
170+
'im star true, no etag' => [
171+
'GET',
172+
null,
173+
null,
174+
true,
175+
[ 'If-Match' => '*' ],
176+
null,
177+
[]
178+
],
163179
'im star false' => [
164180
'GET',
165181
null,
@@ -176,7 +192,7 @@ public static function provider() {
176192
null,
177193
[ 'If-None-Match' => '"a"' ],
178194
412,
179-
[ 'ETag' => [ '"a"' ] ]
195+
[]
180196
],
181197
'im multiple true' => [
182198
'GET',
@@ -194,7 +210,7 @@ public static function provider() {
194210
null,
195211
[ 'If-Match' => '"b", "c"' ],
196212
412,
197-
[ 'ETag' => [ '"a"' ] ]
213+
[]
198214
],
199215
// A strong ETag returned by the server may have been "weakened" by
200216
// a proxy or middleware, e.g. when applying compression and setting
@@ -217,7 +233,7 @@ public static function provider() {
217233
null,
218234
[ 'If-Match' => '"a"' ],
219235
412,
220-
[ 'ETag' => [ 'W/"a"' ] ]
236+
[]
221237
],
222238
'If-Match weak vs weak' => [
223239
'GET',
@@ -226,22 +242,22 @@ public static function provider() {
226242
null,
227243
[ 'If-Match' => 'W/"a"' ],
228244
412,
229-
[ 'ETag' => [ 'W/"a"' ] ]
245+
[]
230246
],
231247
'ims with resource unknown' => [
232248
'GET',
233249
null,
234250
null,
235-
null,
251+
false,
236252
[ 'If-Modified-Since' => 'Mon, 14 Oct 2019 00:00:00 GMT' ],
237253
null,
238254
[]
239255
],
240256
'ius with resource unknown' => [
241257
'GET',
258+
self::makeFail( 'getETag' ),
242259
null,
243-
null,
244-
null,
260+
false,
245261
[ 'If-Unmodified-Since' => 'Mon, 14 Oct 2019 00:00:00 GMT' ],
246262
412,
247263
[]
@@ -257,8 +273,8 @@ public static function provider() {
257273
],
258274
'If-None-Match wildcard non-GET request, resource has representation' => [
259275
'POST',
260-
null,
261-
null,
276+
self::makeFail( 'getETag' ),
277+
self::makeFail( 'getLastModified' ),
262278
true,
263279
[ 'If-None-Match' => '*' ],
264280
412,
@@ -281,7 +297,12 @@ public function testConditionalHeaderUtil( $method, $eTag, $lastModified, $hasRe
281297
$this->assertSame( $expectedStatus, $status );
282298

283299
$response = new Response;
284-
$util->applyResponseHeaders( $response );
285-
$this->assertSame( $expectedResponseHeaders, $response->getHeaders() );
300+
301+
if ( $status ) {
302+
$response->setStatus( $status );
303+
304+
$util->applyResponseHeaders( $response );
305+
$this->assertSame( $expectedResponseHeaders, $response->getHeaders() );
306+
}
286307
}
287308
}

0 commit comments

Comments
 (0)