Add JSON API basic support (continuation) #1175

meyerbaptiste · 2017-06-13T14:55:01Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	api-platform/api-platform#82
License	MIT
Doc PR	N/A

Continuation (and conclusion?) of PRs #785 and #1036.

First, I rebase the branch against the master branch (:cold_sweat:).
Then (see the second commit):

I fixed some issues according to the JSON API schema definition,
I fixed Behat tests,
I added some PHPUnit tests,
I added the enabling of the PATCH operation only if JSON API support is enabled,
I removed dead code.

GTK:

This implementation is very, very basic,
IMO, normalizers could be refactored in the future.

Did I forget something?

Cc: @hectorh30, @Simperfit

Simperfit · 2017-06-13T14:57:54Z

src/JsonApi/EventListener/TransformSortingParametersListener.php

+        // If order query parameter is not defined or is already an array, never mind
+        $orderParameter = $request->query->get($this->orderParameterName);
+        if (null === $orderParameter || is_array($orderParameter)
+        ) {


strange cs?

soyuka · 2017-06-13T15:28:18Z

First, I rebase the branch against the master branch (:cold_sweat:).

Wow! For this you deserve much ❤️! To bad that authors history was lost though. Really nice if this can finally come to an end :D. Thanks @meyerbaptiste!

Simperfit · 2017-06-13T15:32:05Z

@soyuka Yep, anyway the author are in the files and we could @them when mergin :).

Thanks @meyerbaptiste for finishing this !

soyuka · 2017-06-13T15:32:15Z

src/JsonApi/EventListener/TransformSortingParametersListener.php

+ *
+ * @author Héctor Hurtarte <hectorh30@gmail.com>
+ */
+final class TransformSortingParametersListener


It feels like those parametersListener have a lot of code duplication. I don't know if it's okay like this or if we should base those on an abstract class. WDYT?

soyuka · 2017-06-13T15:35:37Z

src/JsonApi/Serializer/CollectionNormalizer.php

+        $returnDataArray = [];
+        if (isset($context['api_sub_level'])) {
+            foreach ($data as $index => $obj) {
+                $returnDataArray['data'][][$index] = $this->normalizer->normalize($obj, $format, $context);


Shouldn't the $returnDataArray be like $returnDataArray = ['data' => []] on initialization for this to work? I don't get how ['data' => [[]]] is self-initializing.

The magic of PHP: https://3v4l.org/GXaOT

Wow okay, I feel dumb right now, thanks @meyerbaptiste!

soyuka · 2017-06-13T15:36:21Z

src/JsonApi/Serializer/CollectionNormalizer.php

+            $lastPage = $data->getLastPage();
+            $itemsPerPage = $data->getItemsPerPage();
+
+            $paginated = 1. !== $lastPage;


1. ? Because of float comparison? Saw that below too.

Yes, I think. $lastPage is a float.

https://github.com/api-platform/core/pull/1175/files#diff-5b154fa3efaf5776695baccd0a52daf4R66 initialized as integer? IMO pagination stuff should be integer-only because it doesn't make sense to have a float ^^ (maybe jsonapi specifies float though I haven't looked at it).

I think they wanted to keep a consistency with the PaginatorInterface interface which returns floats.

Indeed. We use float because of large numbers.

soyuka · 2017-06-13T15:41:44Z

src/JsonApi/Serializer/ItemNormalizer.php

+
+                $typeShortName = '';
+
+                if ($className && $this->resourceClassResolver->isResourceClass($className)) {


Because isResourceClass is quite heavy (#1068) I'd prefer if we could use a variable to store it above instead of calling it a least twice.

I refactored this part.

soyuka · 2017-06-13T15:43:02Z

src/JsonApi/Serializer/ItemNormalizer.php

+     *
+     * @return bool|string
+     */
+    private function getJsonApiCacheKey(string $format = null, array $context)


method typing :bool?

? This method returns a string or false.

wow, my bad missed that.

soyuka · 2017-06-13T15:46:20Z

src/Swagger/Serializer/DocumentationNormalizer.php

@@ -183,6 +183,8 @@ private function getPathOperation(string $operationName, array $operation, strin
                return $this->updateGetOperation($pathOperation, $mimeTypes, $operationType, $resourceMetadata, $resourceClass, $resourceShortName, $operationName, $definitions);
            case 'POST':
                return $this->updatePostOperation($pathOperation, $mimeTypes, $operationType, $resourceMetadata, $resourceClass, $resourceShortName, $operationName, $definitions);
+            case 'PATCH':
+                $pathOperation['summary'] ?? $pathOperation['summary'] = sprintf('Updates the %s resource.', $resourceShortName);


interesting hack 😛 we could break after this IMO so that it gets to the return statement directly.

Why adding a break here? I have to return in the PUT case.

Oh okay then thought it returned as default.

https://github.com/api-platform/core/pull/1036/files#diff-32d63273c86a7e4c14c3fa6bb4131194

👍 better like you did here indeed.

soyuka · 2017-06-13T15:48:39Z

tests/Hydra/Serializer/ItemNormalizerTest.php


        $normalizer = new ItemNormalizer($resourceMetadataFactoryProphecy->reveal(), $propertyNameCollectionFactoryProphecy->reveal(), $propertyMetadataFactoryProphecy->reveal(), $iriConverterProphecy->reveal(), $resourceClassResolverProphecy->reveal(), $contextBuilderProphecy->reveal());

        $this->assertFalse($normalizer->supportsDenormalization('foo', ItemNormalizer::FORMAT));
-        $normalizer->denormalize(['foo'], Dummy::class, 'jsonld', ['jsonld_has_context' => true, 'jsonld_sub_level' => true, 'resource_class' => Dummy::class]);


why did you drop this test?

Huum, I think it was because the test is about denormalization support and not denormalization.

Yeah it's weird indeed, test is testDontSupportDenormalization. Then we still do the denormalization with no assertions ?!

Still, denormalize isn't tested anymore in this class, maybe we could move it in a new test with an assertion then?

Yep, you're right.

Then we still do the denormalization with no assertions

It's wrong, because shouldBeCalled() is an assertion 😛

haha yeah I meant no assertion on the denormalization call like we do on normalization below.

soyuka · 2017-06-13T15:49:11Z

tests/JsonApi/EventListener/FlattenPaginationParametersListenerTest.php

+
+        $this->listener->onKernelRequest($eventProphecy->reveal());
+
+        $expectedRequest = new Request(['page.size' => 5, 'page.number' => 3]);


we could also test negative page parameters values to avoid a recent issue we had.

Okay. But at this level, positive or negative, it has no impact.

Simperfit · 2017-06-14T07:55:05Z

@meyerbaptiste could you push it a little bit further by adding that 0.6% coverage missing ?

meyerbaptiste · 2017-06-14T15:19:15Z

src/JsonApi/Serializer/CollectionNormalizer.php

+        $returnDataArray = [];
+        if (isset($context['api_sub_level'])) {
+            foreach ($data as $index => $obj) {
+                $returnDataArray['data'][][$index] = $this->normalizer->normalize($obj, $format, $context);


I'm really not confident with this piece of code. I don't think it works properly but at the same time, I don't know what is the correct result. I am not familiar with sub-resources...
Anyone can help me here?

Looks correct according to what are the other normalizer's doing in case of a api_sub_level flag.

core/src/Hal/Serializer/CollectionNormalizer.php

Line 60 in b88d975

if (isset($context['api_sub_level'])) {

core/src/Hydra/Serializer/CollectionNormalizer.php

Line 65 in 183b0a5

if (isset($context['api_sub_level'])) {

It's more about the array structure. What type of values can be the index and the normalized object here? According to JSON API spec, data must be primary data or represent resource linkage, we can not put what we want in it! 😕

meyerbaptiste · 2017-06-16T14:26:28Z

I got +0.1% @Simperfit! I don't want to write tests for at least 1 month...

Simperfit · 2017-06-16T14:27:02Z

it's good for me ;)

hectorh30 · 2017-06-19T05:50:23Z

Awesome! Thanks so much for going on with this. I also went out of time to complete this, but I'm glad to see it moving forward.

meyerbaptiste · 2017-10-13T15:08:48Z

Thanks @Simperfit & @hectorh30, 4 Oct 2016 -> 13 Oct 2017, 1 year of collaborative work!

Simperfit · 2017-10-13T15:09:13Z

Thank you for finishing it !

soyuka · 2017-10-13T15:28:15Z

It's MERGGEEEEDDDD ❤️ ❤️ ❤️
big thanks @Simperfit @meyerbaptiste @hectorh30 and every one who reviewed this!

StephenOTT · 2017-10-13T16:15:49Z

@mario-digitalstate

* GraphQL Query support (api-platform#1358) * Fix missing service when no Varnish URL is defined * [PropertyFilter] Fix whitelist comparison (api-platform#1379) * Remove wrong doc * Swagger subcollection documentation issue (api-platform#1395) * Make the requirements configurable Closes api-platform#1401 * Provide a better exception message and type Getting "Not found" on a route where you are getting an object can get really confusing. * Bump branch alias to 2.2.x-dev 2.1.x-dev is already taken by the 2.1 branch, and this should represent the next minor version anyway. * Fix tests * Reuse PriorityTaggedServiceTrait from symfony * Improve payload support and remove duplicate code in ConstraintViolationListNormalizer (api-platform#1416) * Filter Annotation implementation Parent class filters (needs test) Support for nested properties Tests Fix some comments and remove id=>id in compiler pass * Throw on abstract data providers / filters * Remove an unused var * Remove useless badges * Enable the coverage * Fix some quality issues * Add job to test upstream libs deprecations If api-platform uses upstream libs in a deprecated way, we should be aware of it. * Add job to test upstream libs deprecations If api-platform uses upstream libs in a deprecated way, we should be aware of it. * Fix missing cache tag on empty collections * Allow plain IDs with `allow_plain_identifiers` * Fix indentation for GraphQL features. * Add JSON API basic support (api-platform#785, api-platform#1036, api-platform#1175) * Clean Behat tests * Fix tags addition with an empty value * Document swagger-specific description options … and where to find them should there be newer ones. * Fix PHPUnit tests * Fix missing return statement * Support & compatibility for PHP7.2 * Add feature to update swagger context for properties * Generator compat improvements (api-platform#1429) * Add support for resource names without namespace * [SF 4.0] Make actions explicitly public * Allow phpdocumentor/reflection-docblock 4 * fix hydra documentation normalizer with subresources * Create a base collection normalizer * Fix request auto-runner * Update the changelog * Update changelog

jonday-simitive · 2017-12-01T11:33:45Z

Well done to everyone that helped out with this. Looks really great, been testing it out since it was merged in and great to see it has made it into a release. Is there any plans on getting include working http://jsonapi.org/format/#fetching-includes ... If not I'd be interested in contributing as it is one of my favorite parts of JSON API.

soyuka · 2017-12-01T13:29:30Z

This looks really similar to what our Property and Group filters are doing. See https://github.com/api-platform/docs/blob/master/core/filters.md#serializer-filters

jonday-simitive · 2017-12-01T13:44:58Z

@soyuka I could be wrong but it seems like the current implementation of JSON API will only provide me with the relationships property in the response if I add a related entity with a Group annotation for GET for example

{
  "links": {
    "self": "/api/cars/2"
  },
  "meta": {
    "totalItems": 1,
    "itemsPerPage": 25,
    "currentPage": 1
  },
  "data": [
    {
      "id": "/api/cars/2",
      "type": "Car",
      "attributes": {
        "modal": "Fiesta",
        "_id": 2,
      },
      "relationships": {
        "make": {
          "data": {
            "type": "Make",
            "id": "/api/make/77"
          }
        }
      }
    }
  ]
}

So a second API request need to be made. With the functionality that has yet to be implemented of the "include" query string to allow you to return:

{
  "links": {
    "self": "/api/cars/2?include=make"
  },
  "meta": {
    "totalItems": 1,
    "itemsPerPage": 25,
    "currentPage": 1
  },
  "data": [
    {
      "id": "/api/cars/2",
      "type": "Car",
      "attributes": {
        "model": "Fiesta",
        "_id": 2,
      },
      "relationships": {
        "make": {
          "data": {
            "type": "Make",
            "id": "/api/make/77"
          }
        }
      }
    }
  ],
  "included": [
    {
      "type": "make",
      "id": "88",
      "attributes": {
        "name": "Ford",
      }
    }
  ]
}

soyuka · 2017-12-01T14:24:03Z

Indeed, it's just very similar and the implementation of include could follow the same logic as in those filters :).

jonday-simitive · 2017-12-01T14:36:20Z

@soyuka Ah yeah I get you, sorry been a long day ... thought you were saying I could use them already to get the included functionality. I'll take a look at those now though, thanks.

cjunge-work · 2017-12-13T20:20:50Z

Just a note on the includes. They make caching really difficult. A change in an included entity requires any caching to be purged for all other entities that include it. IMHO it's one of the flaws with JSON-API and similar formats that support includes.

A better option is to adopt HTTP/2 for faster connections with less latency and cache at the individual entity level (aggregate in DDD).

Included entities is primarily about saving requests, but HTTP/2 makes requests concurrent and faster/lighter than HTTP/1. Includes make the responses bigger and harder to maintain and less cacheable.

Making the JSON-API support feature-complete compared to the spec is a good thing, but I thought I'd bring some attention to the issues associated with using this feature. My opinion is based on using includes in a project, and having some big issues.

bangbambang · 2017-12-14T14:58:07Z

@cjunge-work I think it's a common issue with embedded relations (i.e using serialization group), irrespective of the format used. And IMO the best way to deal with it is to be mindful on what to embed for each responses to prevent overcomplicating cache management logic.

I personally hope this functionality would be available to provide consistency when used with content negotiation.

deivid11 · 2018-01-17T23:24:04Z

Hey thanks for the feature! How to enable this json api support?.
EDIT: Found it, adding format to the api platform configuraiton jsonapi: mime_types: ['application/vnd.api+json'].
Would be nice to add it to the config reference https://api-platform.com/docs/core/configuration/

…platform#1175)

Ronnie-J · 2018-04-12T10:59:10Z

@jonday-simitive did you start working on this? I am in a situation where I need a subresource to be included with data.

jonday-simitive · 2018-04-12T11:13:34Z

@Ronnie-J We ended up using multiple API requests or the SubResources Api Platform functionality. I've been trying to find the time to implement it but been unable to so far :(

meyerbaptiste force-pushed the add_jsonapi_support branch from 61d1023 to fbcbcde Compare June 13, 2017 14:55

Simperfit reviewed Jun 13, 2017

View reviewed changes

meyerbaptiste force-pushed the add_jsonapi_support branch from fbcbcde to a91b570 Compare June 13, 2017 15:29

meyerbaptiste force-pushed the add_jsonapi_support branch from a91b570 to ec82832 Compare June 13, 2017 15:47

soyuka reviewed Jun 13, 2017

View reviewed changes

meyerbaptiste force-pushed the add_jsonapi_support branch from ec82832 to 86c1e58 Compare June 13, 2017 15:51

meyerbaptiste force-pushed the add_jsonapi_support branch 6 times, most recently from 8e34350 to 257522c Compare June 14, 2017 15:14

meyerbaptiste commented Jun 14, 2017

View reviewed changes

meyerbaptiste force-pushed the add_jsonapi_support branch 8 times, most recently from 2bcdfcc to 481636c Compare June 16, 2017 13:57

Simperfit approved these changes Jun 16, 2017

View reviewed changes

soyuka approved these changes Jun 19, 2017

View reviewed changes

soyuka mentioned this pull request Jun 19, 2017

Disable Hydra from my project? api-platform/api-platform#339

Closed

meyerbaptiste force-pushed the add_jsonapi_support branch from 1203621 to 938c0e0 Compare October 13, 2017 09:19

meyerbaptiste added 9 commits October 13, 2017 11:44

Add support for jsonapi format (api-platform#785, api-platform#1036)

9100c35

Fix support of jsonapi format

eed6008

Fix filters with JSON API

6c3f824

Fix tests

cb5a37e

Use IRI instead of plain IDs & refactor the item normalizer

e912519

Fix tests with deps=low (see symfony/symfony#18360)

daa6fb1

Fix CS

e6bfa50

Fix dunglas' comments

99a05ff

Change links format

4c37880

meyerbaptiste force-pushed the add_jsonapi_support branch from 938c0e0 to 4c37880 Compare October 13, 2017 09:45

Improve tests

2d40a08

meyerbaptiste merged commit 4b3d1ab into api-platform:master Oct 13, 2017

meyerbaptiste deleted the add_jsonapi_support branch October 13, 2017 15:06

hoangnd25 pushed a commit to hoangnd25/core that referenced this pull request Feb 23, 2018

Add JSON API basic support (api-platform#785, api-platform#1036, api-…

913f80a

…platform#1175)


		$typeShortName = '';

		if ($className && $this->resourceClassResolver->isResourceClass($className)) {


		$this->listener->onKernelRequest($eventProphecy->reveal());

		$expectedRequest = new Request(['page.size' => 5, 'page.number' => 3]);

Add JSON API basic support (continuation) #1175

Add JSON API basic support (continuation) #1175

Conversation

meyerbaptiste commented Jun 13, 2017 • edited by dunglas Loading

Choose a reason for hiding this comment

soyuka commented Jun 13, 2017

Simperfit commented Jun 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soyuka Jun 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meyerbaptiste Jun 14, 2017 • edited Loading

Choose a reason for hiding this comment

Simperfit commented Jun 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meyerbaptiste commented Jun 16, 2017

Simperfit commented Jun 16, 2017

hectorh30 commented Jun 19, 2017

meyerbaptiste commented Oct 13, 2017

Simperfit commented Oct 13, 2017

soyuka commented Oct 13, 2017

StephenOTT commented Oct 13, 2017

jonday-simitive commented Dec 1, 2017 • edited Loading

soyuka commented Dec 1, 2017

jonday-simitive commented Dec 1, 2017 • edited Loading

soyuka commented Dec 1, 2017

jonday-simitive commented Dec 1, 2017

cjunge-work commented Dec 13, 2017

bangbambang commented Dec 14, 2017

deivid11 commented Jan 17, 2018 • edited Loading

Ronnie-J commented Apr 12, 2018

jonday-simitive commented Apr 12, 2018

meyerbaptiste commented Jun 13, 2017 •

edited by dunglas

Loading

soyuka Jun 14, 2017 •

edited

Loading

meyerbaptiste Jun 14, 2017 •

edited

Loading

jonday-simitive commented Dec 1, 2017 •

edited

Loading

jonday-simitive commented Dec 1, 2017 •

edited

Loading

deivid11 commented Jan 17, 2018 •

edited

Loading