Issue #3080259 by gabesullice, alexpott, Wim Leers: Links with different target attributes are improperly merged

merge-requests/2419/head
effulgentsia 2019-10-16 09:47:06 -07:00
parent 8556a4d55f
commit 68bacadce5
9 changed files with 199 additions and 86 deletions

View File

@ -991,7 +991,7 @@ class EntityResource {
protected function buildWrappedResponse(TopLevelDataInterface $data, Request $request, IncludedData $includes, $response_code = 200, array $headers = [], LinkCollection $links = NULL, array $meta = []) {
$links = ($links ?: new LinkCollection([]));
if (!$links->hasLinkWithKey('self')) {
$self_link = new Link(new CacheableMetadata(), self::getRequestLink($request), ['self']);
$self_link = new Link(new CacheableMetadata(), self::getRequestLink($request), 'self');
$links = $links->withLink('self', $self_link);
}
$response = new ResourceResponse(new JsonApiDocumentTopLevel($data, $includes, $links, $meta), $response_code, $headers);
@ -1274,20 +1274,20 @@ class EntityResource {
// Check if this is not the last page.
if ($link_context['has_next_page']) {
$next_url = static::getRequestLink($request, static::getPagerQueries('next', $offset, $size, $query));
$pager_links = $pager_links->withLink('next', new Link(new CacheableMetadata(), $next_url, ['next']));
$pager_links = $pager_links->withLink('next', new Link(new CacheableMetadata(), $next_url, 'next'));
if (!empty($total)) {
$last_url = static::getRequestLink($request, static::getPagerQueries('last', $offset, $size, $query, $total));
$pager_links = $pager_links->withLink('last', new Link(new CacheableMetadata(), $last_url, ['last']));
$pager_links = $pager_links->withLink('last', new Link(new CacheableMetadata(), $last_url, 'last'));
}
}
// Check if this is not the first page.
if ($offset > 0) {
$first_url = static::getRequestLink($request, static::getPagerQueries('first', $offset, $size, $query));
$pager_links = $pager_links->withLink('first', new Link(new CacheableMetadata(), $first_url, ['first']));
$pager_links = $pager_links->withLink('first', new Link(new CacheableMetadata(), $first_url, 'first'));
$prev_url = static::getRequestLink($request, static::getPagerQueries('prev', $offset, $size, $query));
$pager_links = $pager_links->withLink('prev', new Link(new CacheableMetadata(), $prev_url, ['prev']));
$pager_links = $pager_links->withLink('prev', new Link(new CacheableMetadata(), $prev_url, 'prev'));
}
return $pager_links;

View File

@ -82,14 +82,22 @@ class EntryPoint extends ControllerBase {
return !$resource->isInternal();
});
$self_link = new Link(new CacheableMetadata(), Url::fromRoute('jsonapi.resource_list'), ['self']);
$self_link = new Link(new CacheableMetadata(), Url::fromRoute('jsonapi.resource_list'), 'self');
$urls = array_reduce($resources, function (LinkCollection $carry, ResourceType $resource_type) {
if ($resource_type->isLocatable() || $resource_type->isMutable()) {
$route_suffix = $resource_type->isLocatable() ? 'collection' : 'collection.post';
$url = Url::fromRoute(sprintf('jsonapi.%s.%s', $resource_type->getTypeName(), $route_suffix))->setAbsolute();
// Using a resource type name in place of a link relation type is not
// technically valid. However, since it matches the link key, it will
// not actually be serialized since the rel is omitted if it matches the
// link key; because of that no client can rely on it. Once an extension
// relation type is implemented for links to a collection, that should
// be used instead. Unfortunately, the `collection` link relation type
// would not be semantically correct since it would imply that the
// entrypoint is a *member* of the link target.
// @todo: implement an extension relation type to signal that this is a primary collection resource.
$link_relation_types = [];
return $carry->withLink($resource_type->getTypeName(), new Link(new CacheableMetadata(), $url, $link_relation_types));
$link_relation_type = $resource_type->getTypeName();
return $carry->withLink($resource_type->getTypeName(), new Link(new CacheableMetadata(), $url, $link_relation_type));
}
return $carry;
}, new LinkCollection(['self' => $self_link]));

View File

@ -178,7 +178,7 @@ class FileUpload {
}
// @todo Remove line below in favor of commented line in https://www.drupal.org/project/jsonapi/issues/2878463.
$self_link = new Link(new CacheableMetadata(), Url::fromRoute('jsonapi.file--file.individual', ['entity' => $file->uuid()]), ['self']);
$self_link = new Link(new CacheableMetadata(), Url::fromRoute('jsonapi.file--file.individual', ['entity' => $file->uuid()]), 'self');
/* $self_link = new Link(new CacheableMetadata(), $this->entity->toUrl('jsonapi'), ['self']); */
$links = new LinkCollection(['self' => $self_link]);

View File

@ -3,6 +3,7 @@
namespace Drupal\jsonapi\JsonApiResource;
use Drupal\Component\Assertion\Inspector;
use Drupal\Component\Utility\DiffArray;
use Drupal\Core\Cache\CacheableDependencyInterface;
use Drupal\Core\Cache\CacheableDependencyTrait;
use Drupal\Core\Cache\CacheableMetadata;
@ -41,6 +42,9 @@ final class Link implements CacheableDependencyInterface {
* The link relation types.
*
* @var string[]
*
* @todo: change this type documentation to be a single string in
* https://www.drupal.org/project/drupal/issues/3080467.
*/
protected $rel;
@ -64,16 +68,23 @@ final class Link implements CacheableDependencyInterface {
* entity on which the link will appear.
* @param \Drupal\Core\Url $url
* The Url object for the link.
* @param string[] $link_relation_types
* @param string $link_relation_type
* An array of registered or extension RFC8288 link relation types.
* @param array $target_attributes
* An associative array of target attributes for the link.
*
* @see https://tools.ietf.org/html/rfc8288#section-2.1
*/
public function __construct(CacheableMetadata $cacheability, Url $url, array $link_relation_types, array $target_attributes = []) {
// @todo: uncomment the extra assertion below when JSON:API begins to use its own extension relation types.
assert(/* !empty($link_relation_types) && */Inspector::assertAllStrings($link_relation_types));
public function __construct(CacheableMetadata $cacheability, Url $url, $link_relation_type, array $target_attributes = []) {
// @todo Remove this conditional block in drupal:9.0.0 and add a type hint to the $link_relation_type argument of this method in https://www.drupal.org/project/drupal/issues/3080467.
if (is_array($link_relation_type)) {
@trigger_error('Constructing a ' . self::class . ' with an array of link relation types is deprecated in drupal:8.8.0 and will throw a fatal error in drupal:9.0.0. Pass a single string instead. See https://www.drupal.org/node/3087821.', E_USER_DEPRECATED);
assert(Inspector::assertAllStrings($link_relation_type));
}
else {
assert(is_string($link_relation_type));
$link_relation_type = [$link_relation_type];
}
assert(Inspector::assertAllStrings(array_keys($target_attributes)));
assert(Inspector::assertAll(function ($target_attribute_value) {
return is_string($target_attribute_value) || is_array($target_attribute_value);
@ -81,7 +92,7 @@ final class Link implements CacheableDependencyInterface {
$generated_url = $url->setAbsolute()->toString(TRUE);
$this->href = $generated_url->getGeneratedUrl();
$this->uri = $url;
$this->rel = $link_relation_types;
$this->rel = $link_relation_type;
$this->attributes = $target_attributes;
$this->setCacheability($cacheability->addCacheableDependency($generated_url));
}
@ -106,13 +117,26 @@ final class Link implements CacheableDependencyInterface {
return $this->href;
}
/**
* Gets the link's relation type.
*
* @return string
* The link's relation type.
*/
public function getLinkRelationType() {
return reset($this->rel);
}
/**
* Gets the link's relation types.
*
* @return string[]
* The link's relation types.
*
* @todo: remove this method in https://www.drupal.org/project/drupal/issues/3080467.
*/
public function getLinkRelationTypes() {
@trigger_error(__METHOD__ . '() is deprecated in drupal:8.8.0 and will be removed in drupal:9.0.0. Use getLinkRelationType() instead. See https://www.drupal.org/node/3087821.', E_USER_DEPRECATED);
return $this->rel;
}
@ -127,7 +151,7 @@ final class Link implements CacheableDependencyInterface {
}
/**
* Compares two links by their href.
* Compares two links.
*
* @param \Drupal\jsonapi\JsonApiResource\Link $a
* The first link.
@ -135,16 +159,35 @@ final class Link implements CacheableDependencyInterface {
* The second link.
*
* @return int
* The result of strcmp() on the links' hrefs.
* 0 if the links can be considered identical, an integer greater than or
* less than 0 otherwise.
*/
public static function compare(Link $a, Link $b) {
return strcmp($a->getHref(), $b->getHref());
// @todo: Remove $rel_to_string function once rel property is a single
// string in https://www.drupal.org/project/drupal/issues/3080467.
$rel_to_string = function (array $rel) {
// Sort the link relation type array so that the order of link relation
// types does not matter during link comparison.
sort($rel);
return implode(' ', $rel);
};
// Any string concatenation would work, but a Link header-like format makes
// it clear what is being compared.
$a_string = sprintf('<%s>;rel="%s"', $a->getHref(), $rel_to_string($a->rel));
$b_string = sprintf('<%s>;rel="%s"', $b->getHref(), $rel_to_string($b->rel));
$cmp = strcmp($a_string, $b_string);
// If the `href` or `rel` of the links are not equivalent, it's not
// necessary to compare target attributes.
if ($cmp === 0) {
return (int) !empty(DiffArray::diffAssocRecursive($a->getTargetAttributes(), $b->getTargetAttributes()));
}
return $cmp;
}
/**
* Merges two link objects' relation types and target attributes.
* Merges two equivalent links into one link with the merged cacheability.
*
* The links must share the same URI.
* The links must share the same URI, link relation type and attributes.
*
* @param \Drupal\jsonapi\JsonApiResource\Link $a
* The first link.
@ -152,15 +195,12 @@ final class Link implements CacheableDependencyInterface {
* The second link.
*
* @return static
* A new JSON:API Link object with the link relation type and target
* attributes merged.
* A new JSON:API Link object with the cacheability of both links merged.
*/
public static function merge(Link $a, Link $b) {
assert(static::compare($a, $b) === 0);
$merged_rels = array_unique(array_merge($a->getLinkRelationTypes(), $b->getLinkRelationTypes()));
$merged_attributes = array_merge_recursive($a->getTargetAttributes(), $b->getTargetAttributes());
assert(static::compare($a, $b) === 0, 'Only equivalent links can be merged.');
$merged_cacheability = (new CacheableMetadata())->addCacheableDependency($a)->addCacheableDependency($b);
return new static($merged_cacheability, $a->getUri(), $merged_rels, $merged_attributes);
return new static($merged_cacheability, $a->getUri(), $a->getLinkRelationType(), $a->getTargetAttributes());
}
}

View File

@ -71,8 +71,9 @@ final class LinkCollection implements \IteratorAggregate {
* Gets a new LinkCollection with the given link inserted.
*
* @param string $key
* A key for the link. If the key already exists and the link shares an href
* with an existing link with that key, those links will be merged together.
* A key for the link. If the key already exists and the link shares an
* href, link relation type and attributes with an existing link with that
* key, those links will be merged together.
* @param \Drupal\jsonapi\JsonApiResource\Link $new_link
* The link to insert.
*

View File

@ -236,7 +236,7 @@ class Relationship implements TopLevelDataInterface {
if ($context_is_versionable) {
$self_link->setOption('query', [JsonApiSpec::VERSION_QUERY_PARAMETER => $context->getVersionIdentifier()]);
}
$links = $links->withLink('self', new Link(new CacheableMetadata(), $self_link, ['self']));
$links = $links->withLink('self', new Link(new CacheableMetadata(), $self_link, 'self'));
}
$has_non_internal_resource_type = array_reduce($context_resource_type->getRelatableResourceTypesByField($public_field_name), function ($carry, ResourceType $target) {
return $carry ?: !$target->isInternal();
@ -252,7 +252,7 @@ class Relationship implements TopLevelDataInterface {
if ($context_is_versionable) {
$related_link->setOption('query', [JsonApiSpec::VERSION_QUERY_PARAMETER => $context->getVersionIdentifier()]);
}
$links = $links->withLink('related', new Link(new CacheableMetadata(), $related_link, ['related']));
$links = $links->withLink('related', new Link(new CacheableMetadata(), $related_link, 'related'));
}
}
return $links;

View File

@ -244,19 +244,19 @@ class ResourceObject implements CacheableDependencyInterface, ResourceIdentifier
// revision changes and to disambiguate resource objects with the same
// `type` and `id` in a `version-history` collection.
$self_with_version_url = $self_url->setOption('query', [JsonApiSpec::VERSION_QUERY_PARAMETER => 'id:' . $entity->getRevisionId()]);
$links = $links->withLink('self', new Link(new CacheableMetadata(), $self_with_version_url, ['self']));
$links = $links->withLink('self', new Link(new CacheableMetadata(), $self_with_version_url, 'self'));
}
if (!$entity->isDefaultRevision()) {
$latest_version_url = $self_url->setOption('query', [JsonApiSpec::VERSION_QUERY_PARAMETER => 'rel:' . VersionByRel::LATEST_VERSION]);
$links = $links->withLink(VersionByRel::LATEST_VERSION, new Link(new CacheableMetadata(), $latest_version_url, [VersionByRel::LATEST_VERSION]));
$links = $links->withLink(VersionByRel::LATEST_VERSION, new Link(new CacheableMetadata(), $latest_version_url, VersionByRel::LATEST_VERSION));
}
if (!$entity->isLatestRevision()) {
$working_copy_url = $self_url->setOption('query', [JsonApiSpec::VERSION_QUERY_PARAMETER => 'rel:' . VersionByRel::WORKING_COPY]);
$links = $links->withLink(VersionByRel::WORKING_COPY, new Link(new CacheableMetadata(), $working_copy_url, [VersionByRel::WORKING_COPY]));
$links = $links->withLink(VersionByRel::WORKING_COPY, new Link(new CacheableMetadata(), $working_copy_url, VersionByRel::WORKING_COPY));
}
}
if (!$links->hasLinkWithKey('self')) {
$links = $links->withLink('self', new Link(new CacheableMetadata(), $self_url, ['self']));
$links = $links->withLink('self', new Link(new CacheableMetadata(), $self_url, 'self'));
}
}
return $links;

View File

@ -377,7 +377,7 @@ abstract class ResourceTestBase extends BrowserTestBase {
// Don't use cached normalizations in tests.
$this->container->get('cache.jsonapi_normalizations')->deleteAll();
$self_link = new Link(new CacheableMetadata(), $url, ['self']);
$self_link = new Link(new CacheableMetadata(), $url, 'self');
$resource_type = $this->container->get('jsonapi.resource_type.repository')->getByTypeName(static::$resourceTypeName);
$doc = new JsonApiDocumentTopLevel(new ResourceObjectData([ResourceObject::createFromEntity($resource_type, $entity)], 1), new NullIncludedData(), new LinkCollection(['self' => $self_link]));
return $this->serializer->normalize($doc, 'api_json', [

View File

@ -19,67 +19,129 @@ use Drupal\Tests\UnitTestCase;
class LinkTest extends UnitTestCase {
/**
* @covers ::merge
* @dataProvider mergeTargetAttributesProvider
* @covers ::compare
* @dataProvider linkComparisonProvider
*/
public function testMergeTargetAttributes($a, $b, $expected) {
$this->assertSame($expected->getTargetAttributes(), Link::merge($a, $b)->getTargetAttributes());
public function testLinkComparison(Link $a, Link $b, $expected) {
$actual = Link::compare($a, $b);
$this->assertSame($expected, $actual === 0);
}
/**
* Provides test data for link comparison.
*/
public function linkComparisonProvider() {
$this->mockUrlAssembler();
return [
'same href and same link relation type' => [
new Link(new CacheableMetadata(), Url::fromUri('https://jsonapi.org/foo'), 'self'),
new Link(new CacheableMetadata(), Url::fromUri('https://jsonapi.org/foo'), 'self'),
TRUE,
],
'different href and same link relation type' => [
new Link(new CacheableMetadata(), Url::fromUri('https://jsonapi.org/foo'), 'self'),
new Link(new CacheableMetadata(), Url::fromUri('https://jsonapi.org/bar'), 'self'),
FALSE,
],
'same href and different link relation type' => [
new Link(new CacheableMetadata(), Url::fromUri('https://jsonapi.org/foo'), 'self'),
new Link(new CacheableMetadata(), Url::fromUri('https://jsonapi.org/foo'), 'related'),
FALSE,
],
'same href and same link relation type and empty target attributes' => [
new Link(new CacheableMetadata(), Url::fromUri('https://jsonapi.org/foo'), 'self', []),
new Link(new CacheableMetadata(), Url::fromUri('https://jsonapi.org/foo'), 'self', []),
TRUE,
],
'same href and same link relation type and same target attributes' => [
new Link(new CacheableMetadata(), Url::fromUri('https://jsonapi.org/foo'), 'self', ['anchor' => 'https://jsonapi.org']),
new Link(new CacheableMetadata(), Url::fromUri('https://jsonapi.org/foo'), 'self', ['anchor' => 'https://jsonapi.org']),
TRUE,
],
// These links are not considered equivalent because it would while the
// `href` remains the same, the anchor changes the context of the link.
'same href and same link relation type and different target attributes' => [
new Link(new CacheableMetadata(), Url::fromUri('https://jsonapi.org/boy'), 'self', ['title' => 'sue']),
new Link(new CacheableMetadata(), Url::fromUri('https://jsonapi.org/boy'), 'self', ['anchor' => '/sob', 'title' => 'pa']),
FALSE,
],
'same href and same link relation type and same nested target attributes' => [
new Link(new CacheableMetadata(), Url::fromUri('https://jsonapi.org/foo'), 'self', ['data' => ['foo' => 'bar']]),
new Link(new cacheablemetadata(), Url::fromUri('https://jsonapi.org/foo'), 'self', ['data' => ['foo' => 'bar']]),
TRUE,
],
'same href and same link relation type and different nested target attributes' => [
new Link(new CacheableMetadata(), Url::fromUri('https://jsonapi.org/foo'), 'self', ['data' => ['foo' => 'bar']]),
new Link(new CacheableMetadata(), Url::fromUri('https://jsonapi.org/foo'), 'self', ['data' => ['foo' => 'baz']]),
FALSE,
],
// These links are not considered equivalent because it would be unclear
// which title corresponds to which link relation type.
'same href and different link relation types and different target attributes' => [
new Link(new CacheableMetadata(), Url::fromUri('https://jsonapi.org/boy'), 'self', ['title' => 'A boy named Sue']),
new Link(new CacheableMetadata(), Url::fromUri('https://jsonapi.org/boy'), 'edit', ['title' => 'Change name to Bill or George']),
FALSE,
],
];
}
/**
* @covers ::merge
* @dataProvider linkMergeProvider
*/
public function testLinkMerge(Link $a, Link $b, $expected) {
if ($expected instanceof Link) {
$this->assertSame($expected->getCacheTags(), Link::merge($a, $b)->getCacheTags());
}
else {
$this->expectExceptionObject($expected);
Link::merge($a, $b);
}
}
/**
* Provides test data for link merging.
*/
public function mergeTargetAttributesProvider() {
$cases = [
'strings' => [
['key' => 'foo'],
['key' => 'bar'],
['key' => ['foo', 'bar']],
public function linkMergeProvider() {
$this->mockUrlAssembler();
return [
'same everything' => [
new Link((new CacheableMetadata())->addCacheTags(['foo']), Url::fromUri('https://jsonapi.org/foo'), 'self'),
new Link((new CacheableMetadata())->addCacheTags(['foo']), Url::fromUri('https://jsonapi.org/foo'), 'self'),
new Link((new CacheableMetadata())->addCacheTags(['foo']), Url::fromUri('https://jsonapi.org/foo'), 'self'),
],
'string and array' => [
['key' => 'foo'],
['key' => ['bar', 'baz']],
['key' => ['foo', 'bar', 'baz']],
],
'one-dimensional indexed arrays' => [
['key' => ['foo']],
['key' => ['bar']],
['key' => ['foo', 'bar']],
],
'one-dimensional keyed arrays' => [
['key' => ['foo' => 'tball']],
['key' => ['bar' => 'ista']],
[
'key' => [
'foo' => 'tball',
'bar' => 'ista',
],
],
],
'two-dimensional indexed arrays' => [
['one' => ['two' => ['foo']]],
['one' => ['two' => ['bar']]],
['one' => ['two' => ['foo', 'bar']]],
],
'two-dimensional keyed arrays' => [
['one' => ['two' => ['foo' => 'tball']]],
['one' => ['two' => ['bar' => 'ista']]],
[
'one' => [
'two' => [
'foo' => 'tball',
'bar' => 'ista',
],
],
],
'different cache tags' => [
new Link((new CacheableMetadata())->addCacheTags(['foo']), Url::fromUri('https://jsonapi.org/foo'), 'self'),
new Link((new CacheableMetadata())->addCacheTags(['bar']), Url::fromUri('https://jsonapi.org/foo'), 'self'),
new Link((new CacheableMetadata())->addCacheTags(['foo', 'bar']), Url::fromUri('https://jsonapi.org/foo'), 'self'),
],
];
}
/**
* @covers ::getLinkRelationType
*/
public function testGetLinkRelationType() {
$this->mockUrlAssembler();
return array_map(function ($arguments) {
return array_map(function ($attributes) {
return new Link(new CacheableMetadata(), Url::fromUri('https://jsonapi.org/'), ['item'], $attributes);
}, $arguments);
}, $cases);
$link = new Link((new CacheableMetadata())->addCacheTags(['foo']), Url::fromUri('https://jsonapi.org/foo'), 'self');
$this->assertSame('self', $link->getLinkRelationType());
}
/**
* @group legacy
* @expectedDeprecation Constructing a Drupal\jsonapi\JsonApiResource\Link with an array of link relation types is deprecated in drupal:8.8.0 and will throw a fatal error in drupal:9.0.0. Pass a single string instead. See https://www.drupal.org/node/3087821.
* @expectedDeprecation Drupal\jsonapi\JsonApiResource\Link::getLinkRelationTypes() is deprecated in drupal:8.8.0 and will be removed in drupal:9.0.0. Use getLinkRelationType() instead. See https://www.drupal.org/node/3087821.
* @covers ::__construct
* @covers ::getLinkRelationTypes
*/
public function testLinkDeprecations() {
$this->mockUrlAssembler();
$link = new Link((new CacheableMetadata())->addCacheTags(['foo']), Url::fromUri('https://jsonapi.org/foo'), ['self', 'foo']);
$this->assertSame(['self', 'foo'], $link->getLinkRelationTypes());
$this->assertSame('self', $link->getLinkRelationType());
$link = new Link((new CacheableMetadata())->addCacheTags(['foo']), Url::fromUri('https://jsonapi.org/foo'), 'self');
$this->assertSame(['self'], $link->getLinkRelationTypes());
}
/**
@ -89,7 +151,9 @@ class LinkTest extends UnitTestCase {
$url_assembler = $this->getMockBuilder(UnroutedUrlAssemblerInterface::class)
->disableOriginalConstructor()
->getMock();
$url_assembler->method('assemble')->willReturn((new GeneratedUrl())->setGeneratedUrl('https://jsonapi.org/'));
$url_assembler->method('assemble')->will($this->returnCallback(function ($uri) {
return (new GeneratedUrl())->setGeneratedUrl($uri);
}));
$container = new ContainerBuilder();
$container->set('unrouted_url_assembler', $url_assembler);