Allow plain IDs with `allow_plain_identifiers` by soyuka · Pull Request #1365 · api-platform/core

soyuka · 2017-09-12T13:42:03Z

Q	A
Bug fix?	yes/no (depends on the POV)
New feature?	yes/no (same)
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	api-platform/api-platform#249 #1363
License	MIT
Doc PR	n/a

soyuka · 2017-09-12T14:12:15Z

        }

        if (!is_array($value)) {
+            // repeat the code so that IRIs keep working with the json format


I'd like to test that Content-Type is application/json here, not sure if it's possible or wanted...

you can just check the $format var.

not really, json is the format for hal, jsonld and json no?

No, it's jsonld for JSON-LD and jsonhal for HAL.

Okay, my bad dunno why I thought so...

dunglas · 2017-09-19T06:46:20Z


        if (!is_array($value)) {
+            // repeat the code so that IRIs keep working with the json format
+            if ('json' === $format && $this->itemDataProvider) {


I get the idea and the use case, but it hurts me a bit:

it doesn't fit the hypermedia philosophie of API Platform

it doesn't work consistently across formats (XML or CSV for instance may need this feature as well)

May I suggest an alternative implementation:

add a new flag allow_plain_identifiers defaulting to false

if this flag is on, execute your code in all circonstances, regardless of the format

And then it will be merged in master.

You see :p had the same concerns ^^

teohhanhui · 2017-09-19T10:45:31Z

I don't think we need a config option. @id in plain JSON (both normalization and denormalization) is a bug. For BC, we should keep it but trigger a deprecation error.

soyuka · 2017-09-19T11:56:53Z

To you we should:

enable @id only for json-ld
enable id only for json, xml, csv

@dunglas wdyt? I'm also to remove @id support in other formats.

teohhanhui · 2017-09-19T12:07:16Z

There is no question that @id is defined in the JSON-LD spec. For all of the plain (non-hypermedia) formats (JSON, XML, etc.), there is not really a right or wrong thing to do, but using the item's ID properties seems appropriate.

soyuka · 2017-09-19T12:10:18Z

Agreed @teohhanhui, though IRIs are such great at identifying things that it'd be a shame not to be able to use them for other things (might be a point to keep them with other formats).

soyuka · 2017-09-19T12:11:55Z

For now I can go back to my previous commit to do:

if ('jsonld' !== $format && $this->itemDataProvider) {
 //use id 
}

dunglas · 2017-09-19T12:48:24Z

I don't agree:

We already decided to use IRIs as identifiers everywhere. I'm ok to add an option to change this behavior in write mode (= support both plain IDs and IRIs), but not to remove support of IRIs in other formats than JSON-LD and HAL.
In JSON-LD (and HAL), we should give the possibility to access to both the raw id property and to the @id IRI. It's mandatory to support client-side generated UUIDs for instance.

soyuka · 2017-09-19T12:53:09Z

In that case @dunglas feel free to merge! I see no issues in keeping @id and id :)

dunglas · 2017-09-19T12:54:39Z

+    protected $allowPlainIdentifiers;

-    public function __construct(PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, IriConverterInterface $iriConverter, ResourceClassResolverInterface $resourceClassResolver, PropertyAccessorInterface $propertyAccessor = null, NameConverterInterface $nameConverter = null, ClassMetadataFactoryInterface $classMetadataFactory = null)
+    public function __construct(PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, IriConverterInterface $iriConverter, ResourceClassResolverInterface $resourceClassResolver, PropertyAccessorInterface $propertyAccessor = null, NameConverterInterface $nameConverter = null, ClassMetadataFactoryInterface $classMetadataFactory = null, ItemDataProviderInterface $itemDataProvider = null, $allowPlainIdentifiers = false)


bool $allowPlainIdentifiers = false

meyerbaptiste · 2017-09-20T08:26:01Z

Should we use IRIs as identifiers with JSON API too?

Simperfit · 2017-10-09T14:31:38Z

@dunglas could you re-review it ? @soyuka could you rebase ?

soyuka · 2017-10-10T09:02:18Z

rebased

sroze · 2017-10-10T09:34:59Z

I really don't think this is a great choice to do so. What's the point? It's just adding another way to do something already possible... If you look at #1363 it's just because "I don't want to use this code" or in api-platform/api-platform#249 the reason is "I think plain IDs will be used for many years to come".

We are convinced that semantic web is the way to go. The way to properly go towards to this direction is to have people using IRIs as their identifiers... let's educate for this instead of allow "plain IDs" :(

soyuka · 2017-10-10T09:59:25Z

@sroze totally agree when it comes to formats like json-ld. Though with standard json, to me, accepting an id (not an IRI) should be valid.

glnemeth · 2017-10-10T10:04:02Z

@sroze This is needed for example in our project, where we are currently using JSON (not LD-JSON), so having IRIs is not required and forcing IRIs on our developers is unneccessary and adds to the work. Also our routing is not fixed as the project is still evolving, so it's easier to send IDs, instead of constantly following the changes of routing on frontend and backend too.

sroze · 2017-10-10T13:05:46Z


        if (!is_array($value)) {
+            // repeat the code so that IRIs keep working with the json format
+            if (true === $this->allowPlainIdentifiers && $this->itemDataProvider) {


What is $this->itemDataProvider === null and true === $this->allowPlainIdentifiers ? Instead of silently not doing anything, should we throw an exception instead?

We could, maybe a bit overkill, this dependency should be a hard dep, it's just nullable to avoid breaking.

sroze · 2017-10-10T13:06:53Z

Also our routing is not fixed as the project is still evolving, so it's easier to send IDs, instead of constantly following the changes of routing on frontend and backend too.

That's an interesting argument because that's actually where using IRIs should help you :)

But fair point for the JSON format. If it helps... 👍 then...

iciantoine · 2017-11-07T11:25:02Z

Hello guys. Is it possible to merge this in 2.1 branch? I think this is a really important feature and I hope to see it in stable version ASAP. Thanks!

dunglas · 2017-11-07T11:34:21Z

Sorry we respect semver and new features are always merged in minor releases. Patch releases only contain bug and security fixes.

julienpillias · 2017-11-09T12:59:49Z

According to this PR, I should be able to update (PUT) nested document with plain id ?
Just tried, a, dstille have this error :

Nested documents for attribute "groups" are not allowed. Use IRIs instead

{
  "id":1,
  "name": "foo",
  "groups": [
       {
           "id":1,
           "name": "bar"
       }
   ]
}

Composer.json :

"api-platform/core": "2.2.x-dev"

(composer update done)

config.sjon

api_platform:
    allow_plain_identifiers: true

Forget something ? Yet not ready maybe ?

soyuka · 2017-11-09T13:07:00Z

@julienpillias you have a serialization issue fix your groups and this will indeed work.

julienpillias · 2017-11-09T13:16:58Z

OK, had to put denormalization_context into the nested document too. TY for the tips
But now, having this error :

No route matches "/app_dev.php/groups/1"

This route exists, but should be http://api.xxxxx.project.local/app_dev.php/groups/1, because of my routing I think :

api:
    resource: '.'
    type:     'api_platform'
    host:     'api.{project_name}'
    defaults:
        project_name: xxxx.project.local
    requirements:
        project_name: xxxx.project.local|xxxx.fr|preprod.xxxx.fr

I don't see any option in apiPlatform to specify a route parameter. At least, is it possible to use the default parameter ?

julienpillias · 2017-11-09T16:19:39Z

So just dig into the code, and found this in https://github.com/api-platform/core/blob/master/src/Bridge/Symfony/Routing/IriConverter.php (Line 141) :

public function getSubresourceIriFromResourceClass(string $resourceClass, array $context, int $referenceType = UrlGeneratorInterface::ABS_PATH): string

Change it to

public function getSubresourceIriFromResourceClass(string $resourceClass, array $context, int $referenceType = UrlGeneratorInterface::ABS_URL): string

(ABS_PATH => ABS_URL).
No more problem !
I do not know if this could have some side effect !
Should we use ABS_URL every where in iriConverter to avoid problem like mine ?

EDIT : maybe still having the problem.... digging in

soyuka · 2017-11-09T17:04:21Z

(ABS_PATH => ABS_URL).

this should be configurable I think but it is not actually :|.

julienpillias · 2017-11-09T17:06:49Z

could try a PR for this.

Something in config.json ?
config.json

api_platform:
    use_absolute_url: true # default will be false

soyuka · 2017-11-09T17:29:02Z

Not sure, I think that this is a configuration issue on your side but I have no idea what (something linked to symfony routing configuration) :|.

julienpillias · 2017-11-09T17:34:06Z

On my project, I have a "classic" web site (SF + twig), using mysite.com (or www.mysite.com) AND have the API on api.mysite.com for mobile application for example.

So if apiPlatform check route /entity/id, it would not be an API ressource. It's why I need ABS_URL their, so ressource will be found with http://api.mysite.com/entity/id

Don't think I have a routing problem here, i've checked router:debug, and do not have any problem with api doc, or my "classic" web site".

soyuka · 2017-11-10T08:12:48Z

Can you open an issue about this with all the details? Thanks.

julienpillias · 2017-11-10T13:45:29Z

Already done few week's ago :)
api-platform/api-platform#427

Allow plain IDs with `allow_plain_identifiers`

soyuka force-pushed the fix/api-platform/249 branch from e7218e0 to 6f862a7 Compare September 12, 2017 14:09

soyuka commented Sep 12, 2017

View reviewed changes

soyuka force-pushed the fix/api-platform/249 branch 6 times, most recently from 3f4a223 to 0cbf4dd Compare September 18, 2017 18:11

soyuka changed the base branch from master to 2.1 September 18, 2017 18:11

soyuka force-pushed the fix/api-platform/249 branch from 0cbf4dd to 5e8619f Compare September 18, 2017 18:14

soyuka requested review from dunglas and meyerbaptiste September 19, 2017 06:17

dunglas reviewed Sep 19, 2017

View reviewed changes

soyuka changed the base branch from 2.1 to master September 19, 2017 07:52

soyuka force-pushed the fix/api-platform/249 branch 2 times, most recently from 43017af to 6a11dd7 Compare September 19, 2017 08:09

soyuka changed the title ~~Allow plain IDs when the format is "json"~~ Allow plain IDs with allow_plain_identifiers Sep 19, 2017

dunglas requested changes Sep 19, 2017

View reviewed changes

soyuka force-pushed the fix/api-platform/249 branch from 6a11dd7 to 965ff42 Compare September 19, 2017 12:59

Simperfit approved these changes Sep 19, 2017

View reviewed changes

meyerbaptiste approved these changes Sep 21, 2017

View reviewed changes

soyuka force-pushed the fix/api-platform/249 branch from 965ff42 to b28d524 Compare September 21, 2017 11:47

soyuka force-pushed the fix/api-platform/249 branch from b28d524 to 44b6fab Compare October 10, 2017 08:14

sroze reviewed Oct 10, 2017

View reviewed changes

dunglas approved these changes Oct 11, 2017

View reviewed changes

soyuka force-pushed the fix/api-platform/249 branch from 44b6fab to ac4622c Compare October 12, 2017 06:23

Allow plain IDs with allow_plain_identifiers

808b5d3

soyuka force-pushed the fix/api-platform/249 branch from ac4622c to 808b5d3 Compare October 13, 2017 08:14

soyuka merged commit 318d3b3 into api-platform:master Oct 13, 2017

soyuka deleted the fix/api-platform/249 branch October 13, 2017 09:31

Simperfit mentioned this pull request Oct 20, 2017

Use plain IDs api-platform/api-platform#249

Closed

dunglas mentioned this pull request Feb 15, 2018

Resource ID's - support for non-url IRIs api-platform/api-platform#569

Closed

ewgRa mentioned this pull request Feb 19, 2018

throw NotFound exception when use plain identifiers #1716

Closed

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

Merge pull request api-platform#1365 from soyuka/fix/api-platform/249

c664c3a

Allow plain IDs with `allow_plain_identifiers`

Uh oh!

Uh oh!

Conversation

soyuka commented Sep 12, 2017

Uh oh!

soyuka Sep 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dunglas Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teohhanhui commented Sep 19, 2017

Uh oh!

soyuka commented Sep 19, 2017

Uh oh!

teohhanhui commented Sep 19, 2017

Uh oh!

soyuka commented Sep 19, 2017

Uh oh!

soyuka commented Sep 19, 2017

Uh oh!

dunglas commented Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soyuka commented Sep 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meyerbaptiste commented Sep 20, 2017

Uh oh!

Simperfit commented Oct 9, 2017

Uh oh!

soyuka commented Oct 10, 2017

Uh oh!

sroze commented Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soyuka commented Oct 10, 2017

Uh oh!

glnemeth commented Oct 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sroze commented Oct 10, 2017

Uh oh!

iciantoine commented Nov 7, 2017

Uh oh!

dunglas commented Nov 7, 2017

Uh oh!

julienpillias commented Nov 9, 2017

Uh oh!

soyuka commented Nov 9, 2017

Uh oh!

julienpillias commented Nov 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

julienpillias commented Nov 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soyuka commented Nov 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

julienpillias commented Nov 9, 2017

Uh oh!

soyuka Sep 12, 2017 •

edited

Loading

dunglas Sep 19, 2017 •

edited

Loading

dunglas commented Sep 19, 2017 •

edited

Loading

sroze commented Oct 10, 2017 •

edited

Loading

julienpillias commented Nov 9, 2017 •

edited

Loading

julienpillias commented Nov 9, 2017 •

edited

Loading

soyuka commented Nov 9, 2017 •

edited

Loading