Allow blank nodes in JSON-LD #956

teohhanhui · 2017-02-24T12:34:47Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	TODO
Fixed tickets	N/A
License	MIT
Doc PR	TODO

TODO:

Tests
Docs

soyuka · 2017-02-24T13:02:09Z

src/Bridge/Symfony/Routing/IriConverter.php

    private $itemDataProvider;
    private $routeNameResolver;
    private $router;
-    private $propertyAccessor;
+    private $itemIdentifiersExtractor;

    public function __construct(PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, ItemDataProviderInterface $itemDataProvider, RouteNameResolverInterface $routeNameResolver, RouterInterface $router, PropertyAccessorInterface $propertyAccessor = null)


We should put a reminder somewhere that (when we can break things again) we need to remove those injections and rather directly inject ItemIdentifiersExtractor.

soyuka · 2017-02-24T13:04:31Z

src/JsonLd/Serializer/ItemNormalizer.php

-        $data['@id'] = $this->iriConverter->getIriFromItem($object);
+        $identifiers = $this->itemIdentifiersExtractor->getIdentifiersFromItem($object);
+
+        $data['@id'] = count(array_filter($identifiers)) > 0 ? $this->iriConverter->getIriFromItem($relatedObject) : $this->blankNodeIdentifiersGenerator->getBlankNodeIdentifier($relatedObject, $context['jsonld_document_root']);


Shouldn't we add a method that takes identifiers and resolves to one of those to avoid repetition?
Is $context['jsonld_document_root'] used only to cache the spl_object_hash?

count(array_filter($identifiers)) > 0 can be replaced by array_filter($identifiers) (faster, easier to read).

Shouldn't we add a method that takes identifiers and resolves to one of those to avoid repetition?

Sure. I'll move it to a private method.

Is $context['jsonld_document_root'] used only to cache the spl_object_hash?

We need to keep track of which JSON-LD document we're in. But of course there are many ways of doing it. I agree this is not ideal.

Yeah, nothing wrong with how you did it, just wanted to make sure I understood ;)

dunglas · 2017-02-24T16:29:33Z

src/JsonLd/Serializer/ItemNormalizer.php

-        $data['@id'] = $this->iriConverter->getIriFromItem($object);
+        $identifiers = $this->itemIdentifiersExtractor->getIdentifiersFromItem($object);
+
+        $data['@id'] = count(array_filter($identifiers)) > 0 ? $this->iriConverter->getIriFromItem($relatedObject) : $this->blankNodeIdentifiersGenerator->getBlankNodeIdentifier($relatedObject, $context['jsonld_document_root']);


count(array_filter($identifiers)) > 0 can be replaced by array_filter($identifiers) (faster, easier to read).

dunglas · 2017-02-24T16:30:56Z

src/JsonLd/Serializer/ItemNormalizer.php

+
+        $identifiers = $this->itemIdentifiersExtractor->getIdentifiersFromItem($relatedObject);
+
+        return count(array_filter($identifiers)) > 0 ? $this->iriConverter->getIriFromItem($relatedObject) : $this->blankNodeIdentifiersGenerator->getBlankNodeIdentifier($relatedObject, $context['jsonld_document_root']);


Same here array_filter($identifiers) is enough.

dunglas · 2017-02-24T16:34:27Z

src/JsonLd/Util/BlankNodeIdentifiersGenerator.php

+     */
+    public function getBlankNodeIdentifier($object, string $documentRootHash): string
+    {
+        $objectHash = spl_object_hash($object);


I'm not a fond of exposing such low level data from the system publicly. Is it really safe? Another potential problem is that this ID will be reused later by PHP. WDYT about using a UUID instead?

It's not being exposed in any way. The hash is only used to keep track of which objects we've already generated a blank node identifier for, and always return that same identifier for the same object (the scope is the JSON-LD document).

We cannot use UUID for this, as the point is to detect the same object instance.

Nevermind got it.

dunglas · 2017-02-24T16:35:25Z

src/Serializer/AbstractItemNormalizer.php

@@ -415,7 +415,7 @@ protected function getAttributeValue($object, $attribute, $format = null, array
     *
     * @return string|array
     */
-    private function normalizeRelation(PropertyMetadata $propertyMetadata, $relatedObject, string $resourceClass, string $format = null, array $context)
+    protected function normalizeRelation(PropertyMetadata $propertyMetadata, $relatedObject, string $resourceClass, string $format = null, array $context)


Can you mark this method as @internal?

dunglas · 2017-03-20T11:09:30Z

Can you rebase? It should be ok to be merged.

teohhanhui · 2017-03-20T11:20:01Z

@dunglas There are no tests yet.

soyuka · 2017-03-20T13:03:27Z

You do you own tests 😈

Joking, if you don't have the time lmk

teohhanhui · 2017-06-01T07:39:59Z

@dunglas I need some help rebasing this... There seems to have been a proliferation of IriConverterInterface::getIriFromItem to more places. Perhaps we need a different approach?

soyuka · 2017-06-01T12:45:56Z

src/Serializer/AbstractItemNormalizer.php

@@ -86,7 +96,7 @@ public function normalize($object, $format = null, array $context = [])
        $context = $this->initContext($resourceClass, $context);
        $context['api_normalize'] = true;

-        if (isset($context['resources'])) {
+        if (isset($context['resources']) && $this->hasIri($object)) {
            $resource = $context['iri'] ?? $this->iriConverter->getIriFromItem($object);


Is the condition still needed? if it hasIri context['iri'] is defined no?

No. $context['iri'] is only set from the JsonLd ItemNormalizer (I think). I don't know what's the reasoning behind, but I did not change it...

Resolved

soyuka · 2019-02-15T13:46:47Z

kinda fixed by #2495

teohhanhui added the enhancement label Feb 24, 2017

teohhanhui force-pushed the feature/jsonld-blank-nodes branch from e3decb3 to d064f7f Compare February 24, 2017 12:36

teohhanhui requested review from dunglas and soyuka February 24, 2017 12:38

teohhanhui force-pushed the feature/jsonld-blank-nodes branch from d064f7f to 30a9715 Compare February 24, 2017 12:41

soyuka reviewed Feb 24, 2017

View reviewed changes

dunglas previously requested changes Feb 24, 2017

View reviewed changes

teohhanhui force-pushed the feature/jsonld-blank-nodes branch from 30a9715 to 0648775 Compare February 27, 2017 04:39

soyuka added this to the 2.1.0 milestone Mar 22, 2017

teohhanhui force-pushed the feature/jsonld-blank-nodes branch 4 times, most recently from effd916 to 8d12b1b Compare May 5, 2017 07:32

teohhanhui force-pushed the feature/jsonld-blank-nodes branch 4 times, most recently from f74f04f to 58d7297 Compare June 1, 2017 12:07

soyuka reviewed Jun 1, 2017

View reviewed changes

teohhanhui force-pushed the feature/jsonld-blank-nodes branch from 58d7297 to 56b76f4 Compare June 7, 2017 02:40

teohhanhui mentioned this pull request Jul 19, 2017

Exception during normalization of embedded entities where not all are ApiResources #1222

Closed

Allow blank nodes in JSON-LD

4c6d144

teohhanhui force-pushed the feature/jsonld-blank-nodes branch from 56b76f4 to 4c6d144 Compare July 21, 2017 07:55

soyuka added the waiting label Nov 6, 2017

soyuka mentioned this pull request Dec 4, 2017

Denormalization behavior with embedded models hofff/json-ld#1

Open

soyuka closed this Feb 15, 2019


		$identifiers = $this->itemIdentifiersExtractor->getIdentifiersFromItem($relatedObject);

		return count(array_filter($identifiers)) > 0 ? $this->iriConverter->getIriFromItem($relatedObject) : $this->blankNodeIdentifiersGenerator->getBlankNodeIdentifier($relatedObject, $context['jsonld_document_root']);

Uh oh!

Allow blank nodes in JSON-LD #956

Allow blank nodes in JSON-LD #956

Uh oh!

Conversation

teohhanhui commented Feb 24, 2017

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!

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!

Choose a reason for hiding this comment

Uh oh!

dunglas commented Mar 20, 2017

Uh oh!

teohhanhui commented Mar 20, 2017

Uh oh!

soyuka commented Mar 20, 2017

Uh oh!

teohhanhui commented Jun 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soyuka commented Feb 15, 2019

Uh oh!

Uh oh!