#2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method).
Problem/Motivation
#2842148: The path for JSON API to "super stability" has had this in its list of crucial things since it was created:
Confidence: correctness verification + regression guards by implementing comprehensive functional tests, similar to #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method
As described in https://wimleers.com/blog/api-first-drupal-really, this is finally complete for core REST. So now we can do the same for JSON API
Proposed resolution
- Create a base test class similar to
\Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase
, restricted to JSON API individualGET
+POST
+PATCH
+DELETE
- But there's only a single format obviously (
api_json
), so it should be simpler: fewer permutations to test - Consider not testing all authentication mechanisms either (would be even fewer permutations to test)
- As much as possible, create follow-up issues for bugs discovered, try not to fix them here.
- Follow-up issue for doing the same with collections of that resource type
- Follow-up issue for doing the same with relationships of that resource type
Remaining tasks
- Patch.
- Review.
User interface changes
None.
API changes
None.
Data model changes
None.
Issues that will get clarity
Comment | File | Size | Author |
---|---|---|---|
#96 | 2930028-96.patch | 26.91 KB | Wim Leers |
| |||
#85 | 2930028-85.patch | 18.87 KB | Wim Leers |
#85 | interdiff-82-85.txt | 3.08 KB | Wim Leers |
Comments
Comment #2
Wim LeersAs the architect of REST module's test coverage, I'll take this on, but I'll require sign-off from @e0ipso before committing.
Comment #3
Wim Leers#2885372: Can't PATCH User entity's 'mail' field via JSON API will get clarity thanks to this issue.
Comment #4
Wim LeersThis will likely end up being blocked on #2882451: Make the response consistent ordered..
Comment #5
Wim Leers#2841851: Audit cacheable metadata generation will also get clarity.
Comment #6
Wim LeersThis is now my #1 priority. Starting to actively work on this. Finally! 🎉
Comment #7
Wim LeersComment #8
e0ipsoYES! 💪🏽
Comment #9
Wim LeersI've started with
Node
, because that's the most commonly used entity type. I'm essentially porting:\Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase
and its parent class 1:1 to\Drupal\Tests\jsonapi\Functional\ResourceTestBase
\Drupal\Tests\rest\Functional\EntityResource\Node\NodeResourceTestBase
1:1 to\Drupal\Tests\jsonapi\Functional\NodeTest
(I started with the test coverage before #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior landed, and that issue added crucial new test coverage, which we should port over.)
I've already made some key simplifications:
basic_auth
right now?_format=api_json
wherever possible, because this is pretty annoying and very much a Drupalism. This works fine, except for 404s, where I'm testing explicitly without?_format=api_json
(HTML response) *and* with (JSON API response), to make the difference abundantly clear. Hopefully we can improve this behavior in the future.Lots and lots of things aren't yet tested.
GET
andDELETE
are mostly complete. ForGET
, I already got one crucial bugfix committed: #2933515: JSON API does not support HEAD requests. #2933939: JSON API module must not send cacheable responses to PATCH, POST and DELETE requests is something I encountered while working on theDELETE
test coverage. Both have plenty of@todo
s.For
PATCH
andPOST
, I'm running into so many bugs that block progress (even with significant portions of their test coverage commented out!) … that I decided to just post a failing patch here, and then work on bugfixes that allow more of the test to become green. Creating patches with explicit test coverage for each of those bugfixes is going to be extremely time consuming. So I propose to be pragmatic and let bugfixes get committed if they allow the integration tests being added here to become green (or fail on a later assertion in the test scenario).Comment #10
Wim LeersCreated #2934149: [>=8.5] JSON API routes not specifying _content_type_format route requirement, resulting in bad DX for the next
POST
test coverage blocker. But turns out there's also a core bug that only appears in the specific way that JSON API has its routes defined. 😩😩😩Comment #11
Wim LeersIf #2934149: [>=8.5] JSON API routes not specifying _content_type_format route requirement, resulting in bad DX landed, we could've done this. But until #2934152: ContentTypeHeaderMatcher should not run for GET, HEAD, OPTIONS or TRACE requests lands, #2934149 won't be able to land.
So, alas, we can't bring back some of the commented test coverage quite yet.
Comment #12
Wim LeersCreated #2934164: \Drupal\jsonapi\Controller\RequestHandler::deserializeBody() should not use the format corresponding to the Content-Type request header's value, to fix not the root cause, but only the direct cause. Also committed it.
Reuploading+retesting #9's patch should show a different error now.
Comment #13
Wim Leers#9:
#12:
Now the test is failing because JSON API it receives in invalid request body. So let's explicitly test that error response, and fix the request body being sent, so that that failing assertion can finally test what it's supposed to test: the validation error when sending multiple values for a single-value field.
Comment #14
Wim LeersAs expected:
Hurray, that is the error we need! It just is formatted in a different way. That should be easy enough to fix. Continuing tomorrow :)
Comment #15
e0ipsoI could not do a proper review, this is what I had time to do:
Why are we losing the static cache? Maybe an attr mutator will get you where you need to be?
Although this looks good I wonder if we can just have a folder with
./snapshots/node--lama.json
.That would enhance readability. On top of that we would be speaking the same encoding format we'll eventually have.
The only downside is the inability to add comments to JSON documents.
Should this be
testPatchPath()
?Is it possible to subclass
Drupal\Tests\rest\Functional\ResourceTestBase
to get some code reuse?Comment #16
Wim Leers#15 is exactly why I didn't want to post a patch yet… I thought #9 made that clear? I guess I wasn't explicit enough — my bad! :(
So let's make it clear for now: This patch is not remotely ready for reviews yet!
I will indicate it with a similarly clear message stating the opposite when it is ready to be reviewed.
Since you've already spent some time reading the patch, I'll answer your specific questions, but please don't spend more time reviewing, since it's going to undergo massive changes still.
camelids
NodeType
is created in the test'ssetUp()
method, but even manually invoking the route builder wouldn't cause its JSON API routes to be generated, due to this static cache.PATCH
ing of thepath
field which is a special snowflake. But as I already indicated in #9, this has not yet been activated, since I'm still struggling to get the basicPATCH
andPOST
test coverage to work, due to the many bugs I'm already encountering. That's what I've been pushing forward in the comments since #9.rest
module into theserialization
module. This also applies to non-test code like theResourceResponse(Subscriber)
classes.Comment #17
e0ipsoDon't worry. I ignored the warnings to see the direction things are going (which I like very much). Ignore my comments, I just got curious about stuff.
Comment #18
Wim LeersCool :)
I can't wait for you to review this patch, but until it's in a better place, it's going to be a frustrating waste of time! (And I see how the frustration is showing in #16.)
Working to get to that place :)
Comment #19
Wim LeersWhile working on this, discovered another problem, this time only a soft bug: #2934362: Specify a "code" for every exception that JSON API throws. Made it a child issue of this issue.
Comment #20
Wim LeersAddressed #14 in #2934370: Entity validation constraint violation messages contain JSON-encoded HTML (a new child issue).
This patch is a straight rebase of #14 (several conflicts had to be resolved). The failing assertion causing #14 is now as simple as it should be, but still had to be updated to account for the
['source'=>['pointers'=>'/data/attributes/title']]
expectation.This test coverage patch now fails on the next assertion: again one step further :)
I'll still need to improve
\Drupal\Tests\jsonapi\Functional\ResourceTestBase::assertResourceErrorResponse()
to allow'source'
to be specified, and'links
to be omitted for 422 errors (per #2934362-14: Specify a "code" for every exception that JSON API throws.4), but keeping that refactoring for a separate interdiff.Comment #21
Wim LeersThe next failing assertion is
This should fail, but in JSON API it results in a 201 response. Why?
It appears to be because JSON API ignores the UUID it receives. In a particular test run, the (random and invalid because 129 characters) UUID
d8cdc218-5d03-4b43-a4bc-4dbbba71aa07
was sent, but the 201 response contained the (generated on the server side)40fffa92-7406-47f7-bdbd-fcb8e5b70089
UUID. Which is why it was able to result in a 201 response.I already encountered a similar bug in the
GET
test coverage, which I worked around by simply commenting the relevant assertion for now:We'll have to solve this at some point, might as well solve it now. There may be a valid reason for JSON API disallowing letting clients specify a UUID when creating entities, but there is no valid reason why deserializing on the server side results in a different UUID: that should result in an object where all fields match those in the given normalization.
Created #2934386: Accept client-generated IDs (UUIDs) for investigating that.
Comment #22
Wim LeersSo, if I comment the assertions blocked on #2934386: Accept client-generated IDs (UUIDs), and also the assertions that follow it that are blocked on #2934149: [>=8.5] JSON API routes not specifying _content_type_format route requirement, resulting in bad DX (see #10), then we're getting to the point where a successful
POST
request is being made!Sadly, that assertion fails because there are cache tags and cache contexts in the response, even though there shouldn't be because it's a response to an unsafe (and hence uncacheable) HTTP request! Which brings us back to #2933939: JSON API module must not send cacheable responses to PATCH, POST and DELETE requests.
I expect this to fail with:
Line 397 does
$this->assertSame($expected_cache_tags !== FALSE, $response->hasHeader('X-Drupal-Cache-Tags'));
— there shouldn't be a cache tags header, but there is. Which will be fixed by #2933939.Comment #23
Wim LeersBTW, we're almost at the end of
testPostIndividual()
. The patches so far contained a full copy-pasted version of REST'sEntityResourceTestBase::testPost()
. But most of the remaining LoC (the ones after the line we're currently failing on) are actually testing REST-specific BC stuff. Which we don't need to test here! :) So let's remove those.Comment #26
Wim LeersThis updates the assertions at the very end of
testPostIndividual()
, so that they make sense for JSON API. In doing so, I started using the@jsonapi.entity.to_jsonapi
service. And that's how I ran into a known bug with that again: #2925043: Server error when using the jsonapi.entity.to_jsonapi service. But I can work around that for now!testPostIndividual()
is now passing tests! 🎉Comment #28
Wim LeersAlright, so now that the work on the Drupal core 8.5.0-alpha release mostly complete, shifting attention back to JSON API.
Next up:
testPatchIndividual()
. Let's start with commenting the assertions that are similar to the ones intestPostIndividual()
that are also commented. Those edge cases didn't work in the latter (POST), they also don't work in the former (PATCH).To my great relief, at least the most crucial security-related test coverage that ensures that fields that cannot be
PATCH
ed (because the fields are read-only) is passing!Well, it's not quite passing just yet: JSON API helpfully indicates the reason why you are not allowed to
PATCH
a field. This is actually a great DX enhancement, that is missing in core's REST module! I created an issue to add it there: #2938035: When PATCHing a field is disallowed, no reason is given for *why* this happens 🤘. I will have to change the tests a bit to accommodate this, see the next comment/patch.That's why this patch should fail with:
Comment #30
Wim LeersGreat, now we're testing that those 403 responses for
PATCH
ing fields do contain helpful information explaining *why*!We're now getting stuck on the thing we're testing just after that, which was fixed in core's REST module the day before I started working on this issue (see #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior): when the fields sent in a
PATCH
request match the currently stored values, we should allow those to be sent, and not complain that the user is not allowed to modify the field … because they're not trying to modify the field!EDIT: note that the core equivalent of this exact interdiff was posted at #2938035-9: When PATCHing a field is disallowed, no reason is given for *why* this happens!
Comment #32
Wim LeersNote that in working on a patch for #2938035: When PATCHing a field is disallowed, no reason is given for *why* this happens, I discovered a core bug that also affects JSON API and will cause us to get blocked pretty soon: #2938053: AccessResult::orIf() fails to retain the reason if both operands are neutral or forbidden, but the first contains a reason and the second one does not.
Comment #33
Wim LeersThis led me back deep into computed field hell, where we recently fixed bugs in core (see #2916300: Use ComputedFieldItemListTrait for the path field type and related issues) to even do #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior in core in the first place … which after figuring out #2939800: FieldNormalizerValue::rasterizeValue() respects cardinality === 1, but doesn't verify that this is true (making JSON API more robust) led me to the root cause: a bug in core's test coverage that I'd copied over to the test coverage here: #2939802: Follow-up for #2824851: EntityResourceTestBase::getModifiedEntityForPatchTesting() handles @FieldType=path incorrectly. The entire reason that #2939802 was even able to happen is because of Field API's absence of triggering exceptions when doing invalid things… It's really turtles all the way down. Building on weak foundations has ripple effects throughout, even after having done all the work of making REST have very very extensive integration test coverage, we still manage to find edge cases 😩
IOW: this made me:
Finally, as explained in #30, we still need to port over the changes from #2824851 in core to JSON API, but for that we need test coverage … which is what this issue is adding. So rather than modifying JSON API's logic, I split that out to a separate issue: #2939810: EntityResource::patchIndividual() should allow fields that the user is not allowed to change, as long as they match the current value, so that this issue can stay focused on adding test coverage with the absolute minimum of changes made to JSON API's behavior/code.
This tiny interdiff took ~4 hours…
Now that this done, the next challenge is:
because the response contains a cache tag header:
X-Drupal-Cache-Tags: http_response node:1
. In other words: JSON API returns responses that implementCacheableResponseInterface
, which indicates that respones to the unsafePATCH
method are cacheable … which of course does not make sense. Should not be too hard to fix.Comment #35
Wim LeersActually, I already ran into this problem in #9. It's why I created #2933939: JSON API module must not send cacheable responses to PATCH, POST and DELETE requests. Let's not fix that now, let's work around it, just like I did before.
This then also:
PATCH
what #22 did forPOST
wrt #2934149: [>=8.5] JSON API routes not specifying _content_type_format route requirement, resulting in bad DXfield_rest_test
andfield_rest_test_multivalue
test coverage, which we want/need, but let's get to green firstjsonapi.entity.to_jsonapi
service, which cannot really be moved to a separate issue because the test coverage that #2874509: Provide service to simplify generating a JSON API representation of an entity in PHP code introduced specifically mocks away the bit that is broken, which is why this easily goes unnoticed…testPatchIndividual()
is now passing tests! 🎉Comment #36
Wim LeersHAH, the tests are failing on 8.6 because of #2891215: Add a way to track whether a revision was default when originally created, which I asked to be reverted… and so now it's impacting JSON API as well. Working on a fix.
Comment #38
Wim LeersFixed that. This patch will now only pass on 8.5.x and 8.6.x core, until that's fixed in core, which will happen in #2937850: Mark revision_default as internal for REST consumers.
Comment #39
Wim LeersAnd the remaining fail for
GET
is due to #2923779: Normalizers violate NormalizerInterface: one must no longer use the@serializer
service to denormalize JSON API normalizations!Comment #42
Wim LeersI decided to first expand to the
Term
test because that's the other test that has explicit test coverage for thepath
field, which theNode
test coverage we've been working on so far also contains, but it's currently still commented.This:
\Drupal\Tests\jsonapi\Functional\NodeTest::testPatchPath()
test coverage that was previously commented, making theNode
test coverage more complete (all test methods that core REST also has!)\Drupal\Tests\jsonapi\Functional\TermTest
, including thetestPatchPath()
andtestGetIndividualTermWithParent()
methods that test edge cases that are unique to theTerm
entity, just like core REST#2940336: JSON API incorrectly requires the "access content" permission for *all* routes
#2940339: Port reference field support for non-empty entity reference fields not pointing to an entity from #2543726
#2940342: Cacheability metadata on an entity fields' properties is lost
Comment #43
Wim LeersFFS d.o … #42 did succesfully attach both a patch and issue relationships, but they don't show up in the comment. 😡
Comment #44
Wim LeersThanks to #42, also adding test coverage for the
EntityTest
was fortunately easy, and did not uncover more unknown bugs.Comment #46
e0ipsoI just read all the observations made here. Great process and better progress. Let me know when this can benefit from a code review.
Comment #47
Wim LeersLater this week hopefully :)
Comment #48
Wim LeersI said "later this week hopefully", but sadly that did not happen. I was asked to help out with critical Workflow Translation issues for much of last week. But I did work on this a lot, specifically on
Comment
entity's test coverage (andUser)
. I ran into lots of problems for both.Comment
test coverage is actively being worked on.Comment
!).ResourceTypeRepository
had to be updated (which is fine), but now causes infinite recursion due to changes elsewhere. All due that annoying static cache. That's because::all()
ends up calling::get()
which ends up calling::all()
(since #2937961: ResourceType should provide related ResourceTypes). Seeinterdiff-static-cache.txt
for that, which shows the new work-around relative to HEAD.Those were easy to fix. However, they came with consequences:
POST
normalizations were succeeding even though they were invalid (Node
contained a resource ID,Term
contained an invalid resource type)POST
ed resources, which is fine, but it also uses those auto-generated UUIDs in 403 responses, which is not fine). Instead of adding work-arounds in this patch, I figured I'd just create an issue to fix it separately: #2942243: Follow-up for #2934386: server-generated IDs for newly POSTed entities should not be exposed in 403 responsesPlus, I forgot to mention this in #44, but that in fact did uncover another unknown bug, I just hadn't created an issue for it yet:
… and I now had to duplicate this work-around to
TermTest
too. Created #2942255: Spec Compliance: Even when successfully POSTing or PATCHing an entity, you can still get a 403 response when you cannot GET it! for that.Comment #49
Wim LeersRetesting, testbot had database problems …
Comment #50
Wim LeersWhew 😓, here's
Comment
, that was … hard. I lost count of how many hours I spent on this one. The better part of a week.PATCH
-protected fields that happen to be relationships in JSON API (entity reference fields in Drupal) — we already have #2939810: EntityResource::patchIndividual() should allow fields that the user is not allowed to change, as long as they match the current value for that.\Drupal\Tests\jsonapi\Functional\CommentTest::testPostSkipCommentApproval()
made me discover #2942561: Assert denormalizing the JSON API response results in the expected object, which fortunately need not be a blocker. Still, that's a pretty flabbergasting find.There are two bits of wonderful news here:
So I'm hopeful progress will be faster from here on out :)
The attached patch should fail because #2942255: Spec Compliance: Even when successfully POSTing or PATCHing an entity, you can still get a 403 response when you cannot GET it! is not yet committed.
Comment #51
Wim LeersThere's the expected failure:
I just committed #2942255: Spec Compliance: Even when successfully POSTing or PATCHing an entity, you can still get a 403 response when you cannot GET it!, now re-testing #50, that should now pass!
Comment #52
Wim LeersHurray, #50 now passes as predicted!
And #2942255 having landed also allows me to remove the work-arounds that did work for other entity types.
Comment #54
Wim Leers#2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method added test coverage for the following content entity types:
Node
User
Term
EntityTest
Comment
All other content entity types were deferred to follow-ups. I propose we do the same here. So that only leaves
User
! Which is the last content entity type with a number of very special edge cases… let's dive right in.Testing
GET
,PATCH
,POST
andDELETE
for theUser
content entity type was easy. There was only one change that I had to make toResourceTestBase
, to ensure that its uses of the@jsonapi.entity.to_jsonapi
service were using the intended account, and hence respected field access correctly. (TheUserAccessControlHandler
does some fairly special things.)But then there were also the two additional pieces of test coverage:
testPatchDxForSecuritySensitiveBaseFields()
, added back in Q4 2016, in the original REST test coverage issue (#2737719). Getting this to work was easy — no new bugs discovered!testPatchSecurityOtherUser()
, added only recently, in #2885469: Regression: manually setting the ID field for newly-created content entities is not possible anymore (public follow-up to SA-2017-002). Thanks to fixes in Entity API, JSON API is secure, but it is not able to return the appropriate error response just yet. Due to a bug in JSON API's handling of denormalizing fields. Fortunately, it's a consequence of an existing problem: #2939810: EntityResource::patchIndividual() should allow fields that the user is not allowed to change, as long as they match the current value. See #2939810-6: EntityResource::patchIndividual() should allow fields that the user is not allowed to change, as long as they match the current value for details. Once that is fixed, this will behave entirely as it should, and we have the test coverage to prove it!Now on to the config entity types (#2737719 did
Block
,ConfigTest
,Role
andVocabulary
), and then on getting the commented assertions uncommented, then this will be ready!Comment #55
Wim LeersWhile working on the
Block
test coverage, I found a significant bug in the normalization of config entities. #2733097: [FEATURE] Add GET support for configuration entities added config entityGET
support, but it did not add any integration test coverage. The only test coverage is\Drupal\Tests\jsonapi\Kernel\Normalizer\JsonApiDocumentTopLevelNormalizerTest::testNormalizeConfig()
, but that didn't verify that a config entity'sdependencies
key is exposed correctly.This is what we'd expect:
but in reality we get:
Obviously that's losing very important information!
Added a work-around for that. To fix this bug, I created #2942979: JSON API's normalization of config entities' "dependencies" key-value pair omits essential information when there's only a single dependency.
The test coverage for
ConfigTest
,Role
andVocabulary
did not suffer from this because none of them have dependencies!Comment #56
Wim LeersHurray, testbot agreed! Now down to the last stretch:
— this means going through this patch's\Drupal\Tests\jsonapi\Functional\ResourceTestBase
class and looking at every thing that is commented out: it should no longer be commented out, otherwise the test coverage is incomplete!That's for tomorrow :)
Comment #57
Wim LeersA first round of analyzing commented code, and attempting to fix it, remove it, or otherwise, create an issue for it.
We have #2942561: Assert denormalizing the JSON API response results in the expected object for that, created in #50.
Updated patch to point to that.
… and now it's clear we're not going to be testing that at all. Because we're only testing
basic_auth
. So, removed that.because:
In other words: when there's no request body for a
POST
orPATCH
request, JSON API has a fatal error instead of sending the appropriate error response.For the latter portion (400 when unparseable request body), I created #2943165: Failing to decode should result in a 400 response, failing to denormalize should result in a 422 response. For everything before it, I created #2943170: JSON API's RequestHandler causes fatal PHP error when a PATCH or POST request has no body.
Comment #58
Wim LeersTime for some long-overdue simplification of the error response assertions. Those assertions are rather verbose and complex, because JSON API error objects (http://jsonapi.org/format/#error-objects) are very rich.
In trying to come up with one simpler assertion helper method, I discovered a few bugs that made it unnecessarily difficult to assert those error objects:
Attached is a patch that:
Comment #59
Wim LeersI missed one spot :(
Comment #60
Wim LeersCorrection, I missed one test method, in which there were two spots. Gah!
Also fixing two nits at the same time.
Comment #61
Wim LeersUncommented all the
field_rest_test
test coverage, to test field access edge cases! Happy to say that this was mostly without problems. I stumbled upon #2938053: AccessResult::orIf() fails to retain the reason if both operands are neutral or forbidden, but the first contains a reason and the second one does not as predicted more than 3 weeks ago in #32 and #2938035-6: When PATCHing a field is disallowed, no reason is given for *why* this happens, but fortunately I can work around that for now (yet another@todo
that we can remove in the future!).Also, a minor clean-up issue for the REST module so that the JSON API module can reuse core's
rest_test
module without awkward consequences (i.e. therest
module being installed forjsonapi
tests … 🤐): #2943338: rest_test module for testing REST module could be reused by contrib JSON API module.Next up: two types of simplifications/nits that are occurring frequently throughout the patch:
$this->serializer->encode(…, 'api_json')
should really becomeJson::encode(…)
everywhere to KISS@todo
s everywhere.Comment #62
Wim LeersI had tested the individual CRUD operations on
NodeTest
, but didn't run the entire test suite. Hence #61 failed in some cases. Here are the fixes.Comment #63
Wim LeersFixed
Comment #64
Wim LeersThis patch has >300 coding standards violations. Fixed them all.
Comment #65
gabesullice🤖💪❤️
Comment #66
Wim LeersReplacing all
json_encode()
withJson::encode()
.Comment #68
Wim LeersWe actually already have issues for them and it won't make sense analyzing those further. Fixing the issues the existing
@todo
s are referring to will automatically ensure we need to update existing assertions.So that means we're ready for final review! Let's get this done, because it blocks many issues!
…
Except #66 is now apparently failing on
InternalEntitiesTest
, which was added earlier today. Gah! Looking into that…Comment #69
Wim LeersTurns out #2939705: Do not include internal resource types under `include` broke JSON API on Drupal 8.6. Reverted it: #2939705-38: Do not include internal resource types under `include`.
Resting #66, should be green!
Comment #70
gabesullice👏 ... ... ... 👏... ... ... 👏... ... 👏... ... 👏...👏... 👏👏👏👏👏👏👏👏👏👏👏👏👏
Wow, this is amazing and immaculate. This is as HUGE step forward for the module and will unblock so many more issues and improvements.
In comparison, my nits, rants and suggestions seem so insignificant, but here they are:
supernit:
s/a HTTP/an HTTP/
Why does this need to be public?
Just curious, how is this different from
EntityTestBundle::create()->save();
I think this should be changed to
getExpectedNormalizedDocument
or we shouldn't be returning the envelope around thedata
values.This is very similar to the above. We should not wrap the normalization of entity with the
data
key.The only thing that a "normalized entity" can be is a JSON API resource object.
Too often we indicate in code, comments or tests that the document and/or the data key are in a 1:1 correspondence with entities, which isn't the intention of the spec at all.
The following JSON is only ever an envelope which may contain entities represented as "resource objects" or "resource identifiers".
</rant>
Should all of these be
field_jsonapi_test
?Well, that's depressing.
Hate to keep beating a dead horse, but all of these should recognize that a normalized entity != a JSON API document, but a resource object.
Unfinished comment?
Should we have a follow-up to sort this data in the normalizers?
Why not
$field->target_id + 1
?I'm not gonna mark this as NW, because I think this is definitely still ready for a review by @e0ipso.
Comment #71
e0ipsoI have some comments. But I want to take the time to review appropriately. I've been following the issue and reading the careful reporting by Wim.
Also… this happened behind your back Wim.
Comment #73
e0ipsoSetting to Needs Work as per #70.
Comment #74
Wim Leers@e0ipso teased me on Twitter:
To which I replied:
I'm still not sure which I'm more: surprised or grateful :P Definitely thank you to both of you!
I will address #70, which contains lots of great remarks. But first, because this is already committed, I want to make the 8.x-1.x branch pass on 8.4.x again. This patch was only passing against Drupal core 8.6.x!
Comment #75
Wim LeersThe committed test coverage doesn't specify the
Accept: application/vnd.api+json
request header anywhere, yet it does result in 4xx responses in that format. This works in 8.5.x/8.6.x thanks to #2858482: Simplify REST routing: disallow requesting POST/PATCH in any format, make consistent fixing significant bugs in the routing system (it changes incorrect priorities in route filters, to correct ones).It's better to always send that
Accept
request header — in fact, the JSON API spec demands us to do so. http://jsonapi.org/format/#content-negotiation-clients says:Comment #76
Wim LeersTermTest::testGetIndividualTermWithParent
cannot possibly pass on 8.4.x because it was added in #2543726: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration, which was committed only to 8.6.x.Comment #79
Wim LeersFor the same reasons as #75, we need to change the 404 test coverage that was added in the initial commit.
Comment #81
Wim LeersThe failure in
VocabularyTest::testPostIndividual()
is because 8.4 doesn't have #2808217: To be able to view Vocabulary config entities via REST, one should not have to grant the 'administer taxonomy' permission, that was committed to 8.5.Comment #82
Wim LeersAnd finally, the failures wrt
field_rest_test_multivalue
occur because #2928702: Make EntityResourceTestBase's field_rest_test_multivalue test field less invasive: omit it from normalizations only landed in 8.5.Comment #85
Wim Leers3 remaining failures, all due to things that aren't fixed in 8.4 yet, but are in 8.5:
EntityTestTest
: #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts added ainternal_string_field
field to test internal vs non-internal — this will come in handy in #2921257: On Drupal 8.5, JSON API should respect Typed Data's new ::isInternal(), and JSON API Extras should use ::setInternal() (we already have a todo pointing to that)NodeTest
: #2891215: Add a way to track whether a revision was default when originally created added this field.TermTest
: #2543726: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration fixed theparent
field, in 8.4 we need a different expectation.Comment #87
Wim LeersForgot one:
TermTest::testPostIndividual()
, for similar reasons as #81.Comment #89
Wim LeersI posted #87 too quickly.
Comment #90
Wim LeersClean-up.
#75 made me realize I could simplify
ResourceTestBase::getAuthenticationRequestOptions()
: it doesn't actually need that parameter!Comment #91
Wim LeersI'm committing #90, because the commit in #72 broke testing on 8.4, and committing #90 now minimizes the duration of JSON API not passing tests on 8.4.
#89 and #90 both passed tests on 8.4. Now testing #90 against 8.6. If that also passes tests, I'm committing it :)
Comment #93
Wim LeersBack to NW for addressing #70.
Comment #94
gabesulliceI had no idea Drupal::VERSION existed. Good to know.
Comment #95
gabesulliceI don't know what happened with my comment. I hadn't refreshed the page since yesterday and somehow it managed to delete files, (??) I didn't do that on purpose.
Comment #96
Wim LeersHaha, no worries. Sometimes this weird stuff happens :)
#70:
ResourceTestBase
to call this, instead of having to duplicate this 100%.Is this too much magic? Yes. Do I understand it? No. Is it out of scope to make this better? Yes.
field_httpapi_test
andfield_httpapi_test_multivalue
. That also why I created #2943338: rest_test module for testing REST module could be reused by contrib JSON API module, which already performs a first baby step (reviews welcome!).Because
target_id
could also be a string, and you can't increment a string. The point is that we pick a non-existing target ID, and hardcoding a value that is never going to exist and can be used as both a string and an ID adequately addresses that. Full history in #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior.Comment #97
gabesulliceThis looks awesome. I really do feel passionately about the document vs. entity thing, so I love this clarification :D
I know it was a tedious change though, so thank you :)
Re: #10, My main concern wasn't really that it makes our tests harder, but that it makes all comparisons harder because it's non-deterministic. It makes a simple string/hash comparison from a CURL impossible. That means you can't do really quick tests for things like a field being changed/removed etc. Not important for this issue though.
Comment #98
Wim LeersThanks to PHPStorm's refactoring feature, it wasn't tedious at all actually :)
As long as the order is consistent, that's not true. You can totally diff. In fact, that's what I do all the time while working on these tests: when the expected
normalizationdocument doesn't match the actual one, I diff.And YAY! Up to you or @e0ipso now to commit this :)
Comment #100
gabesullice💥
I just had to get in on this party :)
Comment #101
Wim LeersWonderful! See you in #2932031: Comprehensive JSON API integration test coverage phase 2: for every entity type, complex use-cases!