Problem/Motivation
According to RFC8288, a link is:
a typed connection between two resources and is comprised of:
- a link context,
- a link relation type (Section 2.1),
- a link target, and
- optionally, target attributes (Section 2.2).
When LinkCollection::merge
merges two Link
objects with the same key and target URI, it does not consider target attributes as a differentiating factor.
An example where this can break link semantics in via the anchor
target attribute. Typically, a "link context" is just the resource on which a link appears, however, a link context can be overridden by the anchor
target attribute. When JSON:API merges links, it only considers the links object key and the target URI to determine if a link is equivalent. In the case that one link defines an anchor
and the other does not, the semantics of the link without the anchor
will be broken. Consider the following example (appearing on a resource /bar
):
1. {"href": "/foo", "anchor": "/foo/alias", "rel": ["canonical"]}
2. {"href": "/foo", "rel": ["related"]}
These JSON objects are simplified. Real JSON:API link objects look different.
would be merged into:
3. {"href": "/foo", "rel": ["related", "canonical"], "anchor": "/foo/alias"}
Another example where this can be an issue is when two links share a common URI but different link relation types and target attributes:
1. {"href": "/foo", "rel": ["comments"], "title": "Comments on Foo"}
2. {"href": "/foo", "rel": ["add-comment"], "title": "Post a comment about Foo"}
would be merged into:
3. {"href": "/foo", "rel": ["comments", "add-comment"], "title": ["Comments on Foo", "Post a comment about Foo"]}
After that merge, it's impossible to know which of the two links represented by the merged object has which title
. The semantics of the link object are pretty much nonsensical.
For background, the reason that Drupal\jsonapi\JsonApiResource\Link
support more than one link relation type and merging of this kind is because RFC 8288 permits a Link
header to have more than one link relationship type and, apparently, I misread the RFC (mea culpa). In the following paragraph, I saw the first sentence, but didn't internalize the second sentence:
The
rel
parameter can, however, contain multiple link relation types. When this occurs, it establishes multiple links that share the same context, target, and target attributes. — Section 3.3 (emphasis added)
Proposed resolution
Deprecate passing an array of link relation types toLink::__construct
in favor of a single stringDeprecateLink::getLinkRelationTypes
(plural) in favor ofLink::getLinkRelationType
UpdateLink::compare
to only treat equivalent links as equal (it currently ignores target attributes)UpdateLink::merge
to only merge cacheability, since links with different relation types or attributes can no longer be merged
User interface changes
None.
API changes
Link::__construct
will have a deprecated argument type. Note: Drupal\jsonapi\JsonApiResource\Link
is an @internal
value object and no JSON:API code actually uses multiple link relation types or target attributes.
It's also important to note that there is currently no public API for adding custom links to JSON:API responses.
Data model changes
None.
Release notes snippet
None.
Comment | File | Size | Author |
---|---|---|---|
#40 | 3080259-40.patch | 26.83 KB | gabesullice |
#40 | interdiff.txt | 3.42 KB | gabesullice |
#36 | 3080259-36.patch | 27.08 KB | alexpott |
#36 | 35-36-interdiff.txt | 911 bytes | alexpott |
#35 | 3080259-35.patch | 26.19 KB | alexpott |
Comments
Comment #2
gabesulliceComment #3
gabesulliceComment #4
gabesulliceLet's see what the testbot thinks.
Comment #5
gabesulliceI added a followup for Drupal 9 here: #3080467: Remove BC layers from \Drupal\jsonapi\JsonApiResource\Link that handle links with more than one link relation type
I also added/updated todos to point to that issue.
Comment #9
gabesulliceLooks like it's pretty much all just deprecation notices. Easy fix.
Comment #11
gabesulliceLooks like I missed one.
Comment #12
gabesulliceGah, I generated that last interdiff and patch backwards. Correct ones attached.
Comment #13
gabesullice(ノಠ益ಠ)ノ彡┻━┻
Comment #15
Wim LeersThe examples in the issue summary are crystal clear. They demonstrate in an obvious way what the problem is. I would even say
😅This made me wonder why we didn't catch this originally. I think we probably didn't originally spot that because it was all still theoretical. We didn't have any concrete examples to look at. Now that @gabesullice has pushed the https://www.drupal.org/project/jsonapi_hypermedia forward towards stability, it now is easy to see how what we did originally is wrong.
Nit: should be singular.
👍 — our future maintainer selves will be grateful for this 🙏
🤔 I want to check my own understanding here: this comment and this comparison are happening after the string concatenation in the previous few lines, because there are potentially many target attributes, and if the link href + rel aren't the same already, there's no point in doing so.
Is that a correct understanding?
But … this code runs always anyway, so what is the real savings here?
This seems like a premature optimization … and one that accidentally was ineffective too? 😅
🤔 I think it would be valuable to include the concrete examples from the issue summary in the test cases.
Comment #16
gabesullice1. Good catch.
2. :)
3. Gah, I'm sorry you read so much into that. That was not supposed to be there. I broke that variable assignment out onto its own line for debugging reasons. Your overall rationale is correct though, I'm sorry my comment made no sense next to that line.
4. Good idea, I added some more cases.
Comment #17
Wim LeersLooks much better! I found two remaining concerns:
Expecting a particular exception class is fine, if it's meaningful. But
\LogicException
is super generic.So if we add more logic exceptions (with other messages) in the future, this test coverage will continue to pass.
I think this should be changed to
new \LogicException('expected message here')
.Comment #18
gabesullice1. Good call. Done.
2. I was trying not to use that example because
comments
andadd-comment
are made-up link relation types. I'm hoping that if we follow JSON:API Hypermedia's lead in core, we'll start validating those. Then this test would have to change. But, I see that it could be made a bit clearer. I came up with a coupled other examples and added some comments explaining why it'd be wrong to merge them. ♫Comment #19
Wim Leers🤩 — love the musical reference! 🎸
Comment #20
alexpottI don't think we should be adding commented out code. The @todo seems fine.
Does this mean that comparing two links actually changes them? That's a bit unexpected.
Why are we doing a Json::encode() in order to a strcmp. This looks super odd. It's not as if this function is being used for sorting.
The change from an assert to an exception is interesting. We should use @throws to document this. Ah here's why I think this is a bit odd here's the only runtime usage...
So here now it's the caller doing the call to ::compare first. With this change we're always going to call compare again.
Comment #21
gabesullice1. Removed. This was a practice we got in the habit of doing in JSON:API contrib to make followups trivial.
2. That's correct, but I don't think it's consequential. Those
sort
s are action on a protected property on afinal
value object. It's an implementation detail. Besides, the order of link relation types shouldn't be significant:Regardless, I changed the implementation slightly at that should eliminate this as a potential sticking point.
3. The idea was to use
Json::encode
as a way to compare a multidimensional array without writing some recursive function. You're right that this isn't actually being used for sorting, though I think it was used for sorting in some earlier iteration of this code (that was never committed). I think it's important to preserve the ability to use this method for sorting links sincecompare
is currently suitable for that purpose. However, instead of encoding, I looked around and foundDiffArray::diffAssocRecursive
which should achieve the same thing. Maybe that's preferable and more readable.4. I changed from an
assert
to aLogicException
to make the validation testable. After you pointed this out, I thought more about it and that desire was misplaced.assert
verifies a contract with a method caller (that only equivalent links may be merged). By testing for aLogicException
I'm just ensuring that the contract can't be violated. But that's exactly what the assert is designed to do in the first place. Instead, it's better to test the validation that's runningwithinthe assert (Link::compare
) so that I know that theassert
will pass or fail just as I expect it to... I updated the test coverage accordingly and changed theLogicException
back to anassert
. Thanks for calling this out and forcing me to think about it more critically!PS. I noticed some changes in the #18 patch that weren't in the interdiff so I reverted those in this interdiff along with the other changes. (for some reason there were whole lines that were lowercased in #18)
Comment #22
Wim Leers😲😲😲 I wish I'd known about the existence of this! Would've saved me a lot of time a few times in the past.
To make sense of what this does, I read the test at
\Drupal\Tests\Core\Common\DiffArrayTest
.This logic looks correct to me :)
🤓 "merged" is now out of place here, because these test cases were moved from the "merging" test coverage to the "comparing" test coverage.
Comment #23
alexpottNow that the json encoding is gone we're not unnecessary encoding.
I think this is more readable in a non ternary form.
I.e
If you have to explain a ternary then perhaps it's not worth using it.
Comment #24
gabesulliceAddressed #22 and #23.
Comment #25
gabesulliceLol, literally forgot a semi-colon.
Comment #26
Wim LeersComment #27
alexpottThe comment is too long > 80 chars.
This assert can move into the is_array part of the if.
We can assert that's its a string in the other.
Also the @todo can be removed because removing the deprecated path is going solve it.
__METHOD__ includes the class - see https://3v4l.org/16jeq. Plus there is no deprecation testing which might have revealed this.
Comment #28
gabesulliceDone. Please LMK if what I did is not the best practice for deprecation testing. I emulated what I could find by going through https://www.drupal.org/list-changes/drupal.
Comment #29
Wim LeersÜbernit: s/is properly/method is properly/. Alternatively, this line could just be deleted.
Can be fixed on commit.
Comment #30
alexpottThis bit conflicted with HEAD - this code appears to have been removed in #3036285: Add a \JsonApiResource\Relationship object to carry relationship data, metadata and a link collection.
We can access protected properties in static methods so there's no need to do the sort in the etc in the getLinkRelationType() method - which in turn means that I think we can make that method return the string we expect it to in the future right now. Which means that we have less BC to remove and more consistenct from the get go. Also the new method has no explicit test coverage.
This test can test the constructor deprecation too and be more explicit (make more assertions) about the BC layer.
Attached patch resolves all of this.
Comment #31
alexpottHopefully #30 fails because of deprecated code paths... It looks like #3036285: Add a \JsonApiResource\Relationship object to carry relationship data, metadata and a link collection. added some usages of constructing a Link object with an array of relationships.
Comment #33
gabesulliceThanks, @alexpott for the reroll and showing me the right way to do that deprecation testing!
I think this is good to go.
Comment #34
Wim Leers#30.2: I wish I had seen that! 👍🤓
I'm a tiny bit concerned about this because it makes it ever so slightly more difficult to move it from
string[]
tostring
. I think it was added to slightly simplifygetLinkRelationTypes()
(removal of the cast).Comment #35
alexpottOops. I think that's unnecessary - it was necessary when I was halfway through coding my thoughts on #30 but then it became unnecessary.
Comment #36
alexpottAlso noticed some now incorrect docs.
Comment #38
alexpottI missed that this was pointing to this issue. This needs to point to a change record.
Comment #39
gabesulliceCR created: https://www.drupal.org/node/3087821
I am not quite sure how to set the version metadata for it re: 8.9 vs 8.8
Patch to follow.
Comment #40
gabesulliceComment #41
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI think we need a 2nd CR to cover the above changes. Unless it makes sense to instead broaden https://www.drupal.org/node/3087821 to cover that, but seems like a separate thing to me. Either way though, Needs work for that.
I'm also wondering whether we're at all concerned about the BC break of callers expecting prior behavior of compare() or merge(). For example, callers of merge() will now get an assertion error where they didn't used to. In the case of equivalent hrefs but differing relation or attributes, should we change that assertion to a deprecation error and return a link with the merged relations? Or is there a compelling reason to justify the BC break?
Comment #42
alexpottCallers to merge previously got an assertion error. Prior to this change callers had to ensure they were merging links objects that ::compare returns 0 for. That behaviour is unchanged.
Comment #43
gabesulliceI created a change record explaining the change: https://www.drupal.org/node/3088385
I'm reluctant to provide a BC-layer because:
Link::compare
will continue to function just fine. Callers that were not usingLink::compare
are using an@internal
API in a way that it was not meant to be used.Link::compare
beforemerge
Comment #44
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThanks for the CR. The rationale in there and in #43 for not including a BC layer for merge() works for me. Back to RTBC.
Comment #45
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAdding reviewer credit.
Comment #48
effulgentsia CreditAttribution: effulgentsia at Acquia commentedPushed to 9.0.x and 8.9.x.
Leaving RTBC for 8.8.x. We're currently in code freeze for alpha, but I'm +1 for committing this to 8.8.x once it's unfrozen (between alpha and beta). Leaving the CRs unpublished until that's done.
Comment #49
gabesullice🎉
Comment #51
effulgentsia CreditAttribution: effulgentsia at Acquia commentedPushed to 8.8.x and published the CRs.